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: increase typescript state resolver limit #787

Merged

Conversation

no-stack-dub-sack
Copy link
Collaborator

This PR addresses the comment by @tormodAase in regards to the limit placed upon the numbers of state values that unstable_effectOn can track for TypeScript users. As the underlying StateResolvers type is shared among computed and unstable_effectOn this change increases the number of supported state resolvers from 6 to 15. One might argue that anything over 10 is bad design, but there may still be limited use-cases for effectOn especially.

It should be noted, however, that if someone does try to exceed this limit, TypeScript simply won't compile without a @ts-ignore. There is an alternative approach which supports N number of resolvers, but which loses inference for the Nth + 1 computed func arg or change array member (for computed and effectOn, respectively). You can find that change (built upon this change) here. If this is preferred, I can merge these changes and squash them into a single commit.

  • update StateResolvers union to support an array up to 15 StateResolvers
  • update computed function's compFunc conditional to allow the maximum number of state resolvers
  • update unstable_EffectOn's Dependencies type to allow the maximum number of state resolvers
  • add state-resolvers typescript test to validate that compilation succeeds with up to 15 state resolvers and fails thereafter

* update  `StateResolvers` union to support an array up to 15
`StateResolver`s

* update `computed` function's `compFunc` conditional to allow the
maximum number of state resolvers

* update `unstable_EffectOn`s `Dependencies` type to allow the
maximum number of state resolvers

* add state-resolvers typescript test to validate that compilation
succeeds with up to 15 state resolvers and fails thereafter
@vercel
Copy link

vercel bot commented Oct 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
easy-peasy ✅ Ready (Inspect) Visit Preview Oct 31, 2022 at 1:41PM (UTC)

Copy link
Collaborator

@jmyrland jmyrland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! I'd explore if it is possible to reduce the type definitions by using a dynamic type - but I would have to look more into this to see if it is feasible.

@ctrlplusb can you take a look at this as well?

@jmyrland
Copy link
Collaborator

jmyrland commented Nov 2, 2022

If we're going for the dynamic state-resolvers we should probably update the description & maybe review the docs to see if there is any mention of the previous limitation.

As stated in the description, we should advice against using excessive state resolvers (>6?).

@no-stack-dub-sack
Copy link
Collaborator Author

If we're going for the dynamic state-resolvers we should probably update the description & maybe review the docs to see if there is any mention of the previous limitation.

As stated in the description, we should advice against using excessive state resolvers (>6?).

I agree - I can comb through the docs as well, but I don't think I remember anything about a limit, as this is just a TS limit anyway. So now TypeScript users will just have the same ability as JS users to abuse state resolvers as much as they want :-)

Worth mentioning it would be considered bad practice though.

@jmyrland jmyrland merged commit eba7fc1 into ctrlplusb:master Nov 13, 2022
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.

2 participants