Fix React adapter in SSR and when rerendering #355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: eba0b93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
andrewiggins
force-pushed
the
react-adapter-fixes
branch
from
b91bdec to
c419f11
Compare
andrewiggins
requested review from
marvinhagemeister,
developit,
JoviDeCroock and
rschristian
|
Size Change: +378 B (+1%) Total Size: 68.9 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Apr 21, 2023
Comment on lines
+43
to
+50
| coverage && [ | ||
| "istanbul", | ||
| { | ||
| // TODO: Currently NodeJS tests always run against dist files. Should we | ||
| // change this? | ||
| // include: minify ? "**/dist/**/*.js" : "**/src/**/*.{js,jsx,ts,tsx}", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Note: coverage isn't reported from this test run and I got tired and didn't set up running tests against source. If someone is feeling motivated and wants to do so, please do!
…y waiting until next frame to return from our fake act method
andrewiggins
force-pushed
the
react-adapter-fixes
branch
from
a58ac34 to
eba0b93
Compare
JoviDeCroock
approved these changes
Apr 23, 2023
rschristian
reviewed
Apr 23, 2023
Comment on lines
+190
to
+200
| /* | ||
| Below is a state machine definition for transitions between the various | ||
| dispatchers in React's prod build. (It does not include dev time warning | ||
| dispatchers which are just always ignored). | ||
|
|
||
| ENTER and EXIT suffixes indicates whether this ReactCurrentDispatcher transition | ||
| signals we are entering or exiting a component render, or if it doesn't signal a | ||
| change in the component rendering lifecyle (NOOP). | ||
|
|
||
| ```js | ||
| // Paste this into https://stately.ai/viz to visualize the state machine. |
rschristian
approved these changes
Apr 23, 2023
marvinhagemeister
approved these changes
Apr 24, 2023
|
Haha @rschristian definitely some cursed knowledge for sure 🧙🪄🔮 |
Merged
|
There are really hard job with researching weird react internals. Really appreciate. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the react adapter to handle SSR and components that call setState while rendering.
I've also added a suite of mocha test that run in NodeJS and use react-dom/server to validate our adapter works in that environment.
Note:
@preact/signals-reactpackage can not be lazily imported. It must be imported before you callReactDOM.render,ReactDOM.hydrate,renderToStaticMarkup, etc.createElementand related methods to handle passing signals as attributes to DOM elements.Fixes #350