mobxjs / serializr

Serialize and deserialize complex object graphs to and from JSON and Javascript classes
MIT License
763 stars 51 forks source link

bug: inconsistent field ordering in serialized object when using `createModelSchema` with inheritance #179

Open akphi opened 1 year ago

akphi commented 1 year ago

Description

When I specify the model schema for 2 classes where one extends the other, the order of fields during serialization of the subclass sometimes do not follow the order specified in in createModelSchema. Look at the example I provided

import { createModelSchema, primitive, serialize } from "serializr";

class Person {
  name;
}

class Worker extends Person {
  company;
}

// 1.
// const workerModelSchema = createModelSchema(Worker, {
//   company: primitive(),
//   name: primitive(),
// });
//
// output: { company: 'FirmX', name: 'John' } --> this works

const personModelSchema = createModelSchema(Person, {
  name: primitive(),
});

// 2.
const workerModelSchema = createModelSchema(Worker, {
  company: primitive(),
  name: primitive(),
});
//
// output: { name: 'John', company: 'FirmX' } --> this doesn't work as expected

// 3.
// const workerModelSchema2 = {
//   factory: () => new Worker(),
//   props: {
//     company: primitive(),
//     name: primitive(),
//   },
// };
//
// output: { company: 'FirmX', name: 'John' } --> this works

const newWorker = new Worker();
newWorker.name = "John";
newWorker.company = "FirmX";

const json = serialize(workerModelSchema, newWorker);

console.log(json);

I have 2 classes Person and Worker extends Person. When using createModelSchema, if I specify the model schema for Person before I specify the model schema for Worker, when I try to serialize an instance of Worker, the props in the model schema of Worker which overlap with the props in the model schema of Person will always be arranged first, and the fields which specialized to Worker will be arranged last, in the example, I'm trying to serialize using workerModelSchema, which specifies company field to go before name, but in the output, I don't see this.

Now, if I use the (1), where I specify the model schema of Worker before I do Person, I would get what I expected, or if I do (3) where I avoid using createModelSchema, I would get what I expected as well.

I really think this is a bug due to the inconsistency we see here, I suppose this has to do with some intricate handling we do in createModelSchema

Reproduce

https://github.com/akphi/issue-repo/pull/13

Thoughts & Ideas

I think it comes down to this logic here

https://github.com/mobxjs/serializr/blob/f8b9196bf6f9e47e56b0dfce2669ad7229b76c44/src/core/serialize.ts#L52

My workaround right now would just be to do something like

const workerSchema = createModelSchema(...);
workerSchema.extends = undefined;

But it's kind of hacky, I wonder if there's a way we can just default to have createModelSchema not doing magic, or maybe have a flag on createModelSchema to turn off this auto-detect extends mechanism, or maybe a flag on serialize to disable this, or a global configuration for serializr to disable this behavior. Let me know what you guys think! Thanks

paulftw commented 9 months ago

Based on this example I think the right way to serialize subclasses is to remove name from Worker and add getDefaultModelSchema(Worker).extends = getDefaultModelSchema(Person). This way name will always go first as the base class state should be (de)serialized before subclasses.

In general, having two serializers for name is a dangerous route because it's error prone: implementation details leak into sub class, have to remember to update multiple code locations if name logic changes, it's not clear what a parent should do when two subclasses serialize name in different ways, etc.

Currently all 3 snippets seem to declare inheritance in JS classes but instead of relying on the serializr inheritance features they use various workarounds to get a (sort of) correct output.

Worth mentioning that while JS guarantees key order, JSON dictionaries are still unsorted dictionaries and it's better to avoid relying on it.