Added invariant for null/undefined create in useEffect, useLayoutEffect #15197
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
okie dokie, following up now...
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Details of bundled changes.Comparing: 5c2b2c0...6c0fb09 react
Generated by 🚫 dangerJS |
|
We try to avoid DEV-only invariants because they can lead to inconsistent behavior between dev and prod. Ideally we would throw right before it would throw anyway. In that case maybe it’s accepable. |
|
@gaearon I'm not super sure if there's a change you wanted, but I'm interpreting that to mean that this should remove the |
|
Could you make it a dev-only warning, instead of an invariant? The idea is to avoid an extra check in production, and to make sure the behavior is consistent across dev and prod. |
|
@acdlite sure - how would you like me to do that? I'm not familiar with the terminology here; does a dev-only warning mean using the |
This reverts commit 55664c6.
|
#15369 is somewhat related to more error checking, but for the return value. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Oh, I forgot about this! I'll tackle it soon. 🚀 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 236385b:
|
Details of bundled changes.Comparing: 64aae7b...236385b react
React: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 64aae7b...236385b react
React: size: 0.0%, gzip: 0.0% Size changes (stable) |
|
Mmhh, no, I don't have time to push this through. Best of wishes! |
## Summary Fixes #32354. Re-creation of #15197: adds a dev-only warning if `create == null` to the three `use*Effect` functions: * `useEffect` * `useInsertionEffect` * `useLayoutEffect` Updates the warning to match the same text given in the `react/exhaustive-deps` lint rule. ## How did you test this change? I applied the changes manually within `node_modules/` on a local clone of https://github.com/JoshuaKGoldberg/repros/tree/react-use-effect-no-arguments. Please pardon me for opening a PR addressing a not-accepted issue. I was excited to get back to #15194 -> #15197 now that I have time. 🙂 --------- Co-authored-by: lauren <[email protected]>
## Summary Fixes #32354. Re-creation of #15197: adds a dev-only warning if `create == null` to the three `use*Effect` functions: * `useEffect` * `useInsertionEffect` * `useLayoutEffect` Updates the warning to match the same text given in the `react/exhaustive-deps` lint rule. ## How did you test this change? I applied the changes manually within `node_modules/` on a local clone of https://github.com/JoshuaKGoldberg/repros/tree/react-use-effect-no-arguments. Please pardon me for opening a PR addressing a not-accepted issue. I was excited to get back to #15194 -> #15197 now that I have time. 🙂 --------- Co-authored-by: lauren <[email protected]> DiffTrain build for [192555b](192555b)
## Summary Fixes #32354. Re-creation of #15197: adds a dev-only warning if `create == null` to the three `use*Effect` functions: * `useEffect` * `useInsertionEffect` * `useLayoutEffect` Updates the warning to match the same text given in the `react/exhaustive-deps` lint rule. ## How did you test this change? I applied the changes manually within `node_modules/` on a local clone of https://github.com/JoshuaKGoldberg/repros/tree/react-use-effect-no-arguments. Please pardon me for opening a PR addressing a not-accepted issue. I was excited to get back to #15194 -> #15197 now that I have time. 🙂 --------- Co-authored-by: lauren <[email protected]> DiffTrain build for [192555b](192555b)
I considered putting this somewhere closer to where
createis used, but also wanted the complaint to be specific to the function & parameter name...Fixes #15194.