-
-
Notifications
You must be signed in to change notification settings - Fork 218
feat: global default value store #1148
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
base: next
Are you sure you want to change the base?
Conversation
|
@TkDodo is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it. |
commit: |
| <TestComponent id="first" defaultValue={5} /> | ||
| <TestComponent id="second" defaultValue={23} /> |
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.
note: this test fails on main because it renders first with 5 and second with 23. Now, it renders both with 5, which I think (hope) is what we want.
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.
I think this relates to the repro for issue #760 (dynamic defaults): a change in the default value should be reflected in the output state.
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.
I don’t think supporting dynamic default value is an intended feature - it feels more like a bug to me as it would result in different values being rendered on screen for the same key.
It also won’t work with the discussed writeDefaults feature.
| rerender({ defaultValue: 'b' }) | ||
| const [state] = result.current | ||
| expect(state.str).toBe('b') | ||
| expect(state.str).toBe('foo') |
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.
note: this change in test actually shows the (imo buggy) current behaviour: If a component re-renders with a different defaultValue, why would the state change? Not even uncontrolled inputs in react with a defaultValue work that way...
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.
The state internally can be nullable, and the default is nullish-coalesced on top of that, so that made the output respect dynamic defaults (issue #760).
| [ | ||
| Object.values(keyMap) | ||
| .map(({ defaultValue }) => defaultValue) | ||
| .join(',') | ||
| ] |
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.
note: this useMemo dependency array being gone is a good thing because stringifying doesn’t work well with complex objects and it wouldn’t work at all if we wanted to support default values as a function.
now, it’s no longer needed because the global store is “stable” and we actually only read the defaultValue imperatively, so it doesn’t need to be reactive.
| useEffect(() => { | ||
| function updateInternalState(state: V) { | ||
| debug('[nuq+ %s `%s`] updateInternalState %O', hookId, stateKeys, state) | ||
| stateRef.current = state |
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.
note: this update was redundant because updateInternalState is only called with stateRef.current, so at this point, that value is already correctly set.
| query | ||
| }: CrossHookSyncPayload) => { | ||
| const { defaultValue } = keyMap[stateKey]! | ||
| const defaultValue = defaultValueStore[stateKey] |
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.
note: it might be a bit brittle here to only read from the defaultValueStore. We basically expect it to be initialized already at this point, which is true, but if we ever remove values from the store we would need to re-initialize here too.
This PR takes the
defaultValueimplementation, which used to be local to a component instance, and hoists it to a “global store” - a simplerefthat we pass down through context.This store gets lazily initialized - when the defaultValue is first required. This ensures that multiple components that render in different places with different defaultValues still show the same data, as the second
defaultValuesimply gets discarded.This PR is a prerequisite for allowing “default-value-as-a-function”, because functions can read from different places (like localstorage), which can change over time, so for consistency, we’d only want the first read.
This makes
defaultValuein-line with howuseStateinitializers work (though on a “global” level), and that’s also exactly howintitialDataworks in TanStack Query.