Conditional type support

[domoritz]

Ref #100

See notes in #100 (comment)

[domoritz]

Let me know when this is ready for review.

[kayahr]

I think it's ready now. I did the following changes:

• Extracted "isAssignable" logic to a utility function and improved it (Inspired by implementation from ts-simple-type). Also wrote lots of unit tests for it.
• Separated "Any" and "Unknown" type because they work different for isAssignableTo checks.
• Simplified the conditional type node parser down to a few lines. I think it is now very easy to understand.
• Added another test for Omit type.

[kayahr]

Supporting enums in conditional type handling now so this PR also fixes #71

[domoritz]

Can you merge master/rebase?

[kayahr]

I'm still completely stuck here and I'm not so sure anymore that the problem with the lost annotations are actually caused by the conditional types. I can't reproduce these problems when using conditional types without a mapping type. And I don't really understand the annotation parsing in the generator and why all this is working with some mapped types at all.

So what are our options? Would be a shame not to merge this just because some complex scenarios are not fully working. When looking at the generated vega-lite schema it actually looks pretty good when you ignore the fact that some annotations are lost and some enum types are dereferened (Couldn't re-produce this outside of vega-lite yet).

[domoritz]

/**
 * Hello world
 */
type LabelOpacity = string | number

export type MyObject = LabelOpacity extends number ? never : LabelOpacity;

The comment gets lost here as well

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "type": "string"
    }
  }
}

Maybe there is a better way to write the TypeScript.

I agree that it would be a shame to not merge this. Let's try for a bit more but I am definitely not against merging this even if Vega-Lite doesn't work yet.

[domoritz]

With

/**
 * Hello world
 */
type LabelOpacity = string | number;

export type MyObject = Exclude<LabelOpacity, number>;

we get the comment from Exclude

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "description": "Exclude from T those types that are assignable to U",
      "type": "string"
    }
  }
}

Which is not all that helpful.

[domoritz]

I suspect what is happening is that we are looking up the docs from the conditional type but we usually care more about the comments on the thing we excluded from.

Maybe we can have a special case for Exclude where we ignore its comment and go straight to the type we exclude from?

Both of these

/**
 * Hello world
 */
type OurString = string;

type LabelOpacity = OurString | number;

export type MyObject = Exclude<LabelOpacity, number>;

/**
 * Hello world
 */
type LabelOpacity = string | number;

export type MyObject = Exclude<LabelOpacity, number>;

should produce

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "description": "Hello world",
      "type": "string"
    }
  }
}

/**
 * Hello world
 */
export type OurString = string;

type LabelOpacity = OurString | number;

export type MyObject = Exclude<LabelOpacity, number>;

would still produce, this, though

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "$ref": "#/definitions/OurString",
      "description": "Exclude from T those types that are assignable to U"
    },
    "OurString": {
      "description": "Hello world",
      "type": "string"
    }
  }
}

[domoritz]

This case actually works

/**
 * Hello world
 */
type OurString = string;

type LabelOpacity = OurString | number;

export type MyObject = LabelOpacity extends number ? never : LabelOpacity;

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "description": "Hello world",
      "type": "string"
    }
  }
}

So one way would be to move the comments to the remaining type. However, it would be nice to use the comment from the type that we remove something from (here LabelOpacity).

[domoritz]

@kayahr do these small use cases help you? I would love to merge this feature soon and make a new release.

[kayahr]

Yes, I think they are helpful. I ran out of time for now but I definitely want to look into it.

[domoritz]

Great.

Btw, my offer to send you chocolate to thank you for adding caching still stands.

[kayahr]

So you were serious about this? :) Well, thanks for the offer but it is really not necessary. And chocolate isn't healthy anyway :)

[kayahr]

Your first example is fixed now:

/**
 * Hello world
 */
type LabelOpacity = string | number

export type MyObject = LabelOpacity extends number ? never : LabelOpacity;

This wasn't actually a jsdoc issue. The result type was just wrong. TypeScript only narrows down the result type if it is a type parameter matching the check type. This isn't the case here so the result type must be LabelOpacity and not string. With this fixed the JSDoc from LabelOpacity is now automatically inherited by MyObject.

I'll continue now with your next example (In case you wonder, I'm giving them thumbs-up when finished to keep track)

[kayahr]

Maybe we can have a special case for Exclude where we ignore its comment and go straight to the type we exclude from?

Yes, it's useless to copy the description of the Exclude type and we should ignore this somehow but I disagree on going straight to the type we exclude from. We should use the actual result type instead. Let's take a look at your examples again:

/**
 * Hello world
 */
type OurString = string;
type LabelOpacity = OurString | number;
export type MyObject = Exclude<LabelOpacity, number>;

The result of the Exclude type here is OurString so we use the comment of this type. That's fine by me. But:

/**
 * Hello world
 */
type LabelOpacity = string | number;
export type MyObject = Exclude<LabelOpacity, number>;

The result of this Exclude is string which has nothing to do anymore with the LabelOpacity type so in my opinion the comment of type string (Which has no comment) must be used here so no description will end up in the schema. If you need a description for this type you have to add a comment to the "MyObject" type instead.

