-
Notifications
You must be signed in to change notification settings - Fork 210
fix: inline purely structural generics #2293
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
base: next
Are you sure you want to change the base?
Conversation
…works not in all cases
if (!(type instanceof AliasType)) { | ||
return false; | ||
} | ||
if (!node.typeParameters?.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!node.typeParameters?.length) { | |
if (node.typeParameters?.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, with this, some other tests fail since undefined != 0
but non‑generic type aliases like type PrivateAnonymousTypeLiteral = { … }
have node.typeParameters
set to undefined
and the function returns false
in that case.
return /[\\/]typescript[\\/]lib[\\/]/i.test(sourceFile.fileName); | ||
} | ||
|
||
private shouldInline(node: ts.Node, type: BaseType, context: Context): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this handle cases where a symbol is created that can be used in multiple cases? Then it would be beneficial to have the alias, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a case like this?
export type MyHelper<A, B> = {
[K in keyof A as K extends keyof B ? never : K]: A[K];
} & B;
type Base = { foo: string; bar: number };
type Patch = { bar: string; baz: boolean };
type Resolved = MyHelper<Base, Patch>; // ← `Resolved` NOT EXPORTED
export interface Foo {
beta: Resolved;
}
export interface Bar {
gamma: Resolved;
}
which previously produced
before
{
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"Bar": {
"additionalProperties": false,
"properties": {
"gamma": {
"$ref": "#/definitions/MyHelper<alias-550436553-91-134-550436553-0-329,alias-550436553-134-178-550436553-0-329>"
}
},
"required": ["gamma"],
"type": "object"
},
"Foo": {
"additionalProperties": false,
"properties": {
"beta": {
"$ref": "#/definitions/MyHelper<alias-550436553-91-134-550436553-0-329,alias-550436553-134-178-550436553-0-329>"
}
},
"required": ["beta"],
"type": "object"
},
"MyHelper<alias-550436553-91-134-550436553-0-329,alias-550436553-134-178-550436553-0-329>": {
"additionalProperties": false,
"properties": {
"bar": { "type": "string" },
"baz": { "type": "boolean" },
"foo": { "type": "string" }
},
"required": ["bar", "baz", "foo"],
"type": "object"
}
}
}
and now produces
after
{
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"Bar": {
"additionalProperties": false,
"properties": {
"gamma": {
"additionalProperties": false,
"properties": {
"bar": { "type": "string" },
"baz": { "type": "boolean" },
"foo": { "type": "string" }
},
"required": ["bar", "baz", "foo"],
"type": "object"
}
},
"required": ["gamma"],
"type": "object"
},
"Foo": {
"additionalProperties": false,
"properties": {
"beta": {
"additionalProperties": false,
"properties": {
"bar": { "type": "string" },
"baz": { "type": "boolean" },
"foo": { "type": "string" }
},
"required": ["bar", "baz", "foo"],
"type": "object"
}
},
"required": ["beta"],
"type": "object"
}
}
}
?
In this case, all that's needed to enable reuse is to export type Resolved
; then the generator will emit a single definition with $ref
s. If the user doesn’t control the source, they probably don’t want those verbose alias-550436553-…
identifiers in their schema anyway. They may make the schema slightly smaller, but is there any real benefit to having it cluttered with definitions like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we have a principle to reuse definitions (even if not explicitly exported). It helps with generating code in other programming languages for example.
I wonder whether we can take a different approach. We could check the generated schema and inline aliases that are generated (rather than inferred from type names) for those cases where the alias is only used once. This could be something that covers the original case you described as well as other cases.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like a simple post-processing pass? That's exactly what I do in my own projects with this lib to strip out the generic helper types, so yes - totally feasible.
The one thing: in the example I showed earlier we'd still end up with a definition called
MyHelper<alias-550436553-91-134-550436553-0-329,alias-550436553-134-178-550436553-0-329>
, right? Would you want to keep it as-is?
And do you think it's worth exploring a way to rename such definitions to a clearer name like MyHelper<Base, Patch>
instead of those long alias-…
placeholders? Curious whether you see that as doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be post processing on the json but somewhere later in the flow. I think we have some deduplication logic if I recall correctly where this could happen.
Renaming aliases makes sense but would also need to be a separate pass when we can ensure that we don't have collisions. I'm less worried about that since you can export your alias if you do care about its name.
Co-authored-by: Dominik Moritz <[email protected]>
if (!sourceFile) { | ||
return false; | ||
} | ||
return /[\\/]typescript[\\/]lib[\\/]/i.test(sourceFile.fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this work in windows?
Partially closes #2233
When a type alias with generic parameters ultimately resolves to a purely structural type (no remaining type-parameters, no public reusable symbol), emitting it as a separate
$ref
just clutters the schema tree.This PR detects those cases and inlines the resolved structure directly into the parent definition, collapsing long reference chains and trimming unused
definitions
.Examples
Case 1 — Mapped/Override helper
Before
After
Case 2 —
ValueOf
on a const objectBefore
After
Inlining these internal generics is safe: the generator already skips emitting them as standalone definitions - even if you'd request
type: "OverrideProperties"
- so this change only cleans the schema output without dropping any functionality.