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

Restrict effect return type to a function or nothing #14119

Merged
merged 4 commits into from Jan 31, 2019

Conversation

@acdlite
Copy link
Member

@acdlite acdlite commented Nov 6, 2018

We already warn in dev if the wrong type is returned. This updates the Flow type.

Copy link
Member

@sebmarkbage sebmarkbage left a comment

Why null? It shouldn’t be allowed. Either you do return or you don’t.

Also the return value from the destroy function itself should always be void.

Loading

@acdlite
Copy link
Member Author

@acdlite acdlite commented Nov 6, 2018

@sebmarkbage We allow null in place of undefined everywhere else. Seems harmless to allow it here, too.

useEffect(() => {
  // An if statement is arguably clearer but this seems fine, too?
  return source !== null ? source.subscribe() : null;
}, [source]);

Loading

@acdlite
Copy link
Member Author

@acdlite acdlite commented Nov 6, 2018

Also the return value from the destroy function itself should always be void.

That's can be annoying if you use an arrow function. For example, sometimes "destroy" -type functions return a boolean:

useEffect(() => {
  mySet.add(thing);
  return () => mySet.delete(thing);
});

Loading

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Nov 7, 2018

null is not allowed instead of undefined in default props, default args etc. These things have a tendency to spread once they’ve leaked so better to narrow it early.

The argument for arrows in destroy also applies to useEffect(() => foo());

Might be better to just enforce a style that makes it clear that these are side-effectful void functions?

Loading

Copy link
Member

@sebmarkbage sebmarkbage left a comment

Should be limited to only undefined return values both in the callback and the destroy function.

Loading

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Jan 16, 2019

null is unfortunately allowed in PropTypes (there's no way to accept null without also accepting undefined, and vice-versa), but that's probably one of the reasons why it's no longer recommended.

We ran into problems with that when we tried to check compatibility between propTypes and the actual declared type of props in typescript. It's best to not let things like this spread further.

Loading

@acdlite acdlite force-pushed the restrict-effect-return-type branch from c9074ed to a5d9d85 Jan 17, 2019
@acdlite acdlite force-pushed the restrict-effect-return-type branch from a5d9d85 to 2cb5b8a Jan 30, 2019
getStackByFiberInDevAndProd(finishedWork),
);
}
effect.destroy = create();
Copy link
Member Author

@acdlite acdlite Jan 30, 2019

Choose a reason for hiding this comment

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

@sebmarkbage Usually we coerce missing values to null before storing them in our internal data structures, but I think that's because we usually accept either, and null is preferred because it's less likely to be unintentional. But in this case, since we don't accept null, I can skip the type check in prod. Let me know if this doesn't make sense.

Loading

Copy link
Member

@sebmarkbage sebmarkbage Jan 31, 2019

Choose a reason for hiding this comment

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

I believe there has been times where V8 has treated undefined as effectively a missing property in the hidden class rather than a reified value. So setting to undefined might mess with the hidden class. Not sure though.

Loading

Copy link
Member Author

@acdlite acdlite Jan 31, 2019

Choose a reason for hiding this comment

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

Ok I'll leave it like this until we learn more, I suppose

Loading

