Fixed bug causing routes not to render on change #224
Merged
Conversation
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
pancaspe87
reviewed
Feb 29, 2024
| throw new Error('No root element found to render basic routing example'); | ||
|
|
||
| const root = createRoot(container); | ||
| root.render(<App />); |
Collaborator
There was a problem hiding this comment.
How are we testing if it works with StrictMode ?
Collaborator
Author
There was a problem hiding this comment.
See integration tests file
Collaborator
There was a problem hiding this comment.
yeahhh, I was looking at it now
pancaspe87
previously approved these changes
Feb 29, 2024
src/controllers/router/index.tsx
Outdated
| }: RouterProps) => { | ||
| useEffect(() => { | ||
| const { unlisten } = getRouterState(); | ||
| const { unlisten } = getRouterState(); |
Collaborator
There was a problem hiding this comment.
Fine, even if I think the problem is the use of getRouterState which is bad (as out of react control flow)
albertogasparin
previously approved these changes
Feb 29, 2024
…ime to unmount in test
AlexanderKaran
dismissed stale reviews from albertogasparin and pancaspe87
via
306fe69
pancaspe87
approved these changes
Feb 29, 2024
|
Cloned locally and confirmed that it works with/without strict mode in R18 concurrent mode for basic-routing example |
JohnsonChauAtlassian
approved these changes
Feb 29, 2024
albertogasparin
approved these changes
Feb 29, 2024
AlexanderKaran
deleted the
222-doesnt-not-work-with-react-18-strictmodel
branch
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.
Fixes: #224
useEffectthat was stopping correct rendering in<StrictMode>I did notice that the Hydration example is broken (even before the changes). I logged a bug for this: #223