Skip to content

[v5]: follow React "standard" way with breaking behavioral change #2395

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 9, 2024
Merged

[v5]: follow React "standard" way with breaking behavioral change #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d6776f
[v5]: follow React "standard" way with breaking behavioral change
dai-shi Mar 8, 2024
cbc0fdd
add test
dai-shi Mar 8, 2024
92213cd
Merge branch 'v5' into v5-no-hacks-with-behavioral-change
dai-shi Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 3 additions & 19 deletions src/react.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,13 @@ import type {
StoreMutatorIdentifier,
} from './vanilla.ts'

const { useDebugValue, useMemo, useSyncExternalStore } = ReactExports
const { useDebugValue, useSyncExternalStore } = ReactExports

type ExtractState<S> = S extends { getState: () => infer T } ? T : never

type ReadonlyStoreApi<T> = Pick<StoreApi<T>, 'getState' | 'subscribe'>

const identity = <T>(arg: T): T => arg

const useMemoSelector = <TState, StateSlice>(
getState: () => TState,
selector: (state: TState) => StateSlice,
) =>
useMemo(() => {
let prev: readonly [TState, StateSlice] | undefined
return () => {
const state = getState()
if (!prev || !Object.is(prev[0], state)) {
prev = [state, selector(state)]
}
return prev[1]
}
}, [getState, selector])

export function useStore<S extends StoreApi<unknown>>(api: S): ExtractState<S>

export function useStore<S extends StoreApi<unknown>, U>(
Expand All @@ -48,8 +32,8 @@ export function useStore<TState, StateSlice>(
) {
const slice = useSyncExternalStore(
api.subscribe,
useMemoSelector(api.getState, selector),
useMemoSelector(api.getInitialState, selector),
() => selector(api.getState()),
() => selector(api.getInitialState()),
)
useDebugValue(slice)
return slice
Expand Down
42 changes: 40 additions & 2 deletions tests/basic.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,47 @@ it('can set the store without merging', () => {
expect(getState()).toEqual({ b: 2 })
})

it('only calls selectors when necessary', async () => {
it('only calls selectors when necessary with static selector', async () => {
type State = { a: number; b: number }
const useBoundStore = create<State>(() => ({ a: 0, b: 0 }))
const useBoundStore = createWithEqualityFn<State>(() => ({ a: 0, b: 0 }))
const { setState } = useBoundStore
let staticSelectorCallCount = 0

function staticSelector(s: State) {
staticSelectorCallCount++
return s.a
}

function Component() {
useBoundStore(staticSelector)
return (
<>
<div>static: {staticSelectorCallCount}</div>
</>
)
}

const { rerender, findByText } = render(
<>
<Component />
</>,
)
await findByText('static: 1')

rerender(
<>
<Component />
</>,
)
await findByText('static: 1')

act(() => setState({ a: 1, b: 1 }))
await findByText('static: 2')
})

it('only calls selectors when necessary (traditional)', async () => {
type State = { a: number; b: number }
const useBoundStore = createWithEqualityFn<State>(() => ({ a: 0, b: 0 }))
Copy link
Member Author

@dai-shi dai-shi Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior (inline selector optimization) isn't supported in v5. Use zustand/traditional.

const { setState } = useBoundStore
let inlineSelectorCallCount = 0
let staticSelectorCallCount = 0
Expand Down
8 changes: 3 additions & 5 deletions tests/ssr.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@ const useBearStore = create<BearStoreState & BearStoreAction>((set) => ({
}))

function Counter() {
const { bears, increasePopulation } = useBearStore(
({ bears, increasePopulation }) => ({
bears,
increasePopulation,
}),
Comment on lines -22 to -26
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it worked before by chance. This selector creates a new object every time, which isn't supported in v5. We should use a memoized selector, zutand/traditional or 👇 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, was this test intentional to check the behavior with such selectors? @TkDodo

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think I copied this way if creating a store from somewhere. Definitely not intentional

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming. That was the reason I introduced useMemoSelector, without looking at the test code. I believe it is an unnecessary hack.

const bears = useBearStore(({ bears }) => bears)
const increasePopulation = useBearStore(
({ increasePopulation }) => increasePopulation,
)

useEffect(() => {
Expand Down