Fixed an issue with mounting a parcel when StrictMode is enabled#206
Fixed an issue with mounting a parcel when StrictMode is enabled#206internettrans merged 4 commits intosingle-spa:mainfrom
Conversation
🦋 Changeset detectedLatest commit: cf6b87d 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 |
|
We are anxiously waiting for this PR to land. We've tested it and it seems to work fine. |
|
I will take a look. |
|
Hi @MilanKovacic did you get a chance to take a look? We'd very much appreciate it! |
There was a problem hiding this comment.
Can we add a new test that renders in strict mode using the https://testing-library.com/docs/react-testing-library/api/#configure-option option, and leave existing tests as-is.
Reason for this is that strict mode alters the behaviour, and tests no longer test how library will behave in production.
There was a problem hiding this comment.
Makes sense, I have pushed the changes where I updated @testing-library/react to 13.4.0. This is the version that added the proposed configure function and I also updated the parcel test where it run all the test with and without strict mode enabled.
|
Thank you @MilanKovacic — appreciated. @nickbosland if you get chance to take a look at the feedback that would be great, or equally we'd be happy to submit a PR if you're short on time. Thanks all |
MilanKovacic
left a comment
MilanKovacic
left a comment
There was a problem hiding this comment.
Looks good. Thank you.
|
@nickbosland are you OK to merge this if you have rights? TY |
|
@MilanKovacic what are the next steps to get this released ? |
|
It is waiting for approval from Joel. |
internettrans
left a comment
internettrans
left a comment
There was a problem hiding this comment.
The code change makes sense. I've left one comment on the tests.
| } | ||
| } | ||
| componentDidMount() { | ||
| this.unmounted = false; |
There was a problem hiding this comment.
I ran this code locally to see which tests failed in strict mode when this line was commented versus uncommented.
All tests pass regardless of whether this line of code is included. Please add a new test that demonstrates that this.unmounted = false is necessary.
There was a problem hiding this comment.
Thanks for the CC. I somehow missed the notification.
I have updated the @testing-library/react to 14.2.0 which actually adds the option to configure whether strict mode is used in the tests or not. I also messed up the property name of the option somehow 🤦. I had to move the configure to the beforeEach as it does not work in the beforeAll.
Now multiple tests fail when this is line is removed.
There was a problem hiding this comment.
Thanks @nickbosland — cc @joeldenning would you be happy to proceed with the PR now? Appreciate you taking a look!
There was a problem hiding this comment.
@MilanKovacic @joeldenning any chance this can be reviewed again ?
There was a problem hiding this comment.
I've contacted Joel to re-review it.
5 participants
StrictMode in React 18 renders a component an extra time where the component is unmounted and mounted again. This sets the
unmountedvalue totruewhich prevent to component from actually mounting.Fixes #160