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

fix(react): track owners separately, mutate updaters with dispatcher #130

Merged

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Sep 12, 2022

Fixes #125
Fixes #135
Fixes #70

This PR tracks ReactCurrentOwner separately from ReactCurrentDispatcher since the former is not guaranteed to be populated when ReactCurrentDispatcher is mutated. This is very evident from #125, and you can verify by running against a production build of React. Annoyingly, this behavior is different than that of a development build of React.

Additionally, signals' updaters are mutated on subsequent runs since there are multiple dispatchers during the lifecycle of a component, and holding on to a stale dispatcher can break the re-rendering mechanism (it wouldn't do anything).

Note: The relevant dispatcher states are invalid, mount, update, re-render -- invalid and re-render are skipped by isInvalidHookAccessor and lock, respectively. These are unique per reconciler, so #115 would have two pairs of mount and update dispatchers.

The changes in this PR originally came from tweaking with v1 JS output, which you can find in this gist. This is runnable if you want to copy/paste it into a Codesandbox, although ensure you're using the old JSX transform (React.createElement) if possible for the moment.

@changeset-bot
Copy link

changeset-bot bot commented Sep 12, 2022

⚠️ No Changeset found

Latest commit: e3ead4a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Sep 12, 2022

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit e3ead4a
🔍 Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/632157c85d0c350008e6105b
😎 Deploy Preview https://deploy-preview-130--preact-signals-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review September 13, 2022 02:23
@CodyJasonBennett CodyJasonBennett changed the title fix(react): track owners separately, create updaters per dispatcher fix(react): track owners separately, mutate updaters with dispatcher Sep 13, 2022
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This looks great! Let's add one or few test cases that ensure that we don't regress on this in the future 👍

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Sep 14, 2022

Would it be fine to settle for cases that cover the linked issues (doesn't crash in production, will consistently re-render in both production/strict-mode) or would you prefer to re-run the test suite (IDK how you'd sanely configure a testing matrix or mock/clear dependencies here)?

I'll commit the first in a moment as a baseline.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Sep 14, 2022

Would it be fine to settle for cases that cover the linked issues

Sounds good enough to me 👍

Instead of mocking dependencies we could add a new alias to something like react-production that points to the production build. I'm good with merging it as is and we can always PR more on top later. What do you think?

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for your PR 🙌

@CodyJasonBennett
Copy link
Contributor Author

We'll have to trust Netlify CI in lieu of a controlled production environment.

@marvinhagemeister marvinhagemeister merged commit 8643e41 into preactjs:main Sep 14, 2022
@CodyJasonBennett CodyJasonBennett deleted the fix/react-stale-dispatcher branch September 14, 2022 10:54
marvinhagemeister added a commit that referenced this pull request Sep 14, 2022
marvinhagemeister added a commit that referenced this pull request Sep 14, 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
2 participants