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

Complex selectors with immer #74

Closed
meluskyc opened this issue May 11, 2023 · 10 comments
Closed

Complex selectors with immer #74

meluskyc opened this issue May 11, 2023 · 10 comments

Comments

@meluskyc
Copy link
Contributor

meluskyc commented May 11, 2023

Hi,

I'm running into an issue when using nested selectors with immer. The test below fails for me.

https://codesandbox.io/s/youthful-grass-14m8cp?file=/src/index.test.js

TypeError: 'get' on proxy: property 'n' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '#<Object>' but got '#<Object>')

Any ideas? Thanks!

import { memoize } from "proxy-memoize";
import { produce } from "immer";

describe("nested selectors w/ immer", () => {
  it("throws", () => {
    let state = Object.freeze({
      x: {
        y: {
          z: [{ m: 3 }]
        }
      },
      q: {
        n: {
          mm: 21
        }
      }
    });

    const selectorOne = memoize((s) => {
      const n = s.q.n;
      console.log(n);
      return s.x.y.z[0];
    });
    const selectorTwo = memoize((s) => {
      const one = selectorOne(s);
      return one && one.m + s.x.y.z.length;
    });

    expect(selectorOne(state)).toEqual({ m: 3 });
    expect(selectorTwo(state)).toEqual(4);

    state = produce(state, (draft) => {
      draft.x.y = { z: [] };
    });

    // throws
    expect(selectorOne(state)).toEqual(undefined);
  });
});
@dai-shi
Copy link
Owner

dai-shi commented May 11, 2023

Thanks for reporting.
Out of curiosity, does it work for a simple case with immer?

@meluskyc
Copy link
Contributor Author

Yeah, this simple test succeeds, strangely. The complex case also succeeds in proxy-memoize 1.2.0, but 2.0.0 seems to break it.

import { memoize } from "proxy-memoize";
import { produce } from "immer";
import { describe, it, expect } from "@jest/globals";

describe("selectors w/ immer", () => {
  it("works", () => {
    let state = Object.freeze({
      x: {
        y: {
          z: [{ m: 3 }],
        },
      },
      q: {
        n: {
          mm: 21,
        },
      },
    });

    const selectorOne = memoize((s: typeof state) => {
      return s.x.y.z[0];
    });

    expect(selectorOne(state)).toEqual({ m: 3 });

    state = produce(state, (draft) => {
      draft.x.y = { z: [] };
    });

    // succeeds
    expect(selectorOne(state)).toEqual(undefined);
  });
});

@dai-shi
Copy link
Owner

dai-shi commented May 11, 2023

Interesting. Would you be able to dig more into it?
The difference between v1.2.0 and v2.0.0 can be in proxy-compare.

@meluskyc
Copy link
Contributor Author

Oops, scratch that. 2.0.3 works fine for this test and 2.0.4 seems to break it.

I'll try to look into it some more.

@valerii15298
Copy link

I am having the same problem. Using immer + zustand + this library.
Very inconvenient problem 😢 ...

Seems for me like immer returning objects with changed config for them...
Bcs in my case immer and proxy-memoize are not used at the same time, but when using immer he returns some object to zustand store and after that object is passed to proxy-memoize memoized function I have this error... but I will investigate more, very annoying problem...

@valerii15298
Copy link

https://immerjs.github.io/immer/freezing/#:~:text=Immer%20automatically%20freezes%20any%20state,this%20feature%20on%20or%20off

⚠️ If auto freezing is enabled, recipes are not entirely side-effect free: Any plain object or array that ends up in the produced result, will be frozen, even when these objects were not frozen before the start of the producer! ⚠️

This solves the issue:

import { setAutoFreeze } from "immer";
setAutoFreeze(false);

@meluskyc
Copy link
Contributor Author

meluskyc commented Jul 7, 2023

@valerii15298 Note that disabling immer's auto freeze will likely result in reduced performance. immerjs/immer#687

To recap, the issue seems to be immer's auto-freeze combined with the target cache from 220bffe returning the non-frozen proxy.

This is unfortunate since freezing helps prevent accidental state mutations. But I don't see another workaround outside of reverting 220bffe...

@dai-shi

@dai-shi
Copy link
Owner

dai-shi commented Jul 7, 2023

Recursive proxying frozen objects doesn't simply work. It's JS restriction:

$ node
Welcome to Node.js v18.14.0.
Type ".help" for more information.
> obj = { nested: {} }
{ nested: {} }
> Object.freeze(obj.nested)
{}
> Object.freeze(obj)
{ nested: {} }
> p = new Proxy(obj, {
... get(target, prop) { return new Proxy(target[prop], {}) }
... })
Proxy [ { nested: {} }, { get: [Function: get] } ]
> p.nested
Uncaught:
TypeError: 'get' on proxy: property 'nested' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '#<Object>' but got '#<Object>')

@meluskyc
Copy link
Contributor Author

meluskyc commented Jul 8, 2023

OK, it sounds like we should update the documentation to recommend disabling auto-freeze with immer. Would you like a PR for that?

It is strange since I would expect immer is also using nested proxies with frozen objects, but maybe that's not the case...

@dai-shi
Copy link
Owner

dai-shi commented Jul 8, 2023

Would you like a PR for that?

Yes, please.

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

No branches or pull requests

3 participants