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

createPortal()'s state merging uses stale data #3253

Closed
notrabs opened this issue May 7, 2024 · 1 comment · Fixed by #3255
Closed

createPortal()'s state merging uses stale data #3253

notrabs opened this issue May 7, 2024 · 1 comment · Fixed by #3255
Labels
bug Something isn't working

Comments

@notrabs
Copy link
Contributor

notrabs commented May 7, 2024

Hi there,

when using createPortal(children, scene, { camera }) with a custom state override (e.g. for the camera), the state gets reverted back to its initial value, when the parent-store updates. This also happens for any other state override you provide there.

Here is a sandbox with a minimal reproduction. There the camera within the portal should not change, when the parent store is updated.

image

This bug is only visible, when the createPortal component is not also immeditaly re-rendered by the store update. In that case, the re-render overwrites the state again with the correct values.

image


The root-cause seems to lie in this effect:

React.useEffect(() => {
  // Subscribe to previous root-state and copy changes over to the mirrored portal-state
  const unsub = previousRoot.subscribe((prev) => usePortalStore.setState((state) => inject(prev, state)))
  return () => {
    unsub()
    usePortalStore.destroy()
  }
  // eslint-disable-next-line react-hooks/exhaustive-deps
}, [])

It is missing a dependency on the inject function, which means that it always uses the initial state data to do the state merge.

If I understand it right, I think the hook needs to be split in two:

React.useEffect(() => {
  // Subscribe to previous root-state and copy changes over to the mirrored portal-state
  const unsub = previousRoot.subscribe((prev) => usePortalStore.setState((state) => inject(prev, state)))
  return () => {
    unsub()
  }
}, [inject])

React.useEffect(() => {
  return () => {
    usePortalStore.destroy()
  }
}, [])
@CodyJasonBennett CodyJasonBennett added the bug Something isn't working label May 7, 2024
@CodyJasonBennett
Copy link
Member

That looks good to me if you want to send a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants