Fix StrictMode in React 18 #255
Conversation
🦋 Changeset detectedLatest commit: a566a8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
@sarneeh Friendly ping - any thoughts on the approach I took here? |
Codecov Report
@@ Coverage Diff @@
## master #255 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 321 320 -1
Branches 37 37
=========================================
- Hits 321 320 -1
Continue to review full report at Codecov.
|
|
@jgoz Thank you very much for you contribution 🙏 I'm sorry for the late reply, I didn't have too much time to take a look at this.
I have to move the tests to something different (probably RTL) anyways as Enzyme seems dead. For now using the unofficial adapter is fine. |
| @@ -78,6 +77,7 @@ const PROPS_THAT_SHOULD_CAUSE_RERENDER: Array<keyof RecaptchaBaseConfig> = [ | |||
|
|
|||
| class Reaptcha extends Component<Props, State> { | |||
| container?: HTMLDivElement | null; | |||
| timer?: number | undefined; | |||
There was a problem hiding this comment.
When the property is optional (with ?), it's automatically type | undefined, so the explicit | undefined is not needed. But it's just cosmetics.
|
Thank you for looking! Much appreciated. |
|
Still facing the same error on NextJS 12 |
|
@rafakwolf Would need some kind of reproduction to understand what's going on 👀 |
|
@rafakwolf Are the screenshots above from the reproduction repo? I just tested it and it works on my side 🤔 btw. Let's move the conversation here to not unnecessarily notify |
Fixes #254.
This PR moves the
timerproperty from state to a component instance field, which fixes StrictMode usage in React 18.React 18's StrictMode mounts, unmounts, and remounts components before actually rendering them in order to flush out any side effect cleanup issues. This presented as an error in Reaptcha because the component was being unmounted before
this.state.timerwas actually being set, which caused two copies of the interval to become active.It turns out that
timerdoesn't actually need to be stored in state since it doesn't affect rendering. In other words, moving it to an instance field is arguably more "correct" and it ensures compatibility with StrictMode.Note: in order to test this locally, I updated the react version in the examples to 18, which apparently affected the version used by the library's unit tests. I'd be happy to revert those changes and only leave the StrictMode fixes if you would prefer.