redux-toolkit icon indicating copy to clipboard operation
redux-toolkit copied to clipboard

`trackProperties` method in `immutableStateInvariantMiddleware.ts` is not correctly managing the circularity

Open paztis opened this issue 9 months ago • 10 comments

https://github.com/reduxjs/redux-toolkit/blob/master/packages/toolkit/src/immutableStateInvariantMiddleware.ts#L57

function trackProperties(
  isImmutable: IsImmutableFunc,
  ignorePaths: IgnorePaths = [],
  obj: Record<string, any>,
  path: string = '',
  checkedObjects: Set<Record<string, any>> = new Set(),
) {
  const tracked: Partial<TrackedProperty> = { value: obj }

  if (!isImmutable(obj) && !checkedObjects.has(obj)) {
    checkedObjects.add(obj)
    tracked.children = {}

    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,
      )
    }
  }
  return tracked as TrackedProperty
}

recursive call to trackProperties (line 57) is not passing down the checkedObjects argument. It means all the circularity check you do in your code is totally bypass.

That's why immutable is not working with circular references

paztis avatar Feb 05 '25 16:02 paztis

I create a PR to fix this: https://github.com/reduxjs/redux-toolkit/pull/4844

paztis avatar Feb 05 '25 16:02 paztis

Hi. No approval or reject ? This problem is important to fix

paztis avatar Feb 20 '25 16:02 paztis

It's low on the priority list atm - all of us maintainers have been busy with other things the last few weeks. We'll try to take a look when we get a chance!

In the meantime, you could disable the default version of the immutability middleware, paste the modified version into your app, and add that to the store instead.

markerikson avatar Feb 20 '25 16:02 markerikson

I also have this same issue.

soyjuanmacias avatar Apr 03 '25 19:04 soyjuanmacias

Can someone validates the PR ? I think you can accept it easily ^^

paztis avatar Apr 04 '25 09:04 paztis

I think the CI failures have been putting people off - I'll try rebasing the PR to see if the CI failures still happen.

EskiMojo14 avatar Apr 04 '25 09:04 EskiMojo14

Same. Are the unit tests passing without my change ?

paztis avatar Apr 04 '25 11:04 paztis

yes - i rebased https://github.com/reduxjs/redux-toolkit/pull/4864 today and its CI is passing fine.

EskiMojo14 avatar Apr 04 '25 15:04 EskiMojo14

The PR seems to break things, and I don't have the time to investigate why. It does seem to be related to the value field that's created in the test data, not the value field from the TrackedProperties type.

If someone can debug it further, figure out why this breaks things, and fix it, we can look at merging this, but it's not on my priority list at this point.

markerikson avatar Apr 06 '25 18:04 markerikson

@paztis I just discovered the problema and fix it.

You have complete solution in: https://github.com/reduxjs/redux-toolkit/pull/4844#issuecomment-2782546797

soyjuanmacias avatar Apr 07 '25 09:04 soyjuanmacias