-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@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
timer
property 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.timer
was actually being set, which caused two copies of the interval to become active.It turns out that
timer
doesn'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.