makeObservable should be called only after deserialization to enable better interop with mobx
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.
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.