Skip to content
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

update error message to include useLayoutEffect or useEffect on bad e… #22279

Merged
merged 3 commits into from
Sep 10, 2021
Merged

update error message to include useLayoutEffect or useEffect on bad e… #22279

merged 3 commits into from
Sep 10, 2021

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Sep 9, 2021

Summary

See conversation here: reactwg/react-18#95 (comment)

In react, when an effect returns something unexpected like a promise we show an error like:

An effect function must not return anything besides a function, which is used for clean-up. It looks like you wrote useEffect(async () => ...) or returned a Promise. Instead, write the async function inside your effect and call it immediately

The problem is that this error message says "useEffect" even when using "useLayoutEffect". To fix, we can check the effect tag to see if it's a layout or passive effect.

See: https://codesandbox.io/s/effect-destroy-function-type-forked-dwpjm?file=/src/App.js

How did you test this change?

Updating existing test cases to expect the updated error message

@sizebot
Copy link

sizebot commented Sep 9, 2021

Comparing: 8f96c6b...5e010af

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 128.27 kB 128.27 kB = 40.93 kB 40.93 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 131.09 kB 131.09 kB = 41.86 kB 41.86 kB
facebook-www/ReactDOM-prod.classic.js = 407.09 kB 407.09 kB = 75.40 kB 75.40 kB
facebook-www/ReactDOM-prod.modern.js = 395.65 kB 395.65 kB = 73.69 kB 73.69 kB
facebook-www/ReactDOMForked-prod.classic.js = 407.09 kB 407.09 kB = 75.40 kB 75.40 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 5e010af

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@salazarm salazarm merged commit 0fd195f into facebook:main Sep 10, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 22, 2021
Summary:
This sync includes the following changes:
- **[f4ac680c7](facebook/react@f4ac680c7 )**: Fixed broken build script --unsafe-partial flag ([#22324](facebook/react#22324)) //<Brian Vaughn>//
- **[67222f044](facebook/react@67222f044 )**: [Experiment] Warn if callback ref returns a function ([#22313](facebook/react#22313)) //<Dan Abramov>//
- **[263cfa6ec](facebook/react@263cfa6ec )**: [Experimental] Add useInsertionEffect ([#21913](facebook/react#21913)) //<Ricky>//
- **[806aaa2e2](facebook/react@806aaa2e2 )**: [useSES shim] Import prefixed native API ([#22310](facebook/react#22310)) //<Andrew Clark>//
- **[fd5e01c2e](facebook/react@fd5e01c2e )**: [useSES/extra] Reuse old selection if possible ([#22307](facebook/react#22307)) //<Andrew Clark>//
- **[33226fada](facebook/react@33226fada )**: Check for store mutations before commit ([#22290](facebook/react#22290)) //<Andrew Clark>//
- **[86c7ca70a](facebook/react@86c7ca70a )**: Fix link ([#22296](facebook/react#22296)) //<Konstantin Popov>//
- **[0fd195f29](facebook/react@0fd195f29 )**: update error message to include useLayoutEffect or useEffect on bad e… ([#22279](facebook/react#22279)) //<salazarm>//
- **[8f96c6b2a](facebook/react@8f96c6b2a )**: [Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect ([#22277](facebook/react#22277)) //<Andrew Clark>//
- **[4ce89a58d](facebook/react@4ce89a58d )**: Test bad useEffect return value with noop-renderer ([#22258](facebook/react#22258)) //<Sebastian Silbermann>//
- **[a3fde2358](facebook/react@a3fde2358 )**: Detect subscriptions wrapped in startTransition ([#22271](facebook/react#22271)) //<salazarm>//

Changelog:
[General][Changed] - React Native sync for revisions 95d762e...e8feb11

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D30966369

fbshipit-source-id: 6c88e591005deb1fd93493628ef4695add49186c
@salazarm salazarm deleted the showUseLayoutEffectInWarning branch September 27, 2021 22:56
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
facebook#22279)

* update error message to include useLayoutEffect or useEffect on bad effect return

* Update packages/react-reconciler/src/ReactFiberCommitWork.new.js

Co-authored-by: Ricky <rickhanlonii@gmail.com>

* use existing import

Co-authored-by: Ricky <rickhanlonii@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants