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

zustand does not behave properly with useLayoutEffect #276

Closed
leobastiani opened this issue Jan 7, 2021 · 5 comments · Fixed by #280
Closed

zustand does not behave properly with useLayoutEffect #276

leobastiani opened this issue Jan 7, 2021 · 5 comments · Fixed by #280
Labels
good first issue Good for newcomers

Comments

@leobastiani
Copy link
Contributor

leobastiani commented Jan 7, 2021

IMO this codesandbox should not blink a red page

https://codesandbox.io/s/sad-currying-wwrm8

Is it an expected behavior?

@dai-shi
Copy link
Member

dai-shi commented Jan 7, 2021

Nice catch. I'd say it's not a desired behavior, but it's the expected behavior from the current implementation.

useEffect(() => {

It subscribes in useEffect and to mitigate when it's changed before subscription it checks at L105.

To fix this behavior, we'd need to change this useEffect to useLayoutEffect, but I'm not sure if it's worth it.

Note this behavior is only for the mount time, once subscription is made, useLayoutEffect should work properly, I suppose.

@leobastiani leobastiani changed the title zustand does not behave properly useLayoutEffect zustand does not behave properly with useLayoutEffect Jan 8, 2021
@leobastiani
Copy link
Contributor Author

leobastiani commented Jan 8, 2021

Thank you!
It might be a hard decision to change it or not but I do think it should be changed or at least to have a caveat
I think that useIsoLayoutEffect seems to be more appropriated there.

@dai-shi
Copy link
Member

dai-shi commented Jan 8, 2021

Hm, in one of my libs, I used LayoutEffect as there's no other choices.

https://github.com/dai-shi/use-context-selector/blob/65eaf66e27555a78f3f46397f535c8b03f0b7899/src/index.ts#L169

useSubscription from react team uses passive Effect intentionally.

https://github.com/facebook/react/blob/d17086c7c813402a550d15a2f56dc43f1dbd1735/packages/use-subscription/src/useSubscription.js#L70

Maybe because it doesn't know the cost of subscribe.

Finally, react-redux uses LayoutEffect.

https://github.com/reduxjs/react-redux/blob/a9235530f4799dd4b2acb3cc65e9caf32efbc44b/src/hooks/useSelector.js#L55


As we know subscribe in zustand is pretty lightweight, we can change it to useIsomorphicLayoutEffect. (It would be obsoleted in v4-experimental #160 anyway.)

Anyone wants to open a PR? (Preferably with a test)

@dai-shi dai-shi added the good first issue Good for newcomers label Jan 8, 2021
@leobastiani
Copy link
Contributor Author

I'm working on it right now 🤗

@leobastiani
Copy link
Contributor Author

#280

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

Successfully merging a pull request may close this issue.

2 participants