@acdlite acdlite requested review from sebmarkbage and removed request for sophiebits Jan 30, 2019
warningWithoutStack(
false,
'useEffect function must return a cleanup function or ' +
'nothing (undefined).%s%s',
Copy link
Member

@sebmarkbage sebmarkbage Jan 31, 2019

Choose a reason for hiding this comment

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

Are there other common examples we can include here? So it doesn't sounds like a jargon/technical error message.

Loading

Copy link
Member Author

@acdlite acdlite Jan 31, 2019

Choose a reason for hiding this comment

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

I'll add a branch specifically for null

Loading

@sizebot
Copy link

@sizebot sizebot commented Jan 31, 2019

ReactDOM: size: -0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 70d4075...bec016d

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 749.42 KB 749.73 KB 171.01 KB 171.09 KB UMD_DEV
react-dom.production.min.js -0.0% 0.0% 104.6 KB 104.57 KB 34.01 KB 34.01 KB UMD_PROD
react-dom.profiling.min.js -0.0% -0.0% 107.59 KB 107.56 KB 34.66 KB 34.66 KB UMD_PROFILING
react-dom.development.js 0.0% 0.0% 744.48 KB 744.79 KB 169.59 KB 169.67 KB NODE_DEV
react-dom.production.min.js -0.0% -0.0% 104.65 KB 104.62 KB 33.45 KB 33.44 KB NODE_PROD
react-dom.profiling.min.js -0.0% -0.0% 107.76 KB 107.73 KB 34.1 KB 34.09 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 766.97 KB 767.25 KB 170.82 KB 170.9 KB FB_WWW_DEV
ReactDOM-prod.js -0.0% -0.0% 313.33 KB 313.24 KB 57.55 KB 57.53 KB FB_WWW_PROD
ReactDOM-profiling.js -0.0% -0.0% 320.47 KB 320.38 KB 58.88 KB 58.86 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js 0.0% 0.0% 749.76 KB 750.08 KB 171.15 KB 171.23 KB UMD_DEV
react-dom-unstable-fire.production.min.js -0.0% 0.0% 104.62 KB 104.59 KB 34.02 KB 34.02 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js -0.0% -0.0% 107.61 KB 107.58 KB 34.67 KB 34.67 KB UMD_PROFILING
react-dom-unstable-fire.development.js 0.0% 0.0% 744.82 KB 745.13 KB 169.74 KB 169.82 KB NODE_DEV
react-dom-unstable-fire.production.min.js -0.0% -0.0% 104.67 KB 104.64 KB 33.46 KB 33.45 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js -0.0% -0.0% 107.78 KB 107.75 KB 34.11 KB 34.1 KB NODE_PROFILING
ReactFire-dev.js 0.0% 0.0% 766.18 KB 766.46 KB 170.74 KB 170.82 KB FB_WWW_DEV
ReactFire-prod.js -0.0% -0.0% 301.92 KB 301.83 KB 55.2 KB 55.19 KB FB_WWW_PROD
ReactFire-profiling.js -0.0% -0.0% 308.99 KB 308.9 KB 56.57 KB 56.55 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% -0.0% 44.87 KB 44.87 KB 12.3 KB 12.3 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 9.97 KB 9.97 KB 3.71 KB 3.71 KB UMD_PROD
react-dom-test-utils.development.js 0.0% -0.0% 44.59 KB 44.59 KB 12.24 KB 12.24 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 9.74 KB 9.74 KB 3.65 KB 3.65 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 11.01 KB 11.01 KB 3.81 KB 3.81 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.29 KB 60.29 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.75 KB 10.75 KB 3.71 KB 3.7 KB NODE_PROD
react-dom-server.browser.development.js 0.0% 0.0% 125.57 KB 125.57 KB 33.5 KB 33.5 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 18.57 KB 18.57 KB 7.17 KB 7.17 KB UMD_PROD
react-dom-server.browser.development.js 0.0% -0.0% 121.7 KB 121.7 KB 32.57 KB 32.57 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 18.49 KB 18.49 KB 7.15 KB 7.15 KB NODE_PROD
ReactDOMServer-dev.js 0.0% -0.0% 122.67 KB 122.67 KB 32.08 KB 32.08 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 44.65 KB 44.65 KB 10.34 KB 10.34 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% -0.0% 123.76 KB 123.76 KB 33.12 KB 33.12 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 19.36 KB 19.36 KB 7.45 KB 7.45 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.63 KB 3.63 KB 1.44 KB 1.44 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.3% 1.21 KB 1.21 KB 706 B 704 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.45 KB 3.45 KB 1.39 KB 1.39 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.5% 1.05 KB 1.05 KB 638 B 635 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.3% 1.1 KB 1.1 KB 668 B 666 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 528.26 KB 528.58 KB 114.62 KB 114.7 KB UMD_DEV
react-art.production.min.js -0.0% 0.0% 96.66 KB 96.63 KB 29.8 KB 29.8 KB UMD_PROD
react-art.development.js +0.1% +0.1% 459.79 KB 460.1 KB 97.52 KB 97.61 KB NODE_DEV
react-art.production.min.js -0.1% -0.0% 61.67 KB 61.63 KB 18.94 KB 18.93 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 468.69 KB 468.97 KB 96.85 KB 96.94 KB FB_WWW_DEV
ReactART-prod.js -0.0% -0.1% 189.23 KB 189.14 KB 32.27 KB 32.25 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 473 KB 473.31 KB 100.44 KB 100.52 KB UMD_DEV
react-test-renderer.production.min.js -0.1% -0.0% 63.07 KB 63.04 KB 19.45 KB 19.44 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 467.96 KB 468.27 KB 99.21 KB 99.29 KB NODE_DEV
react-test-renderer.production.min.js -0.1% -0.0% 62.74 KB 62.7 KB 19.18 KB 19.17 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 477.44 KB 477.71 KB 98.85 KB 98.93 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 36.8 KB 36.8 KB 9.36 KB 9.36 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.1% 11.03 KB 11.03 KB 3.33 KB 3.33 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 31.1 KB 31.1 KB 7.98 KB 7.98 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.1% 11.68 KB 11.68 KB 3.63 KB 3.63 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 457.62 KB 457.93 KB 96.01 KB 96.09 KB NODE_DEV
react-reconciler.production.min.js -0.1% -0.0% 62.84 KB 62.81 KB 18.78 KB 18.78 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.1% 456 KB 456.31 KB 95.37 KB 95.45 KB NODE_DEV
react-reconciler-persistent.production.min.js -0.1% -0.0% 62.85 KB 62.82 KB 18.79 KB 18.78 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% -0.0% 15.4 KB 15.4 KB 4.84 KB 4.83 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.2% 2.56 KB 2.56 KB 1.13 KB 1.13 KB NODE_PROD

Generated by 🚫 dangerJS

Loading

@acdlite
Copy link
Member Author

@acdlite acdlite commented Jan 31, 2019

Ok the warning message now has three branches, based on the type of the return value.

If you return null

An Effect function must not return anything besides a function, which is used for clean-up. You returned null. If your effect does not require clean up, return undefined (or nothing).

If you return a promise

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, you may write an async function separately and then call it from inside
the effect:

async function fetchComment(commentId) {
  // You can await here
}

useEffect(() => {
  fetchComment(commentId);
}, [commentId]);

In the future, React will provide a more idiomatic solution for data fetching that doesn't involve writing effects manually.

If you return anything else that's neither a function nor undefined

An Effect function must not return anything besides a function, which is used for clean-up. You returned: [value coerced to string]


I'll merge and we can bikeshed the messages more if they still need work.

Loading

@acdlite acdlite merged commit 66eb293 into facebook:master Jan 31, 2019
1 check passed
Loading
acdlite added a commit that referenced this issue Feb 5, 2019
pull bot pushed a commit to SimenB/react that referenced this issue Feb 6, 2019
* Add 16.8.0 changelog

* Mention ESLint plugin

* Remove experimental notices from the ESLint plugin README

* Update CHANGELOG.md

* Add more details for Hooks

* fix

* Set a date

* Update CHANGELOG.md

Co-Authored-By: gaearon <dan.abramov@gmail.com>

* Update CHANGELOG.md

* useReducer in changelog

* Add to changelog

* Update date

* Add facebook#14119 to changelog

* Add facebook#14744 to changelog

* Fix PR links

* act() method was added to test utils, too

* Updated release date to February 6th
pull bot pushed a commit to SimenB/react that referenced this issue Feb 6, 2019
* Add 16.8.0 changelog

* Mention ESLint plugin

* Remove experimental notices from the ESLint plugin README

* Update CHANGELOG.md

* Add more details for Hooks

* fix

* Set a date

* Update CHANGELOG.md

Co-Authored-By: gaearon <dan.abramov@gmail.com>

* Update CHANGELOG.md

* useReducer in changelog

* Add to changelog

* Update date

* Add facebook#14119 to changelog

* Add facebook#14744 to changelog

* Fix PR links

* act() method was added to test utils, too

* Updated release date to February 6th
pull bot pushed a commit to chojar/react that referenced this issue Feb 6, 2019
* Add 16.8.0 changelog

* Mention ESLint plugin

* Remove experimental notices from the ESLint plugin README

* Update CHANGELOG.md

* Add more details for Hooks

* fix

* Set a date

* Update CHANGELOG.md

Co-Authored-By: gaearon <dan.abramov@gmail.com>

* Update CHANGELOG.md

* useReducer in changelog

* Add to changelog

* Update date

* Add facebook#14119 to changelog

* Add facebook#14744 to changelog

* Fix PR links

* act() method was added to test utils, too

* Updated release date to February 6th
facebook-github-bot added a commit to facebook/flow that referenced this issue Feb 6, 2019
…eturns nothing

Summary:
to match the changes in facebook/react#14119.

ran `make` and the tests, seemed ok?
Pull Request resolved: #7430

Reviewed By: bvaughn

Differential Revision: D13927998

Pulled By: jbrown215

fbshipit-source-id: 1bc0a6673d5f1bc9a58baa7ec30fdd6aace34813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants