Skip to content

Commit

Permalink
Merge pull request #653 from reduxjs/warning-stack
Browse files Browse the repository at this point in the history
  • Loading branch information
markerikson authored Dec 2, 2023
2 parents 5f0df89 + 50a327f commit 3712eae
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 8 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"prepack": "yarn build",
"bench": "vitest --run bench --mode production",
"test": "node --expose-gc ./node_modules/vitest/dist/cli-wrapper.js run",
"test:watch": "node --expose-gc ./node_modules/vitest/dist/cli-wrapper.js watch",
"test:cov": "vitest run --coverage",
"type-check": "vitest --run typecheck",
"type-check:trace": "vitest --run typecheck && tsc --noEmit -p typescript_test/tsconfig.json --generateTrace trace && npx @typescript/analyze-trace trace && rimraf trace",
Expand Down
10 changes: 9 additions & 1 deletion src/devModeChecks/identityFunctionCheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,19 @@ export const runIdentityFunctionCheck = (resultFunc: AnyFunction) => {
// Do nothing
}
if (isInputSameAsOutput) {
let stack: string | undefined = undefined
try {
throw new Error()
} catch (e) {
// eslint-disable-next-line @typescript-eslint/no-extra-semi, no-extra-semi
;({ stack } = e as Error)
}
console.warn(
'The result function returned its own inputs without modification. e.g' +
'\n`createSelector([state => state.todos], todos => todos)`' +
'\nThis could lead to inefficient memoization and unnecessary re-renders.' +
'\nEnsure transformation logic is in the result function, and extraction logic is in the input selectors.'
'\nEnsure transformation logic is in the result function, and extraction logic is in the input selectors.',
{ stack }
)
}
}
11 changes: 9 additions & 2 deletions src/devModeChecks/inputStabilityCheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ export const runInputStabilityCheck = (
createAnEmptyObject.apply(null, inputSelectorResults) ===
createAnEmptyObject.apply(null, inputSelectorResultsCopy)
if (!areInputSelectorResultsEqual) {
// do we want to log more information about the selector?
let stack: string | undefined = undefined
try {
throw new Error()
} catch (e) {
// eslint-disable-next-line @typescript-eslint/no-extra-semi, no-extra-semi
;({ stack } = e as Error)
}
console.warn(
'An input selector returned a different result when passed same arguments.' +
'\nThis means your output selector will likely run more frequently than intended.' +
Expand All @@ -40,7 +46,8 @@ export const runInputStabilityCheck = (
{
arguments: inputSelectorArgs,
firstInputs: inputSelectorResults,
secondInputs: inputSelectorResultsCopy
secondInputs: inputSelectorResultsCopy,
stack
}
)
}
Expand Down
25 changes: 21 additions & 4 deletions test/identityFunctionCheck.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,21 @@ import { createSelector, setGlobalDevModeChecks } from 'reselect'
import type { LocalTestContext, RootState } from './testUtils'
import { localTest } from './testUtils'

describe<LocalTestContext>('identityFunctionCheck', () => {
describe('identityFunctionCheck', () => {
const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
const identityFunction = vi.fn(<T>(state: T) => state)
const badSelector = createSelector(
let badSelector = createSelector(
[(state: RootState) => state],
identityFunction
)

afterEach(() => {
consoleSpy.mockClear()
identityFunction.mockClear()
badSelector.clearCache()
badSelector.memoizedResultFunc.clearCache()
badSelector = createSelector(
[(state: RootState) => state],
identityFunction
)
})
afterAll(() => {
consoleSpy.mockRestore()
Expand All @@ -39,6 +41,21 @@ describe<LocalTestContext>('identityFunctionCheck', () => {
}
)

localTest('includes stack with warning', ({ state }) => {
expect(badSelector(state)).toBe(state)

expect(identityFunction).toHaveBeenCalledTimes(2)

expect(consoleSpy).toHaveBeenCalledWith(
expect.stringContaining(
'The result function returned its own inputs without modification'
),
{
stack: expect.any(String)
}
)
})

localTest('disables check if global setting is set to never', ({ state }) => {
setGlobalDevModeChecks({ identityFunctionCheck: 'never' })

Expand Down
3 changes: 2 additions & 1 deletion test/inputStabilityCheck.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ describe('inputStabilityCheck', () => {
]),
secondInputs: expect.arrayContaining([
expect.objectContaining({ a: 1, b: 2 })
])
]),
stack: expect.any(String)
})
)
})
Expand Down

0 comments on commit 3712eae

Please sign in to comment.