mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.55k stars 1.78k forks source link

The same object being proxied has different references in scope of the same observable #3917

Open andres-kovalev opened 2 months ago

andres-kovalev commented 2 months ago

When I assign the same const object as a value of the same observable - I get different reference every time:

store.reactiveField = constObject;
const cache = store.reactiveField;
store.reactiveField = constObject; // even tho we assign the same object, we got different reference
console.log(cache === store.reactiveField); // false
...

I understand store.reactiveField and constObject are different objects (the first one is a proxy for the second one), but it still looks counterintuitive and error prone to get a new reference every time. For instance, when this proxy is used as a dependency for a react hook:

useEffect(() => {
  // this effect is triggered every time I do `store.reactiveField = constObject`
}, [store.reactiveField]);

Is it possible to preserve the reference in a case when the actual proxy target has the same reference?

If there is a reason for such behavior, is there any workaround for that? At the moment the only idea I have is to keep the copy of the original (non-proxied) value and perform deep assign manually:

// without safety checks for simplicity
function deepAssign(reactive, original, value) {
  Object.entries(value).forEach(([ key, value ]) => {
    if (original[key] !== value) {
      reactive[key] = value;
      return;
    }

    if (typeof value !== 'object') {
      return;
    }

    deepAssign(reactive[key], original[key], value);
  });
}

Intended outcome:

I expect the same reference can be reused (since the underlying value has not changed):

console.log(cache === store.reactiveField); // true

Actual outcome:

Reference is different every time:

console.log(cache === store.reactiveField); // false

How to reproduce the issue:

Here is the example code to reproduce:

type TestObject = { number: number };
type Parent = { object: TestObject };

const object = { number: 0 };
const parent = { object };

class Store {
  value: ParentObject = {}

  constructor() {
    makeAutoObservable(this);
  }
}

const store = new Store();
store.value = parent;
const parentCache = store.value;
const objectCache = store.value.object;
store.value = parent;

console.log(parentCache === store.value); // false
console.log(objectCache === store.value.object); // false

Versions

^6.13.1

mweststrate commented 2 months ago

This is intentionally, to have behavior consistent throughout all APIs. If you want to share the same ref, just observable(object) (and store it!) first and then assign it to the property (e.g. const parent = observable({object})). Or, if that object doesn't have to become observable use an annotation to indicate so, for example makeAutoObservable(this, { value: observable.ref}).

https://mobx.js.org/observable-state.html#available-annotations

Please note that using observables in useEffect deps is an anti pattern, I recommend to set up a reaction inside a useEffect, and have MobX instead of React deal with the deps.

https://mobx.js.org/react-integration.html#useeffect

andres-kovalev commented 2 months ago

Thanks for the answer, especially for the link.

We were also thinking about converting an object into an observable, but it make the parent and child components know about each other's internals: parent component should wrap an object with observable() only because child puts this object into an observable state. Probably it's better to wrap it on a child level and memoize:

const Child = ({ object }: Props) => {
  const observableObject = useMemo(() => obervable({ object }), [object]);

  ...
}

WDYT?

mweststrate commented 2 months ago

yeah I think that should work.

On Thu, Sep 5, 2024 at 12:01 AM Andres Kovalev @.***> wrote:

Thanks for the answer, especially for the link https://mobx.js.org/react-integration.html#useeffect.

We were also thinking about converting an object into an observable, but it make the parent and child components know about each other's internals: parent component should wrap an object with observable() only because child puts this object into an observable state. Probably it's better to wrap it on a child level and memoize:

const Child = ({ object }: Props) => { const observableObject = useMemo(() => obervable({ object }), [object]);

... }

WDYT?

— Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/3917#issuecomment-2330221591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBBBX2LJHZBEBGDRQG3ZU57K5AVCNFSM6AAAAABNUYW3RCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZQGIZDCNJZGE . You are receiving this because you commented.Message ID: @.***>