-
Notifications
You must be signed in to change notification settings - Fork 48.7k
Restrict effect return type to a function or nothing #14119
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -320,42 +320,49 @@ function commitHookEffectList( | |
if ((effect.tag & unmountTag) !== NoHookEffect) { | ||
// Unmount | ||
const destroy = effect.destroy; | ||
effect.destroy = null; | ||
if (destroy !== null) { | ||
effect.destroy = undefined; | ||
if (destroy !== undefined) { | ||
destroy(); | ||
} | ||
} | ||
if ((effect.tag & mountTag) !== NoHookEffect) { | ||
// Mount | ||
const create = effect.create; | ||
let destroy = create(); | ||
if (typeof destroy !== 'function') { | ||
if (__DEV__) { | ||
if (destroy !== null && destroy !== undefined) { | ||
warningWithoutStack( | ||
false, | ||
'useEffect function must return a cleanup function or ' + | ||
'nothing.%s%s', | ||
typeof destroy.then === 'function' | ||
? '\n\nIt 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:\n\n' + | ||
'async function fetchComment(commentId) {\n' + | ||
' // You can await here\n' + | ||
'}\n\n' + | ||
'useEffect(() => {\n' + | ||
' fetchComment(commentId);\n' + | ||
'}, [commentId]);\n\n' + | ||
'In the future, React will provide a more idiomatic solution for data fetching ' + | ||
"that doesn't involve writing effects manually." | ||
: '', | ||
getStackByFiberInDevAndProd(finishedWork), | ||
); | ||
effect.destroy = create(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
if (__DEV__) { | ||
const destroy = effect.destroy; | ||
if (destroy !== undefined && typeof destroy !== 'function') { | ||
let addendum; | ||
if (destroy === null) { | ||
addendum = | ||
' You returned null. If your effect does not require clean ' + | ||
'up, return undefined (or nothing).'; | ||
} else if (typeof destroy.then === 'function') { | ||
addendum = | ||
'\n\nIt 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:\n\n' + | ||
'async function fetchComment(commentId) {\n' + | ||
' // You can await here\n' + | ||
'}\n\n' + | ||
'useEffect(() => {\n' + | ||
' fetchComment(commentId);\n' + | ||
'}, [commentId]);\n\n' + | ||
'In the future, React will provide a more idiomatic solution for data fetching ' + | ||
"that doesn't involve writing effects manually."; | ||
} else { | ||
addendum = ' You returned: ' + destroy; | ||
} | ||
warningWithoutStack( | ||
false, | ||
'An Effect function must not return anything besides a function, ' + | ||
'which is used for clean-up.%s%s', | ||
addendum, | ||
getStackByFiberInDevAndProd(finishedWork), | ||
); | ||
} | ||
destroy = null; | ||
} | ||
effect.destroy = destroy; | ||
} | ||
effect = effect.next; | ||
} while (effect !== firstEffect); | ||
|
@@ -696,7 +703,7 @@ function commitUnmount(current: Fiber): void { | |
let effect = firstEffect; | ||
do { | ||
const destroy = effect.destroy; | ||
if (destroy !== null) { | ||
if (destroy !== undefined) { | ||
safelyCallDestroy(current, destroy); | ||
} | ||
effect = effect.next; | ||
|
Uh oh!
There was an error while loading. Please reload this page.