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

Support for nullable types #32

Open
olivierr91 opened this issue Apr 10, 2020 · 5 comments
Open

Support for nullable types #32

olivierr91 opened this issue Apr 10, 2020 · 5 comments

Comments

@olivierr91
Copy link

olivierr91 commented Apr 10, 2020

Placing @field() above field of nullable types such as:

  • string | null
  • number | null

results in a runtime error. Is it possible to add support for this?

@AndriiZelenskyi
Copy link
Owner

Hello, @orobert91! Sure, I will take a look at it soon. Thank you for creating an issue! Could you provide some feedback about the library, I would like to make it more useful.

@olivierr91
Copy link
Author

olivierr91 commented Apr 13, 2020

Hi I took a deeper look at how reflection works in TypeScript and from what I've read it is not possible to retrieve the type from design:type when using nullable primitives. Until Microsoft improves it, there won't be much you can do to support this.

Here are my 2 workarounds:

  1. Force the type serialization using @Type(new PrimitiveSerializer())
  2. Use undefined instead of null (myVar?: string instead of myVar: string | null)

Option 1 pollutes the models a lot, option 2 is preferable since TypeScript guidelines encourages us to not use null at all (https://github.com/microsoft/TypeScript/wiki/Coding-guidelines)

As for the feedback of the library in general, I think it's well done and well designed. I have found 2 additional limitations:

  1. Missing support for serializing/deserializing dictionaries and maps. This is easily solved by writing a custom serializer.
  2. Support for circular dependencies, often present in business entities (for example a PurchaseOrder that contains Items, and Items that reference to the parent PurchaseOrder). Once again I think it is more of a TypeScript limitation than a problem with your library, because design:type is very unreliable in these circumstances. These are discussed here
    Better design:type and reflection microsoft/TypeScript#14971
    and here
    Design:type metadata for cyclic dependencies throw at runtime microsoft/TypeScript#27519

@AndriiZelenskyi
Copy link
Owner

Thank you for this research! I think I have to add something about it to the README file.

What other steps do you see to improve it? Maybe change the error message or something like that.

Could you send a use case for map serialization? I could add a default serializer for dictionaries and maps.

@olivierr91
Copy link
Author

Yes you could throw a more descriptive error message. It was a generic JavaScript message and I had to dig a lot to understand what was going on.

Here is an example class:

@Model()
export default class UserModel {
    @Field()
    @Type(new DictionarySerializer(new ModelSerializer(AccessControlEntryModel)))
    public accessControlEntries: { [securableName: string]: AccessControlEntryModel } = {};
}

And here is the serializer:

import { Serializer } from "serialize-ts";

export class DictionarySerializer<T extends Object> implements Serializer<{ [index in string | number]: T }> {
    serialize: (model: { [index in string | number]: T }) => Object | null;
    deserialize: (json: Object) => { [index in string | number]: T } | null;

    constructor(serializer: Serializer<T>) {
        this.serialize = model => {
            let result: { [index in string | number]: T } = {}
            for (let key in model) {
                result[key] = serializer.serialize(model[key]);
            }
            return result;
        }
        this.deserialize = json => {
            let result: { [index in string | number]: T } = {}
            for (let key in json) {
                result[key] = serializer.deserialize(json[key]);
            }
            return result;
        }
    }
}

The same can be done for Maps as they are very similar.

@AndriiZelenskyi
Copy link
Owner

Thank you for your feedback! I do appreciate it!

I have created sub-tasks for it to track it easier. I will take care of it soon! 😇

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

2 participants