Revert react integration to tracking current dispatcher - #335
Merged
Conversation
🦋 Changeset detectedLatest commit: 465cc0c 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-update
branch
from
March 30, 2023 06:20
96f680e to
8415b8a
Compare
Contributor
|
Size Change: +121 B (0%) Total Size: 68.5 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Mar 30, 2023
…null as a signal for entering and exiting a component render
…ly and valid dispatchers
andrewiggins
force-pushed
the
react-adapter-update
branch
from
March 30, 2023 06:25
8415b8a to
b158661
Compare
andrewiggins
marked this pull request as ready for review
March 30, 2023 06:33
andrewiggins
commented
Mar 30, 2023
|
|
||
| import { signal } from "@preact/signals-react"; | ||
| import { createElement } from "react"; | ||
| import { Route, Routes, MemoryRouter } from "react-router-dom"; |
Member
Author
There was a problem hiding this comment.
I was lazy and just pulled in react-router-dom to write the test for the current issue. I could instead write a minimal reproduction if people would like lol
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@preact/signals-react": minor | |||
Member
Author
There was a problem hiding this comment.
Making this a minor bump since it is a non-trivial but also non-breaking change to the react package
Contributor
|
This is really good job |
andrewiggins
commented
Mar 30, 2023
… implementation for those versions of React
…ly provide a better devtools experience when the hooks show up in React DevTools
JoviDeCroock
approved these changes
Mar 31, 2023
Contributor
|
When it will be merged? |
|
Hoping it fixes some of my issues as well. |
Member
Author
|
Gonna go ahead and merge this now. Happy to address any feedback afterwards though. Just @-mention in any comments :) |
Merged
|
@andrewiggins Thanks a lot for fixing that issue :D |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Revert our react integration back to setting up signal subscription tracking by watching changes in the ReactCurrentDispatcher instead of proxying function Component calls in createElement so it works with more ecosystem libraries such as react-router-dom.
Check the comment in the source for a deeper discussion of the implementation details and choices.
Fixes #251
Fixes #330