Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: improve inferred types for selector subscriptions #5096

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Dec 20, 2024

Explanation

This was a low hanging fruit to investigate the subscription selector/handler inference (for fun during Quality Quest).

Currently if you do not provide explicit type in the selector, then the handler function will have unknown params. This is because TS does not know which which function (handler or selector) to assign the generic type SelectorReturnValue.

Here is a TypeScript Playground with a simplified example.
Playground Link

Before After
Screenshot 2024-12-20 at 15 40 09 Screenshot 2024-12-20 at 15 57 24

References

Changelog

@metamask/base-controller

  • CHANGED: Modified SelectorEventHandler to use a deferred version of SelectorReturnValue, so that TS will pick up and use the selectors return value as the Generic.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

currently if you do not provide explicit type in the selector, then the handler function will have `unknown` params.
This is because TS does not know which generic type to use for `SelectorReturnValue`.

I tried using the `NoInfer` type to prevent the handler function from winning the inference, but unfortunately `NoInfer` does not work with return values.

The other option was to switch the handler fn and selector fn around, however this is a destructive change.

By making the handler fn use its own generics (which are constrained by the original `SelectorReturnValue`), we effectively defer or ensure that the handler doesn't win the inference.
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team as a code owner December 20, 2024 16:10
@Prithpal-Sooriya Prithpal-Sooriya changed the title refactor: improve intellisense for selector subscriptions refactor: improve inferred types for selector subscriptions Dec 20, 2024
@Prithpal-Sooriya
Copy link
Contributor Author

Welp, NVM this fails.

It works in the typescript playground (looking at types). But then it breaks when trying to use it lol

@mcmire
Copy link
Contributor

mcmire commented Dec 21, 2024

This is something that's bothered me for a while, so I appreciate you trying to figure it out at least!

@Gudahtt
Copy link
Member

Gudahtt commented Dec 21, 2024

Agreed, thanks! That unknown inferred type is unfortunate. Let's revisit this, we'll need to solve this somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants