Rework inheritance

[MatthiasKunnen]

Implemented in fork @Decoverto

This PR continues on top of #158 by reworking inheritance.

The PR removes knownTypes, typeHintEmitter, typeResolver, and nameResolver with a more comprehensive @jsonObjectInheritance decorator. The new implementation is also a lot less complex. 330 lines of source code get deleted 🚀.

Since this PR is branched from #158, the diff will include changes from that PR. For only the inheritance rework, see: MatthiasKunnen/TypedJSON@issue-139-define-type-lazily...MatthiasKunnen:rework-inheritance.

Todo:

• Add documentation
• Add more tests, any scenarios you would like to see tested?

[Neos3452]

I skimmed through it quickly, and I see two major points atm:

1. I think you completely removed typeHintEmitter which creates a discriminator used by a service consuming TypedJSON serialisation output. Without it, there is no easy way to serialise abstract objects, or am I missing something?
2. From experience when I had to use inheritance in serialisation it would take a form of a list of subclasses each having a predefined discriminator value. With the annotation you provided this seems pretty easy but would grow quickly in size with the amount of subclasses. This could be even better if one could provide a table of subclass -> discriminator value, and get a default serialisation out of the box, and allow to customise it completely with a callback like you did. Something like this https://www.baeldung.com/jackson-inheritance#2-per-class-annotations . What do you think?

[MatthiasKunnen]

1. I removed it indeed. Why? Because I worked with services such as GraphQL which only allow data matching an exact schema. When TypedJSON adds a _type parameter this schema is no longer adhered to and the call will fail.
   About serialization: I do not see an issue here, do you have a specific example or test I could perform?

2. I believe we should make our solutions as simple as possible, both for our sake and the user's sake. Ideally, users can understand and implement without reading documentation. To that end we should limit ourselves to the least amount of options required.
   The example you mentioned could be implemented as follows:

   @jsonObjectInheritance({
       resolveType: data => {
           const discriminators = {
               Cow: Cow,
               Dog: Dog,
               Cat: Cat,
           };
   
           return discriminators[data.type] ?? Animal;
       },
   })
   @jsonObject()
   class Animal {
   }

   I believe this approach would be more readable and easier to understand than adding the feature you described. It does not require more code on our end and does not leave the user thinking about how the resolveType function and discriminatorMatcher work together. What do you think?

[MatthiasKunnen]

@JohnWeisz, I've appended this PR with discriminatorProperty which is used like this:

        @jsonInheritance(discriminatorProperty({
            property: 'type',
            types: () => ({
                Employee: Employee,
                Investor: Investor,
                PartTimeEmployee: PartTimeEmployee,
            }),
        }))
        @jsonObject
        class Person {
            name: string;
        }

This functionality enables the following flow: {name: "Bob", type: "PartTimeEmployee"} gets deserialized into ParTimeEmployee {name: 'bob'} and this gets serialized back into {name: "Bob", type: "PartTimeEmployee"}.

Does this approach work for your use case?

[MatthiasKunnen]

I'm currently investigating some issues with circular references and multiple jsonInheritance decorators. I'll let you know when the PR is ready for review again.

[Neos3452]

Couple of points:

1. Can we get the old tests to have the same exceptions, but with just the new decorator? I saw some were changed, and we would like to support the same use cases
2. I saw some tests completely removed, why is that?
3. We need some docs, and explanation for the circular dependency issue. Would be nice to link to the original blog post
4. I think we will need to document an upgrade path too

[Neos3452]

Instead of mutation can it return a value that should be used as a result? Does not change much, but allows to use pure functions.

[MatthiasKunnen]

Done.

[Neos3452]

types() is evaluated for each element of the dictionary. You could save it in a variable

[MatthiasKunnen]

Does not make a difference IMO as I have to call a function regardless after the map is made, it is no longer called. I'll add a commit to show it. The result is less readable with minimal benefit IMO. I could save it in a variable and make it even less readable.

[Neos3452]

This is maybe a nit, but onSerializeType evaluates types once and stores the result, while here it will evaluated every time. It could mean the behaviour is not consistent across both

[MatthiasKunnen]

Same comment here: Does not make a difference IMO as I have to call a function regardless so no gain. I'll add a commit to show it. The result is less readable with minimal benefit IMO.

[MatthiasKunnen]

Rebase complete