-
-
Notifications
You must be signed in to change notification settings - Fork 287
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?
Changes from 8 commits
f2c164d
4bb4c8d
37e7af9
cfeb236
fb49c07
1670de9
e7696c8
317c524
bfb7fc1
50d3569
122662c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| parseAsString | ||
| } from './parsers' | ||
| import { useQueryState } from './useQueryState' | ||
| import { useQueryStates } from './useQueryStates' | ||
|
|
||
| describe('useQueryState: referential equality', () => { | ||
| const defaults = { | ||
|
|
@@ -109,7 +110,7 @@ describe('useQueryState: referential equality', () => { | |
| expect(result.current.str[0]).toBe('foo') | ||
| rerender({ defaultValue: 'b' }) | ||
| const { str, obj, arr } = result.current | ||
| expect(str[0]).toBe('b') | ||
| expect(str[0]).toBe('foo') | ||
| expect(obj[0]).toBe(defaults.obj) | ||
| expect(arr[0]).toBe(defaults.arr) | ||
| expect(arr[0][0]).toBe(defaults.arr[0]) | ||
|
|
@@ -129,6 +130,40 @@ describe('useQueryState: referential equality', () => { | |
| }) | ||
| }) | ||
|
|
||
| describe('useQueryState: defaultValue', () => { | ||
| it('should read the same default value for multiple usages of the same parser', () => { | ||
| function TestComponent({ | ||
| id, | ||
| defaultValue | ||
| }: { | ||
| id: string | ||
| defaultValue: number | ||
| }) { | ||
| const [value] = useQueryState( | ||
| 'a', | ||
| parseAsInteger.withDefault(defaultValue) | ||
| ) | ||
|
|
||
| return ( | ||
| <div> | ||
| {id} value: {value} | ||
| </div> | ||
| ) | ||
| } | ||
| const result = render( | ||
| <div> | ||
| <TestComponent id="first" defaultValue={5} /> | ||
| <TestComponent id="second" defaultValue={23} /> | ||
|
Comment on lines
+155
to
+156
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: this test fails on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| </div>, | ||
| { | ||
| wrapper: withNuqsTestingAdapter() | ||
| } | ||
| ) | ||
| expect(result.getByText('first value: 5')).toBeInTheDocument() | ||
| expect(result.getByText('second value: 5')).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| describe('useQueryState: clearOnDefault', () => { | ||
| it('honors clearOnDefault: true by default', async () => { | ||
| const onUrlUpdate = vi.fn<OnUrlUpdateFunction>() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,7 +192,7 @@ describe('useQueryStates: referential equality', () => { | |
| expect(result.current[0].str).toBe('foo') | ||
| rerender({ defaultValue: 'b' }) | ||
| const [state] = result.current | ||
| expect(state.str).toBe('b') | ||
| expect(state.str).toBe('foo') | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
| expect(state.obj).toBe(defaults.obj) | ||
| expect(state.arr).toBe(defaults.arr) | ||
| expect(state.arr[0]).toBe(defaults.arr[0]) | ||
|
|
@@ -266,6 +266,39 @@ describe('useQueryStates: urlKeys remapping', () => { | |
| }) | ||
| }) | ||
|
|
||
| describe('useQueryStates: defaultValue', () => { | ||
| it('should read the same default value for multiple usages of the same parser', () => { | ||
| function TestComponent({ | ||
| id, | ||
| defaultValue | ||
| }: { | ||
| id: string | ||
| defaultValue: number | ||
| }) { | ||
| const [value] = useQueryStates({ | ||
| a: parseAsInteger.withDefault(defaultValue) | ||
| }) | ||
|
|
||
| return ( | ||
| <div> | ||
| {id} value: {value.a} | ||
| </div> | ||
| ) | ||
| } | ||
| const result = render( | ||
| <div> | ||
| <TestComponent id="first" defaultValue={5} /> | ||
| <TestComponent id="second" defaultValue={23} /> | ||
| </div>, | ||
| { | ||
| wrapper: withNuqsTestingAdapter() | ||
| } | ||
| ) | ||
| expect(result.getByText('first value: 5')).toBeInTheDocument() | ||
| expect(result.getByText('second value: 5')).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| describe('useQueryStates: clearOnDefault', () => { | ||
| it('honors clearOnDefault: true by default', async () => { | ||
| const onUrlUpdate = vi.fn<OnUrlUpdateFunction>() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| import { useCallback, useEffect, useId, useMemo, useRef, useState } from 'react' | ||
| import { | ||
| type DefaultValueStore, | ||
| useAdapter, | ||
| useAdapterDefaultOptions, | ||
| useAdapterProcessUrlSearchParams | ||
| useAdapterProcessUrlSearchParams, | ||
| useDefaultValueStore | ||
| } from './adapters/lib/context' | ||
| import type { Nullable, Options, UrlKeys } from './defs' | ||
| import { compareQuery } from './lib/compare' | ||
|
|
@@ -76,6 +78,7 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| const hookId = useId() | ||
| const defaultOptions = useAdapterDefaultOptions() | ||
| const processUrlSearchParams = useAdapterProcessUrlSearchParams() | ||
| const defaultValueStore = useDefaultValueStore() | ||
|
|
||
| const { | ||
| history = 'replace', | ||
|
|
@@ -100,17 +103,14 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| const adapter = useAdapter(Object.values(resolvedUrlKeys)) | ||
| const initialSearchParams = adapter.searchParams | ||
| const queryRef = useRef<Record<string, Query | null>>({}) | ||
| const defaultValues = useMemo( | ||
| () => | ||
| Object.fromEntries( | ||
| Object.keys(keyMap).map(key => [key, keyMap[key]!.defaultValue ?? null]) | ||
| ) as Values<KeyMap>, | ||
| [ | ||
| Object.values(keyMap) | ||
| .map(({ defaultValue }) => defaultValue) | ||
| .join(',') | ||
| ] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: this 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. |
||
| ) | ||
|
|
||
| // lazily initialize defaultValues in store | ||
| for (const [stateKey, { defaultValue }] of Object.entries(keyMap)) { | ||
| if (!(stateKey in defaultValueStore)) { | ||
| defaultValueStore[stateKey] = defaultValue ?? null | ||
| } | ||
| } | ||
|
|
||
| const queuedQueries = debounceController.useQueuedQueries( | ||
| Object.values(resolvedUrlKeys) | ||
| ) | ||
|
|
@@ -191,7 +191,6 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| useEffect(() => { | ||
| function updateInternalState(state: V) { | ||
| debug('[nuq+ %s `%s`] updateInternalState %O', hookId, stateKeys, state) | ||
| stateRef.current = state | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: this update was redundant because |
||
| setInternalState(state) | ||
| } | ||
| const handlers = Object.keys(keyMap).reduce( | ||
|
|
@@ -200,7 +199,7 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| state, | ||
| query | ||
| }: CrossHookSyncPayload) => { | ||
| const { defaultValue } = keyMap[stateKey]! | ||
| const defaultValue = defaultValueStore[stateKey] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| const urlKey = resolvedUrlKeys[stateKey]! | ||
| // Note: cannot mutate in-place, the object ref must change | ||
| // for the subsequent setState to pick it up. | ||
|
|
@@ -257,7 +256,7 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| const newState: Partial<Nullable<KeyMap>> = | ||
| typeof stateUpdater === 'function' | ||
| ? (stateUpdater( | ||
| applyDefaultValues(stateRef.current, defaultValues) | ||
| applyDefaultValues(stateRef.current, defaultValueStore) | ||
| ) ?? nullMap) | ||
| : (stateUpdater ?? nullMap) | ||
| debug('[nuq+ %s `%s`] setState: %O', hookId, stateKeys, newState) | ||
|
|
@@ -358,13 +357,17 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |
| adapter.getSearchParamsSnapshot, | ||
| adapter.rateLimitFactor, | ||
| processUrlSearchParams, | ||
| defaultValues | ||
| defaultValueStore | ||
| ] | ||
| ) | ||
|
|
||
| const outputState = useMemo( | ||
| () => applyDefaultValues(internalState, defaultValues), | ||
| [internalState, defaultValues] | ||
| () => | ||
| applyDefaultValues( | ||
| internalState, | ||
| defaultValueStore as Partial<Values<KeyMap>> | ||
| ), | ||
| [internalState, defaultValueStore] | ||
| ) | ||
| return [outputState, update] | ||
| } | ||
|
|
@@ -430,7 +433,7 @@ function parseMap<KeyMap extends UseQueryStatesKeysMap>( | |
|
|
||
| function applyDefaultValues<KeyMap extends UseQueryStatesKeysMap>( | ||
| state: NullableValues<KeyMap>, | ||
| defaults: Partial<Values<KeyMap>> | ||
| defaults: DefaultValueStore | ||
| ) { | ||
| return Object.fromEntries( | ||
| Object.keys(state).map(key => [key, state[key] ?? defaults[key] ?? null]) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.