[kayahr]

Any good idea how to safely detect the internal TypeScript Exclude type? I can do a simple ts.isTypeAliasDeclaration(node) && node.name.getText() === "Exclude" check but this would also target custom types named Exclude... I could also check if the source file of the type is in node_modules/typescript/lib but this just doesn't feel right...

[kayahr]

For now I've added a "not-so-nice" hack. Because I guess ignoring Exclude is not enough because there are many other types in the typescript libs from which we don't want to use the comments I'm ignoring now all comments coming from nodes of the typescript libs by checking the source file path with a regular expression. If you know a better solution then please tell me.

With this last fix all the examples you mentioned are working with the already explained exception of not using the comment from a union type which was narrowed down by an exclude because in my opinion that would be wrong.

So this:

/** Description A */
type A = string;

/** Description B */
type B = number;

/** Description AorB */
type AorB = A | B;

/** Description StringOrNumber */
type StringOrNumber = string | number;

export type MyObject = {
    a?: Exclude<AorB, B>;
    b?: Exclude<AorB, A>;
    c?: Exclude<AorB, boolean>;
    d?: Exclude<StringOrNumber, string>;
};

Now becomes this:

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "additionalProperties": false,
      "properties": {
        "a": {
          "description": "Description A",
          "type": "string"
        },
        "b": {
          "description": "Description B",
          "type": "number"
        },
        "c": {
          "anyOf": [
            {
              "description": "Description A",
              "type": "string"
            },
            {
              "description": "Description B",
              "type": "number"
            }
          ],
          "description": "Description AorB"
        },
        "d": {
          "type": "number"
        }
      },
      "type": "object"
    }
  }
}

Notice that property "d" has no description because type "number" has no comment. And in my opinion this is correct.

[domoritz]

This will also affect Partial, which we use in Vega-Lite but that seems okay.

[domoritz]

I tried to compile the Vega-Lite schema but am now running into an issue with the stack size. If I set the stack size to 10000 (smaller values are not enough) then I get a segfault. Apparently, that's because node's --stack-size doesn't actually set the stack size but just limits the stack size. V8 has a maximum stack size and it's too small for this use case. I need to find out why we go so deep and see how we can resolve it before I can really test this PR.

You can reproduce the issue by going into dom/omit-value-ref and then running yarn schema.

[domoritz]

Conditional doesn't actually seem to work in mapped types anymore.

For example, if you use

export interface Axis {
    /**
     * The label opacity.
     *
     * @minimum 0
     */
    labelOpacity: number | string;
}

type OmitString<T> = {
    [P in keyof T]: Exclude<T[P], string>
};

export type MyObject = OmitString<Axis>;

it produces (note how the string is still in the enum)

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "additionalProperties": false,
      "properties": {
        "labelOpacity": {
          "description": "The label opacity.",
          "minimum": 0,
          "type": [
            "number",
            "string"
          ]
        }
      },
      "required": [
        "labelOpacity"
      ],
      "type": "object"
    }
  }
}

[kayahr]

Hm? I just tried your example and it works fine here:

{
  "$ref": "#/definitions/MyObject",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "MyObject": {
      "additionalProperties": false,
      "properties": {
        "labelOpacity": {
          "type": "number"
        }
      },
      "required": [
        "labelOpacity"
      ],
      "type": "object"
    }
  }
}

And BTW: The conditional type isn't the reason here why the comment of the property gets lost. That's a problem of the mapping type. With a mapping like the one below you also lose all comments without conditional types to be involved so that should be considered a separate bug:

type StringsEverywhere<T> = {
    [P in keyof T]: string
};

[kayahr]

I think the stack overflow is an unrelated problem. Yes, the schema generator is much more under stress when the millions of conditional types in vega-lite are processed now but in the end I think the problem is here. This code recursively generates a very large array. I checked the content and noticed that there are a lot of duplicates in it. And I'm not speaking about equal objects with same ids, I speak about exactly the same objects. I don't think this makes sense and it can be fixed like this:

children.push(...new Set(this.childTypeFormatter.getChildren(type)));

Maybe an even better solution would be to change the return type of this getChildren() to a Set everywhere.

After fixing this the stack overflow is gone and you get a new error:

Error: Type "Field" has multiple definitions.
    at /home/k/Projects/ts-json-schema-generator/dist/src/SchemaGenerator.js:69:23

There are indeed two different types in Vega named Field. There is already an open issue for this: #113 (Edit: This issue wasn't what I thought it was so I created a new one: #143)

So both problems seems to be unrelated to this PR:

[domoritz]

Hm? I just tried your example and it works fine here:

Sorry, wrong program. Try this one.

export interface Axis {
    /**
     * The label opacity.
     *
     * @minimum 0
     */
    labelOpacity: number | string;
}

type OmitString<T> = {
    [P in keyof T]: T[P] extends string ? never : T[P]
};

export type MyObject = OmitString<Axis>;

But now I am realizing that the issue is Typescript itself.

Alright, lets merge this and keep iterating.