Restrict effect return type to a function or nothing#14119
Restrict effect return type to a function or nothing#14119acdlite merged 4 commits intofacebook:masterfrom
Conversation
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this 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.
|
@sebmarkbage We allow useEffect(() => {
// An if statement is arguably clearer but this seems fine, too?
return source !== null ? source.subscribe() : null;
}, [source]); |
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);
}); |
|
The argument for arrows in destroy also applies to Might be better to just enforce a style that makes it clear that these are side-effectful void functions? |
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
Should be limited to only undefined return values both in the callback and the destroy function.
|
We ran into problems with that when we tried to check compatibility between |
acdlite
force-pushed
the
restrict-effect-return-type
branch
from
c9074ed to
a5d9d85
Compare
We already warn in dev if the wrong type is returned. This updates the Flow type.
acdlite
force-pushed
the
restrict-effect-return-type
branch
from
a5d9d85 to
2cb5b8a
Compare
| getStackByFiberInDevAndProd(finishedWork), | ||
| ); | ||
| } | ||
| effect.destroy = create(); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok I'll leave it like this until we learn more, I suppose
| warningWithoutStack( | ||
| false, | ||
| 'useEffect function must return a cleanup function or ' + | ||
| 'nothing (undefined).%s%s', |
There was a problem hiding this comment.
Are there other common examples we can include here? So it doesn't sounds like a jargon/technical error message.
There was a problem hiding this comment.
I'll add a branch specifically for null
|
ReactDOM: size: -0.0%, gzip: 0.0% Details of bundled changes.Comparing: 70d4075...bec016d react-dom
react-art
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
|
Ok the warning message now has three branches, based on the type of the return value. If you return null
If you return a promise
If you return anything else that's neither a function nor undefined
I'll merge and we can bikeshed the messages more if they still need work. |
* 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
* 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
* 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
…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
6 participants
We already warn in dev if the wrong type is returned. This updates the Flow type.