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

makeObservable should be called only after deserialization to enable better interop with mobx #149

Open
peey opened this issue Mar 18, 2021 · 1 comment

Comments

@peey
Copy link

peey commented Mar 18, 2021

Consider this unit test:

    class Reactive {
      @serializable
      @observable
      x : number;
      constructor(data : {x : number}) {
        Object.assign(this, data);
        makeObservable(this);
      }
    }

    const r = new Reactive({x: 3});
    const serialized = toPlaintextAndBack(serialize(r));
    const deserialized = deserialize(Reactive, serialized);
    when(() => deserialized.x === 4, () => {
      expect(deserialized.x).toEqual(4);
      done();
    })
    deserialized.x = 4;

It fails because the when is never triggered.

Difference between behavior of r and deserialzied arises because for r, .x is a property when makeObservable is called, and for deserialized .x is not a property on the object when makeObservable is called.

This is a subtle issue. I think intended resolution for this by the library is to provide a factory. But if I'm providing a factory:

    getDefaultModelSchema(Reactive)!.factory = context => {
      return new Reactive(context.json)
    }

Then a different issue is that some fields might be initialized twice. (once by the ctor arg, once by mobx).

I fear that these semantics are unnecessary complex to keep in mind and reason about and may lead to subtle bugs in the future (e.g. if I pass something in the factory, but then I see those values being overridden by serializr, or if there's a reaction on property write that gets triggered twice). So I'm not sure this is the right way.

I'd think that it'd make much more sense if serializr was "mobx-aware" and would call extendObservable after it's done setting properties.

@peey
Copy link
Author

peey commented Mar 19, 2021

The following sub-part of this issue:

Then a different issue is that some fields might be initialized twice. (once by the ctor arg, once by mobx).

Can be partially addressed if SKIP return value was supported from custom deserializers as a shorthand for ctx.target.prop, i.e. omit deserialization of this prop because it was handled in the factory ctor call.

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

1 participant