Skip to content

Commit fbe4896

Browse files
committed
Merge branch 'master' into v9.0-integration
2 parents 0d9b0e1 + 854f3e1 commit fbe4896

File tree

4 files changed

+186
-8
lines changed

4 files changed

+186
-8
lines changed

.github/workflows/test.yml

+2-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ jobs:
5151
fail-fast: false
5252
matrix:
5353
node: ['16.x']
54-
ts: ['4.7', '4.8', '4.9', '5.0', '5.1']
54+
ts: ['4.7', '4.8', '4.9', '5.0', '5.1', '5.2']
55+
5556
steps:
5657
- name: Checkout repo
5758
uses: actions/checkout@v3

src/hooks/useSelector.ts

+15-1
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,12 @@ export function createSelectorHook(
105105
) {
106106
const toCompare = selector(state)
107107
if (!equalityFn(selected, toCompare)) {
108+
let stack: string | undefined = undefined
109+
try {
110+
throw new Error()
111+
} catch (e) {
112+
;({ stack } = e as Error)
113+
}
108114
console.warn(
109115
'Selector ' +
110116
(selector.name || 'unknown') +
@@ -114,6 +120,7 @@ export function createSelectorHook(
114120
state,
115121
selected,
116122
selected2: toCompare,
123+
stack,
117124
}
118125
)
119126
}
@@ -126,11 +133,18 @@ export function createSelectorHook(
126133
) {
127134
// @ts-ignore
128135
if (selected === state) {
136+
let stack: string | undefined = undefined
137+
try {
138+
throw new Error()
139+
} catch (e) {
140+
;({ stack } = e as Error)
141+
}
129142
console.warn(
130143
'Selector ' +
131144
(selector.name || 'unknown') +
132145
' returned the root state when called. This can lead to unnecessary rerenders.' +
133-
'\nSelectors that return the entire state are almost certainly a mistake, as they will cause a rerender whenever *anything* in state changes.'
146+
'\nSelectors that return the entire state are almost certainly a mistake, as they will cause a rerender whenever *anything* in state changes.',
147+
{ stack }
134148
)
135149
}
136150
}

src/utils/Subscription.ts

+38-5
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,26 @@ export function createSubscription(store: any, parentSub?: Subscription) {
9999
let unsubscribe: VoidFunc | undefined
100100
let listeners: ListenerCollection = nullListeners
101101

102+
// Reasons to keep the subscription active
103+
let subscriptionsAmount = 0
104+
105+
// Is this specific subscription subscribed (or only nested ones?)
106+
let selfSubscribed = false
107+
102108
function addNestedSub(listener: () => void) {
103109
trySubscribe()
104-
return listeners.subscribe(listener)
110+
111+
const cleanupListener = listeners.subscribe(listener)
112+
113+
// cleanup nested sub
114+
let removed = false
115+
return () => {
116+
if (!removed) {
117+
removed = true
118+
cleanupListener()
119+
tryUnsubscribe()
120+
}
121+
}
105122
}
106123

107124
function notifyNestedSubs() {
@@ -115,10 +132,11 @@ export function createSubscription(store: any, parentSub?: Subscription) {
115132
}
116133

117134
function isSubscribed() {
118-
return Boolean(unsubscribe)
135+
return selfSubscribed
119136
}
120137

121138
function trySubscribe() {
139+
subscriptionsAmount++
122140
if (!unsubscribe) {
123141
unsubscribe = parentSub
124142
? parentSub.addNestedSub(handleChangeWrapper)
@@ -129,21 +147,36 @@ export function createSubscription(store: any, parentSub?: Subscription) {
129147
}
130148

131149
function tryUnsubscribe() {
132-
if (unsubscribe) {
150+
subscriptionsAmount--
151+
if (unsubscribe && subscriptionsAmount === 0) {
133152
unsubscribe()
134153
unsubscribe = undefined
135154
listeners.clear()
136155
listeners = nullListeners
137156
}
138157
}
139158

159+
function trySubscribeSelf() {
160+
if (!selfSubscribed) {
161+
selfSubscribed = true
162+
trySubscribe()
163+
}
164+
}
165+
166+
function tryUnsubscribeSelf() {
167+
if (selfSubscribed) {
168+
selfSubscribed = false
169+
tryUnsubscribe()
170+
}
171+
}
172+
140173
const subscription: Subscription = {
141174
addNestedSub,
142175
notifyNestedSubs,
143176
handleChangeWrapper,
144177
isSubscribed,
145-
trySubscribe,
146-
tryUnsubscribe,
178+
trySubscribe: trySubscribeSelf,
179+
tryUnsubscribe: tryUnsubscribeSelf,
147180
getListeners: () => listeners,
148181
}
149182

test/hooks/useSelector.spec.tsx

+131-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import React, {
66
useLayoutEffect,
77
useState,
88
useContext,
9+
Suspense,
10+
useEffect,
911
} from 'react'
1012
import { createStore } from 'redux'
1113
import * as rtl from '@testing-library/react'
@@ -723,6 +725,130 @@ describe('React', () => {
723725
const expectedMaxUnmountTime = IS_REACT_18 ? 500 : 7000
724726
expect(elapsedTime).toBeLessThan(expectedMaxUnmountTime)
725727
})
728+
729+
it('keeps working when used inside a Suspense', async () => {
730+
let result: number | undefined
731+
let expectedResult: number | undefined
732+
let lazyComponentAdded = false
733+
let lazyComponentLoaded = false
734+
735+
// A lazy loaded component in the Suspense
736+
// This component does nothing really. It is lazy loaded to trigger the issue
737+
// Lazy loading this component will break other useSelectors in the same Suspense
738+
// See issue https://github.com/reduxjs/react-redux/issues/1977
739+
const OtherComp = () => {
740+
useLayoutEffect(() => {
741+
lazyComponentLoaded = true
742+
}, [])
743+
744+
return <div></div>
745+
}
746+
let otherCompFinishLoading: () => void = () => {}
747+
const OtherComponentLazy = React.lazy(
748+
() =>
749+
new Promise<{ default: React.ComponentType<any> }>((resolve) => {
750+
otherCompFinishLoading = () =>
751+
resolve({
752+
default: OtherComp,
753+
})
754+
})
755+
)
756+
let addOtherComponent: () => void = () => {}
757+
const Dispatcher = React.memo(() => {
758+
const [load, setLoad] = useState(false)
759+
760+
useEffect(() => {
761+
addOtherComponent = () => setLoad(true)
762+
}, [])
763+
useEffect(() => {
764+
lazyComponentAdded = true
765+
})
766+
return load ? <OtherComponentLazy /> : null
767+
})
768+
// End of lazy loading component
769+
770+
// The test component inside the suspense (uses the useSelector which breaks)
771+
const CompInsideSuspense = () => {
772+
const count = useNormalSelector((state) => state.count)
773+
774+
result = count
775+
return (
776+
<div>
777+
{count}
778+
<Dispatcher />
779+
</div>
780+
)
781+
}
782+
// The test component outside the suspense (uses the useSelector which keeps working - for comparison)
783+
const CompOutsideSuspense = () => {
784+
const count = useNormalSelector((state) => state.count)
785+
786+
expectedResult = count
787+
return <div>{count}</div>
788+
}
789+
790+
// Now, steps to reproduce
791+
// step 1. make sure the component with the useSelector inside the Suspsense is rendered
792+
// -> it will register the subscription
793+
// step 2. make sure the suspense is switched back to "Loading..." state by adding a component
794+
// -> this will remove our useSelector component from the page temporary!
795+
// step 3. Finish loading the other component, so the suspense is no longer loading
796+
// -> this will re-add our <Provider> and useSelector component
797+
// step 4. Check that the useSelectors in our re-added components still work
798+
799+
// step 1: render will render our component with the useSelector
800+
rtl.render(
801+
<>
802+
<Suspense fallback={<div>Loading... </div>}>
803+
<ProviderMock store={normalStore}>
804+
<CompInsideSuspense />
805+
</ProviderMock>
806+
</Suspense>
807+
<ProviderMock store={normalStore}>
808+
<CompOutsideSuspense />
809+
</ProviderMock>
810+
</>
811+
)
812+
813+
// step 2: make sure the suspense is switched back to "Loading..." state by adding a component
814+
rtl.act(() => {
815+
addOtherComponent()
816+
})
817+
await rtl.waitFor(() => {
818+
if (!lazyComponentAdded) {
819+
throw new Error('Suspense is not back in loading yet')
820+
}
821+
})
822+
expect(lazyComponentAdded).toEqual(true)
823+
824+
// step 3. Finish loading the other component, so the suspense is no longer loading
825+
// This will re-add our components under the suspense, but will NOT rerender them!
826+
rtl.act(() => {
827+
otherCompFinishLoading()
828+
})
829+
await rtl.waitFor(() => {
830+
if (!lazyComponentLoaded) {
831+
throw new Error('Suspense is not back to loaded yet')
832+
}
833+
})
834+
expect(lazyComponentLoaded).toEqual(true)
835+
836+
// step 4. Check that the useSelectors in our re-added components still work
837+
// Do an update to the redux store
838+
rtl.act(() => {
839+
normalStore.dispatch({ type: '' })
840+
})
841+
842+
// Check the component *outside* the Suspense to check whether React rerendered
843+
await rtl.waitFor(() => {
844+
if (expectedResult !== 1) {
845+
throw new Error('useSelector did not return 1 yet')
846+
}
847+
})
848+
849+
// Expect the useSelector *inside* the Suspense to also update (this was broken)
850+
expect(result).toEqual(expectedResult)
851+
})
726852
})
727853

728854
describe('error handling for invalid arguments', () => {
@@ -805,6 +931,7 @@ describe('React', () => {
805931
}),
806932
selected: expect.any(Number),
807933
selected2: expect.any(Number),
934+
stack: expect.any(String),
808935
})
809936
)
810937
})
@@ -920,7 +1047,10 @@ describe('React', () => {
9201047
)
9211048

9221049
expect(consoleSpy).toHaveBeenCalledWith(
923-
expect.stringContaining('returned the root state when called.')
1050+
expect.stringContaining('returned the root state when called.'),
1051+
expect.objectContaining({
1052+
stack: expect.any(String),
1053+
})
9241054
)
9251055
})
9261056
})

0 commit comments

Comments
 (0)