simplify syncing props to state in ThemeProvider
#4855
Conversation
🦋 Changeset detectedLatest commit: 6c5af04 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| // Update state if props change | ||
| React.useEffect(() => { | ||
| setColorMode(props.colorMode ?? fallbackColorMode ?? defaultColorMode) | ||
| }, [props.colorMode, fallbackColorMode]) | ||
|
|
||
| React.useEffect(() => { | ||
| setDayScheme(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) | ||
| }, [props.dayScheme, fallbackDayScheme]) | ||
|
|
||
| React.useEffect(() => { | ||
| setNightScheme(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme) | ||
| }, [props.nightScheme, fallbackNightScheme]) | ||
|
|
There was a problem hiding this comment.
these are all synced automatically in the useSyncedState calls and avoid useEffect so we get to skip a render pass every time one changes
There was a problem hiding this comment.
also, I doubt these get hit often, but I saw this while looking at something related and just figured why not
mattcosta7
changed the title
ThemeProvider
| const nextInitialValue = initialValue instanceof Function ? initialValue() : initialValue | ||
| if (!isPropUpdateDisabled && !isEqual(previous, nextInitialValue)) { | ||
| setPrevious(nextInitialValue) | ||
| setState(nextInitialValue) | ||
| } |
There was a problem hiding this comment.
these setState calls in render are all good because they're done in the same component. This will immediately queue an update and re-render instead of completing a render, then comparing in a useEffect, queuing an update and then rendering again then re-comparing.
the main value is we save a full render from react on any change - a render we know will immediately be discarded
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/337959 |
Closes #
Changelog
New
Added a
useSyncedStatehook that avoids using useEffect to sync state when props change inline.I think it's likely this could be applied to other components in primer to save a render occasionally, but can easily be used elsewhere once it's in the codebase
Changed
In ThemeProvider I used the
useSyncedStatehook which was added in this PR.This hook tracks the values used to initialize the state and syncs in the initial render pass instead of in a useEffect, which should avoid a handful of theme provider renders in some cases. it also avoids the need to use a useEffect to sync the state when the props change inline - making the code a bit clearer. These are no longer synced after render but mid-render leading to a few render passes being skipped
Removed
Rollout strategy
Testing & Reviewing
Merge checklist