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

"foo" | "bar" | string; should not only map to a string #1921

Closed
domoritz opened this issue Apr 15, 2024 · 9 comments · Fixed by #1927
Closed

"foo" | "bar" | string; should not only map to a string #1921

domoritz opened this issue Apr 15, 2024 · 9 comments · Fixed by #1927
Labels
released This issue/pull request has been released.

Comments

@domoritz
Copy link
Member

We should preserve string literals for better autocomplete.

Right now

export type MyType = "foo" | "bar" | string;

just results in

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

This makes sense since the literals are strings. See microsoft/TypeScript#29729 for why TS behaves this way.

It would be nice to recognize when string literals and strings are merged and not merge them.

See YousefED/typescript-json-schema#164 and #20.

Also uwdata/mosaic#358

@arthurfiorette
Copy link
Collaborator

arthurfiorette commented Apr 16, 2024

Typescript itself resolves "foo" | "bar" | string to string.

You can use "foo" | "bar" | (string & {}) to handle it correctly. This is what frameworks do when they need to support autocomplete AND any string.

image

image

@domoritz
Copy link
Member Author

"foo" | "bar" | (string & {}) feels like a hack so I wonder whether we should just always have the string literals in the json schema. Alternatively, we could try to detect the (string & {}) pattern.

@arthurfiorette
Copy link
Collaborator

I also don't like string & {} but diverging from typescript feels a no go at least for me.

Can (string & {}) be detected in an easy way?

@domoritz
Copy link
Member Author

I haven't checked.

@jheer
Copy link
Member

jheer commented Apr 19, 2024

I would very much like to have support for autocomplete "hacks" like string & {} or string & Record<never,never>. This would enable proper JSON schema generation in the Mosaic project.

I dug in a bit and it seems one place to potentially handle this is in the IntersectionTypeFormatter class. I tried inserting the following around line 33. The idea is to catch a type paired with {} and propagate the intended type forward. (The existing code path appears to assume an object, and I think tries to aggregate a collection of properties, which fails here when confronted with literals.)

            // handle autocomplete hacks like `string & {}`
            if (nonArrayLikeTypes.length == 2) {
                const types = nonArrayLikeTypes.filter(x => !isEmptyObject(x));
                if (types.length === 1) {
                    return this.childTypeFormatter.getDefinition(types[0]);
                }
            }

where isEmptyObject is simply the following (you might have a better/cleaner/more general idea of how to do this):

function isEmptyObject(x) {
    const t = x instanceof AliasType ? x.type : x;
    return t instanceof ObjectType && !t.additionalProperties && !t.properties.length;
}

This works in my limited tests except the output may include redundant types (e.g., a template string may ground to a literal that was already included for autocomplete purposes), so after such processing any union types need to be rechecked for repeated definitions.

OR: An entirely different approach (that I have not tried to implement) might be to instead allow Type AST transforms: a custom (opt-in?) transform could walk the type tree prior to formatting and rewrite nodes where the above pattern occurs, and also perform consolidation for any immediately surrounding union types.

Happy to take a stab at a PR if any of these directions are deemed desirable.

@domoritz
Copy link
Member Author

domoritz commented Apr 19, 2024

I tried export type MyType = "foo" | "bar" | (string & {}); and never even get into IntersectionTypeFormatter. It seems to already be gone by the time I'm in the UnionNodeParser.

Screenshot 2024-04-18 at 23 34 36

I'm thinking the cleanest approach might be to mark the StringType somehow to signal to the LiteralUnionTypeFormatter to leaves the literals in the enum. I'm playing with this (and generally simplifying the string literal union handling) in #1927.

Copy link

🚀 Issue was released in v2.1.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Apr 20, 2024
@Jason3S
Copy link
Contributor

Jason3S commented May 3, 2024

@domoritz,

This is an awesome feature improvement. At the same time, I wish it was behind a feature flag where I could opt in / out. It exploded the size of the generated schema and caused validation issues.

@domoritz
Copy link
Member Author

domoritz commented May 3, 2024

Let's see whether we can fix the issues. I'm not a fan of too many feature flags since it makes testing and development way harder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants