Implement lazily defining types

[MatthiasKunnen]

Closes #139.
Closes #78.

Eager to know what you think!

[Neos3452]

This looks really great! But, I will need you to allow me few more days to dig into the details, as there are some corner cases that I want to make sure we cover. Right now I have one thing.

This will make @jsonArrayMember and so on, backwards incompatible. I was trying to avoid that as much as possible, but maybe it is not worth it in the end. As you contributed all of the work recently :) I will leave it up to whether we should stick with backwards compatibility or just go with v2. From my perspective v2 is all fine. Nice thing with v2 is that we could also drop the old knownTypes, and keep the deferred version only.

[MatthiasKunnen]

This will make @jsonArrayMember and so on, backwards incompatible.

The idea is that there is going to be one way and one way only to define types in order to prevent confusion and mistakes. v2 will be breaking.

Nice thing with v2 is that we could also drop the old knownTypes, and keep the deferred version only.

Indeed. To get rid of knownTypes completely I'm reworking inheritance, a preview can be seen here: https://github.com/MatthiasKunnen/TypedJSON/tree/rework-inheritance
Example:

    @jsonObjectInheritance({
        resolveType: data => {
            if ('inputType' in data) {
                return SmallNode;
            } else {
                return BigNode;
            }
        },
    })
    @jsonObject()
    abstract class Node {
        @jsonMember
        name: string;
    }
    @jsonObject
    class SmallNode extends Node {
        @jsonMember
        inputType: string;
        @jsonMember
        outputType: string;
    }
    @jsonObject
    class BigNode extends Node {
        @jsonArrayMember(() => String)
        inputs: Array<string>;
        @jsonArrayMember(() => String)
        outputs: Array<string>;
        constructor() {
            super();
            this.inputs = [];
            this.outputs = [];
        }
    }

[JohnWeisz]

Thanks for your work on this @MatthiasKunnen,

Any chance for a variation where we don't break already existing functionality?

I understand the value and demand for this approach, but if we were to add this in, previous tests without the callback syntax must continue to function, at least for a while: even if we want to phase the non-callback syntax out permanently at one point in the future, the current syntax should be deprecated first, but not outright removed with a single update. This would be a considerable breaking change.

Consequently, instead of replacing existing tests, we should add to them to test both callback and non-callback based syntax.

Any thoughts?

[MatthiasKunnen]

Any chance for a variation where we don't break already existing functionality?

We could, but I don't think we should. It would be better if there was only one way to define a type. Multiple different ways, even if one is deprecated, could confuse users and will require more code on our end. Merging the original jsonMember overloads with the current one will be a mess.

I understand the value and demand for this approach, but if we were to add this in, previous tests without the callback syntax must continue to function, at least for a while: even if we want to phase the non-callback syntax out permanently at one point in the future, the current syntax should be deprecated first, but not outright removed with a single update. This would be a considerable breaking change.

It is undeniably a breaking change yet I do not see any issue in adding a breaking change with a new major version. If users don't want breaking changes, they can stay with the current major version, can't they?

[Neos3452]

Let me add my two cents into this discussion. Although this is a big breaking change, there are practical reasons for that too. Every now, and then, we have a new issue with a problem that classes are depending on each other, and they happen to be incorrectly ordered. This solves this problem in a way that other projects with similar problem already did.

@MatthiasKunnen do you maybe know a way to make this migration easier for the users. I know angular provides an automated migration, but here maybe a regex would suffice? John, maybe you also know something that would be good enough of an approach?

[MatthiasKunnen]

@JohnWeisz, I discussed backwards compatibility with @Neos3452. It is possible but I'm not convinced about the advantages.

Backward compatibility could be achieved by re-adding constructor to the IJsonMemberOptions in this PR. The constructor option will be deprecated. We will deprecate all but jsonMember in favor of @jsonMember(() => ArrayT(Number)). In the next major release, all deprecated features are to be removed.

Disadvantages:

• The possibility of @jsonMember(() => String, {constructor: Number}) will confuse people so we'll have to add an error for that.
• We will need to duplicate all tests for both approaches
• We will need to add tests for the combined usage as described in the first point
• More code == larger bundle size == more bugs

Let me know what you think!

[JohnWeisz]

All I have is a slight API-design concern here, but before I go into details, with the changes in this PR, what would the following produce?

@jsonObject
class ClassA
{
    @jsonMember
    public propA: ClassA;

    @jsonMember
    public propB: ClassB;

    @jsonMember
    public propC: ClassC;
}

@jsonObject
class ClassB
{
    @jsonMember
    public propA: ClassA;

    @jsonMember
    public propB: ClassB;

    @jsonMember
    public propC: ClassC;
}

@jsonObject
class ClassC
{
    @jsonMember
    public propA: ClassA;

    @jsonMember
    public propB: ClassB;

    @jsonMember
    public propC: ClassC;
}

Would it still result in 3 errors logged into the console?

[MatthiasKunnen]

Your code snippet results in:

@jsonMember on ClassA.propB: could not resolve detected property constructor at runtime.Are you sure, that you have both "experimentalDecorators" and "emitDecoratorMetadata" in your tsconfig.json?
@jsonMember on ClassA.propC: could not resolve detected property constructor at runtime.Are you sure, that you have both "experimentalDecorators" and "emitDecoratorMetadata" in your tsconfig.json?
@jsonMember on ClassB.propC: could not resolve detected property constructor at runtime.Are you sure, that you have both "experimentalDecorators" and "emitDecoratorMetadata" in your tsconfig.json?

I suppose we should change this message into something like this: "@jsonMember on ClassB.propC: cannot determine type. Specify type using () => Type and make sure decorators are enabled in tsconfig.json`

[JohnWeisz]

Thanks for that @MatthiasKunnen

Alright so, please pardon me for trying to flip the table considerable late on this one, when a lot of awesome work has went into this excellent PR already.

Before all else, to make my agenda clear here: syntax clarity was my primary goal with this library (hence metadata-based, parameterless jsonMember being a preferred 1st class API), and I would like to keep it that way by all means. Whatever additional data we are forced to gather manually due to limitations in TS metadata (e.g. array and map element types), should be kept as bare metal simple to the core as technically possible (syntax-wise).

Consequentially, I'm kinda required to do some pushback on anything that risks this.

---

To this end, the 1st reason I'm slightly concerned about this change is that it bloats the primary API with additional syntax for something that will be a user mistake in like 95% of cases (in my experience at least):

- @jsonMapMember(Number, MySecondDataClass)
- public prop: Map<Number, MySecondDataClass>;
- @jsonMapMember(Number, MySecondDataClass)
- public prop: Map<Number, MySecondDataClass>;
- @jsonMapMember(Number, MySecondDataClass)
- public prop: Map<Number, MySecondDataClass>;
- 
+ @jsonMapMember(() => Number, () => MySecondDataClass)
+ public prop: Map<Number, MySecondDataClass>;
+ @jsonMapMember(() => Number, () => MySecondDataClass)
+ public prop: Map<Number, MySecondDataClass>;
+ @jsonMapMember(() => Number, () => MySecondDataClass)
+ public prop: Map<Number, MySecondDataClass>;

With these changes, we force a more verbose syntax on everyone, instead of just simply asking the occasional incorrect consumer code to swap the order of 2 or so classes (if they don't already have a linter that's been screaming at them already in the first place -- maybe this is something we should recommend in the documentation under the install section), or offering this as secondary syntax for the rare cases when there is really a legit need for it (e.g. circular dependencies, etc.).

This is my first problem with this API change. It forces a more verbose syntax on everyone even when there is zero need for it.

---

The 2nd thing I'm concerned about: we should consider the fact that metadata type lookup, which should remain the preferred syntax due to its sheer simplicity, i.e. this:

@jsonMember
public prop: ClassType;

... is, due to tech limitations in TS metadata emit, technically equal to this:

@jsonMember({ constructor: ClassType })
public props: ClassType;

... and not this:

@jsonMember(() => ClassType)
public props: ClassType;

Thus an explicit type definition now behaves considerably differently than metadata-based syntax, introducing an inconsistency in the 1st class API here.

+1: it's also a breaking change

---

With the above in mind, I prefer to keep the simplified, cleaner syntax as a preferred option, and instead support both callback and non-callback based syntax, because I don't see how the benefits outweigh the downsides here:

Pros of callbacks only:

• No need to duplicate most unit tests
• No need to resort to a secondary syntax to solve the occasional circular dependency and similar

Cons of callback only:

• Increased forced verbosity and syntax noise for everyone
• Major breaking change
• Inconsistent with preferred metadata-based behavior
• Encourages bad practices (arguably)

Now luckily...

To support both cases, I believe it should be possible to check for a name property on the passed in type to determine between the new callback-based and simplified syntax very easily (the presence of a name should indicate a constructor function and thus a class, whereas an anonymous callback won't have it).

So, pardon me again for asking this so late, but can we try to see if this works? I see a big win here with a small subsequent change.

---

Also, I understand this is a lot of detail when we are discussing something as simple as () =>, but hopefully my concerns are reasonable. Syntax clarity was the primary goal of this library, and I would like to keep it that way.

TL;DR: can we see if a name prop check on the passed in value lets us support both callback and simplified syntax? If it does the job and we can add in tests for both approaches, I'm all for giving this awesome PR a green light once it's done.

[MatthiasKunnen]

I don't agree with your statement that only callbacks encourages bad practices. Circular references are unavoidable at times. I also don't see a breaking change in your first example. That @jsonMember is different from @jsonMember(() => ClassType)? yes. Breaking change? no. @jsonMember would just have kept working.

I strongly favor single solution code, and find it worth it compared to the syntax noise, as it:

• Limits confusion by users
• Limits footguns
• Requires less code in the library
• Reduces complexity of the library

That being said, I've implemented backwards compatibility.

[Neos3452]

Personally, I am accustomed to arrow style declaration as they are frequently used in different frameworks, but I would say it is an outcome of javascript limitations nothing else. For us less options == less problems in code maintenance, and user questions. But, to not drag out the discussion, seeing that Matthias has implemented backwards compatibility I am merging this PR. Thanks to both of you for all the work!

[JohnWeisz]

I don't agree with your statement that only callbacks encourages bad practices. Circular references are unavoidable at times.

True, perhaps I just misemphasized this by mistake, what I really meant was that true circular references are IMO not as common as to be considered a 1st class API case, and in practice the core issue is often just an incorrect, easily fixable declaration order (which IMO is consequently not worth forcing the additional syntax verbosity). I also totally get your point about a single-solution API, but I'm ultimately biased towards a cleaner syntax whenever possible.

In any event, thanks for your awesome work on this PR @MatthiasKunnen and thanks for addressing the syntax concern.