fix(pulsing-dots): keep the hook order stable when dotCount changes - #48
Open
dennytosp wants to merge 1 commit into
Open
fix(pulsing-dots): keep the hook order stable when dotCount changes#48dennytosp wants to merge 1 commit into
dennytosp wants to merge 1 commit into
Conversation
`PulsingDots` called `useSharedValue` and `useAnimatedProps` once per dot from inside `Array.from`/`map` callbacks, so the number of hooks the component ran was driven by the `dotCount` prop. Changing `dotCount` at runtime changed the hook order and React tore the component down — it rendered nothing from that point on. Move the per-dot animation into a `Dot` child component so each instance runs a fixed number of hooks and React can mount and unmount dots normally. The staggered start now uses `withDelay` instead of an uncancelled `setTimeout`, which also stops the animation from being scheduled after unmount, and the effect tracks `duration` so the prop is no longer ignored after the first render.
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.
Found while reading through the component library — not tied to an existing issue.
Problem
PulsingDotsbreaks permanently the momentdotCountchanges at runtime. React logs "React has detected a change in the order of Hooks called by PulsingDots" and the component renders nothing from that point on.Cause
Both animation hooks were called from inside loop callbacks, so the number of hooks the component ran was driven by a prop:
Two smaller problems sat alongside it:
setTimeoutwith no cleanup, so an unmounted component could still be scheduled;[], so changingdurationafter the first render had no effect.Fix
Move the per-dot animation into a
Dotchild component. Each instance runs a fixed number of hooks, so React mounts and unmounts dots normally asdotCountchanges.The stagger now uses Reanimated's
withDelayinstead ofsetTimeout— no uncancelled timer, and the animation is torn down with the component — and the effect tracksduration.No public API change: the same props render the same output.
Verification
Rendered the real component before and after (native modules stubbed so the stubs map onto real React hooks), mounting at
dotCount=3and then changing it to5and2:tsc --noEmitreports the same error count asmain(none in this file).website/react-native/pulsing-dots.tsxis regenerated viabun run sync:changed.While looking at this,
src/components/base/curved-bottom-tabs/index.tsxhas the same class of problem —useSharedValueinsideuseRef(tabs.map(...))(L224) anduseAnimatedStyleinsidetabs.map(...)(L311). It needs a larger refactor, so I have left it out of this PR; happy to open a separate one if you'd like.