JohnWeisz / TypedJSON

Typed JSON parsing and serializing for TypeScript that preserves type information.
MIT License
603 stars 64 forks source link

Rework inheritance #159

Open MatthiasKunnen opened 3 years ago

MatthiasKunnen commented 3 years ago

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 :rocket:.

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

Todo:

Neos3452 commented 3 years ago

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 commented 3 years ago
  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 commented 3 years ago

@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 commented 3 years ago

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.

MatthiasKunnen commented 3 years ago

Rebase complete