-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: support ref cleanup functions #4436
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10k
duration
usedJSHeapSize
filter-list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many-updates
duration
usedJSHeapSize
replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
update10th1k
duration
usedJSHeapSize
|
Size Change: +145 B (+0.23%) Total Size: 61.9 kB
ℹ️ View Unchanged
|
ref._unmount(); | ||
} | ||
|
||
if (!hasRefUnmount || value != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need !hasRefUnmount
here? Wouldn't it be safe to always invoke this and inline typeof ref._unmount == 'function'
above?
EDIT: seems needed when I tested it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe inverting the check makes it more clear. When a cleanup function is returned, then the ref
function should never be invoked with null
. But when no cleanup function is there it should be invoked with null
.
if (typeof ref == 'function') ref(value); | ||
else ref.current = value; | ||
if (typeof ref == 'function') { | ||
let hasRefUnmount = typeof ref._unmount == 'function'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to add _unmount
to mangle to be safe I reckon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already there as we use that for options._unmount
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Fixes #4435