Skip to content

Commit

Permalink
fix(react): state selector may use old closures
Browse files Browse the repository at this point in the history
  • Loading branch information
forehalo committed Sep 18, 2021
1 parent 3ff8a62 commit 69b19ce
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 35 deletions.
11 changes: 2 additions & 9 deletions packages/core/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export class Store<S> implements IStore<S> {
private readonly reducer: Reducer<S, Action>
private readonly epic$: BehaviorSubject<Epic>
private actionSub = new Subscription()
private readonly stateSub = new Subscription()
private readonly initAction: Action<null> = {
type: INIT_ACTION_TYPE_SYMBOL,
payload: null,
Expand Down Expand Up @@ -47,14 +46,8 @@ export class Store<S> implements IStore<S> {
*/
setup(defaultState: S) {
this.internalState = defaultState

this.state$.next(defaultState)
this.subscribeAction()
this.stateSub.add(
this.state$.subscribe((state) => {
this.internalState = state
}),
)
this.state$.next(this.state)
this.log(this.initAction)
this.isReady = true
}
Expand Down Expand Up @@ -101,6 +94,7 @@ export class Store<S> implements IStore<S> {
if (process.env.NODE_ENV !== 'production' && newState === undefined) {
console.warn(`${action.type} produced an undefined state, you may forget to return new State in @Reducer`)
}
this.internalState = newState
this.state$.next(newState)
}
this.log(action)
Expand All @@ -114,7 +108,6 @@ export class Store<S> implements IStore<S> {
}

dispose() {
this.stateSub.unsubscribe()
this.actionSub.unsubscribe()
this.action$.complete()
this.state$.complete()
Expand Down
9 changes: 6 additions & 3 deletions packages/react/src/__tests__/react.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,12 @@ describe('React components test', () => {
const TestComponent = () => {
const [localCount, setLocalCount] = useState(0)
const state = useModuleState(CountModel, {
selector: (state) => state.count + localCount,
selector: (state) => {
return state.count + localCount
},
dependencies: [localCount],
})
spy()
spy(state)
stub.callsFake(() => {
setLocalCount(localCount + 1)
})
Expand All @@ -87,7 +89,8 @@ describe('React components test', () => {
it('should render three times while change local state which in dependencies list', () => {
const reactNode = create(<TestComponent />)
act(() => stub())
expect(spy.callCount).toBe(3)
expect(spy.callCount).toBe(2)
expect(spy.args).toEqual([[0], [1]])
expect(reactNode).toMatchSnapshot()
})
})
Expand Down
50 changes: 27 additions & 23 deletions packages/react/src/index.browser.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { EffectModule, ActionOfEffectModule } from '@sigi/core'
import { ConstructorOf, IStore } from '@sigi/types'
import { useEffect, useState, useRef, useMemo, useCallback } from 'react'
import { distinctUntilChanged, map, skip } from 'rxjs/operators'
import { identity } from 'rxjs'
import { skip, tap } from 'rxjs/operators'

import { useInstance } from './injectable-context'
import { shallowEqual } from './shallow-equal'
Expand Down Expand Up @@ -32,38 +33,41 @@ function _useModuleState<S, U = S>(
}
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
dependencies = dependencies || []
const isFirstRendering = useRef(true)
const [appState, setState] = useState(() => {
const initialState = store.state
return selector ? selector(initialState) : initialState
})

const subscribe = useCallback(() => {
return store.state$
.pipe(
map((s) => (selector ? selector(s) : s)),
distinctUntilChanged((s1, s2) => equalFn(s1, s2)),
// skip initial state emission
skip(isFirstRendering.current ? 1 : 0),
)
.subscribe(setState)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [store, ...dependencies])
const stateRef = useRef<S | U>()
const depsRef = useRef<any[]>()
const selectorRef = useRef<(s: S) => S | U>()

if (!equalFn(depsRef.current, dependencies)) {
selectorRef.current = selector ?? identity
stateRef.current = selectorRef.current(store.state)
}
depsRef.current = dependencies

const subscription = useMemo(() => {
return subscribe()
const [_, _flipSig] = useState(false)

const tryUpdateState = useCallback((state: S) => {
const newState = selectorRef.current!(state)
if (!equalFn(stateRef.current, newState)) {
stateRef.current = newState
_flipSig((v) => !v)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [subscribe])
}, [])

const subscribe = useCallback(
() => store.state$.pipe(skip(1), tap(tryUpdateState)).subscribe(),
[store, tryUpdateState],
)
const subscription = useMemo(() => subscribe(), [subscribe])

useEffect(() => {
isFirstRendering.current = false
const maybeReSubscribeInConcurrencyMode = subscription.closed ? subscribe() : subscription
return () => {
maybeReSubscribeInConcurrencyMode.unsubscribe()
}
}, [subscription, subscribe])

return appState
return stateRef.current!
}

export function useModuleState<M extends EffectModule<any>>(
Expand Down
1 change: 1 addition & 0 deletions packages/vue/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export const reactive = <
}
}
if (changed) {
store['internalState'] = latestState
store.state$.next(latestState)
}
if (typeof originalBeforeUpdate === 'function') {
Expand Down

0 comments on commit 69b19ce

Please sign in to comment.