Skip to content
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

fix trackProperties recursion #4844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix trackProperties recursion #4844

wants to merge 1 commit into from

Conversation

paztis
Copy link

@paztis paztis commented Feb 5, 2025

passdown the checkedObjects to detect circularity

Copy link

codesandbox bot commented Feb 5, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

codesandbox-ci bot commented Feb 5, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6a17ea9:

Sandbox Source
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 6a17ea9
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/67efa6573d2b120008a05162
😎 Deploy Preview https://deploy-preview-4844--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@soyjuanmacias
Copy link

soyjuanmacias commented Apr 3, 2025

Same problem here, which can be solved with this PR

passdown the checkedObjects to detect circularity
@markerikson
Copy link
Collaborator

The stack trace here is particularly odd, given that line 138 seems to be in the middle of a comment?

TypeError: Cannot read properties of undefined (reading 'value')
 ❯ detectMutations node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:138:40
 ❯ detectMutations node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:138:20
 ❯ detectMutations node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:138:20
 ❯ detectMutations node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:138:20
 ❯ Object.detectMutations node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:41:3
 ❯ node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:187:5
 ❯ Object.measureTime node_modules/@reduxjs/toolkit/src/utils.ts:31:12
 ❯ node_modules/@reduxjs/toolkit/src/immutableStateInvariantMiddleware.ts:185:16
 ❯ src/tests/immutableStateInvariantMiddleware.test.ts:148:7

export interface ImmutableStateInvariantMiddlewareOptions {
/**
Callback function to check if a value is considered to be immutable.
This function is applied recursively to every value contained in the state.
The default implementation will return true for primitive types
(like numbers, strings, booleans, null and undefined).
*/
isImmutable?: IsImmutableFunc
/**
An array of dot-separated path strings that match named nodes from
the root state to ignore when checking for immutability.
Defaults to undefined
*/
ignoredPaths?: IgnorePaths
/** Print a warning if checks take longer than N ms. Default: 32ms */
warnAfter?: number
}

@markerikson
Copy link
Collaborator

I can reproduce the brokenness from this change locally, although the line numbers are different:

TypeError: Cannot read properties of undefined (reading 'value')
 ❯ detectMutations src/immutableStateInvariantMiddleware.ts:117:23
    115|       isImmutable,
    116|       ignoredPaths,
    117|       trackedProperty.children[key],
       |                       ^
    118|       obj[key],
    119|       sameRef,
 ❯ detectMutations src/immutableStateInvariantMiddleware.ts:114:20
 ❯ detectMutations src/immutableStateInvariantMiddleware.ts:114:20
 ❯ detectMutations src/immutableStateInvariantMiddleware.ts:114:20
 ❯ Object.detectMutations src/immutableStateInvariantMiddleware.ts:24:14
 ❯ src/immutableStateInvariantMiddleware.ts:228:28
 ❯ Object.measureTime src/utils.ts:9:16
 ❯ src/immutableStateInvariantMiddleware.ts:225:22
 ❯ src/tests/immutableStateInvariantMiddleware.test.ts:148:7

@markerikson
Copy link
Collaborator

It looks like the ".value of undefined" bit is related to the mock data being generated for the test:

state.foo.bar = new Array(10).fill({ value: 'more' })

rather than the .value field from the TrackedProperty objects.

Honestly I'm not sure what's going on here, and this is low enough priority that I don't want to get bogged down debugging it atm.

@soyjuanmacias
Copy link

soyjuanmacias commented Apr 7, 2025

Just solve it @paztis. Can you try it and modify #4844 ?

Problem:

I was encountering false-positive mutation detections because my trackProperties function wasn't consistently memoizing object references. When the same object was encountered multiple times (for example, using Array.fill({ value: 'more' }) or updating isPrefetch), a new tracker with an empty children property was created instead of reusing the existing one. This inconsistency caused the mutation detector to incorrectly report changes.

Solution:

I fixed the issue by introducing memoization in trackProperties using a WeakMap. Now, when processing an object:

  • If the object is a primitive or already considered immutable, I simply return a basic tracker.
  • If I’ve already seen the object, I retrieve its tracker from the WeakMap to ensure consistency.
  • Otherwise, I create a new tracker, store it in the WeakMap, and use it for all future detections.

This change ensures that each object reference is tracked only once, completely eliminating the false-positive mutation detections.

This is the complete code for trackProperties with the problem fixed. @paztis , only have replace and update the PR.

function trackProperties(
  isImmutable: IsImmutableFunc,
  ignorePaths: IgnorePaths = [],
  obj: Record<string, any>,
  path: string = '',
  checkedObjects: WeakMap<any, TrackedProperty> = new WeakMap(),
) {
  if (typeof obj !== 'object' || obj === null || isImmutable(obj)) {
    return { value: obj, children: {} }
  }

  if (checkedObjects.has(obj)) {
    return checkedObjects.get(obj)!
  }

  const tracked: TrackedProperty = { value: obj, children: {} }
  checkedObjects.set(obj, tracked)

  for (const key in obj) {
    const childPath = path ? path + '.' + key : key
    if (ignorePaths.length && ignorePaths.indexOf(childPath) !== -1) {
      continue
      }

    tracked.children[key] = trackProperties(
      isImmutable,
      ignorePaths,
      obj[key],
      childPath,
      checkedObjects,
    )
  }
  return tracked as TrackedProperty
}

And here, tests passed locally:
image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants