fix: stop rebuilding the web measurement probe on every render - #748
Open
giaBaoJS wants to merge 1 commit into
Open
fix: stop rebuilding the web measurement probe on every render#748giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
The measurement effect in NativeSafeAreaProvider.web listed onInsetsChange in its dependency array. SafeAreaListener passes a fresh inline arrow on every render, so every render of a SafeAreaListener tore down and rebuilt the hidden probe element, its transitionend and resize listeners and the ResizeObserver, and re-measured. The effect never needs the callback's identity, only the latest callback, so read it through a ref and drop it from the dependencies. Setup now runs once per mount while inset and frame changes still propagate to the most recent callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is no issue filed for this — I found it while reading
NativeSafeAreaProvider.web.tsx, so I'm leading with the reproduction rather than a report.What happens
The measurement effect in
src/NativeSafeAreaProvider.web.tsxhad[onInsetsChange]in its dependency array.SafeAreaListener(src/SafeAreaContext.tsx:122) builds itsonInsetsChangeas an inline arrow inside its own render:That arrow is a new function on every render of
SafeAreaListener, so the effect's cleanup and setup ran on every render of the listener. Each cycle:<div>fromdocument.bodyand appends a freshly created one,transitionendlistener on the probe and theresizelistener onwindow,ResizeObserverand constructs and re-observes a new one,window.getComputedStyle(element)andproviderElement.getBoundingClientRect()and firesonInsetsChangeagain.Note this is driven by render count, not by anything actually changing. It fires even when
onChangeis a stable function, because the churn comes fromSafeAreaListener's own inline arrow, not the caller's prop.SafeAreaProvideris not affected — it already memoises its callback withReact.useCallback(..., [])atsrc/SafeAreaContext.tsx:56. The path here isSafeAreaListeneron web.Reproduction
I added three tests to the existing
src/__tests__/NativeSafeAreaProvider.web-test.tsxjsdom suite. The user-visible one renders a realSafeAreaListenerwith a stableonChangeand re-renders it three times, then counts probe attachments,ResizeObserverconstructions and measurement calls.On
main, after 3 renders:document.bodyResizeObserverinstances constructedonChangecallsI have measured the setup/teardown counts above and nothing else — I am not claiming a frame-time or benchmark number.
The fix
Store the latest
onInsetsChangein a ref and drop it from the effect's dependencies, so setup runs once per mount. 10 lines of source change.I considered fixing it in
SafeAreaListenerinstead by wrapping its callback inuseCallback([onChange]), and rejected it: that only helps callers who already memoiseonChange, and an inlineonChange— the common usage — would still churn. Fixing it in the web provider makes the effect correct for every caller. The effect body genuinely does not depend on the callback's identity, only on being able to call the current one, so the ref is the right tool here; there are no other values in the closure that would go stale (viewRefis a ref andcreateContextElementis module-level).For contrast, the native
NativeSafeAreaProviderpassesonInsetsChangestraight through to the native view, where a changing identity is just a prop update with no teardown — the web implementation is the only one holding resources across renders, so this stays scoped to the web file.Not regressing what the effect is for
Dropping a dependency risks a stale callback, so one of the tests guards exactly that: it mounts with callback A, re-renders with callback B (asserting B is not called just for being swapped in), then triggers a genuine resize and asserts B receives the new insets and frame while A is not called again. I verified this test fails if I take the dependency-array change without the ref, so the ref is load-bearing and not decoration. The existing tests covering window resize,
ResizeObserverupdates and theResizeObserver-less fallback all still pass.yarn testpasses (prettier, eslint, tsc, jest — 24 tests, 11 snapshots). The 3 eslintno-deep-importswarnings are pre-existing onmain.Possible overlap
If an
unstable_disableViewOnWebprop forSafeAreaProvider(#637) lands around the same time it may touch this file. I have kept this change as narrow as I could — it only touches the ref plumbing and the dependency array, not the measurement logic or the rendered tree — so it should rebase cleanly either way.