-
Notifications
You must be signed in to change notification settings - Fork 214
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?
Changes from 9 commits
c7c1baa
7c1ebba
d6bee00
434d7b5
c3b2c94
45a530b
1622f18
095a8eb
dcf9389
45ab994
705f1a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,6 +6,10 @@ | |||||
import type { ReferenceType } from "./Type/ReferenceType.js"; | ||||||
import { hasJsDocTag } from "./Utils/hasJsDocTag.js"; | ||||||
import { symbolAtNode } from "./Utils/symbolAtNode.js"; | ||||||
import { AliasType } from "./Type/AliasType.js"; | ||||||
import { derefAliasedType, isDeepLiteralUnion } from "./Utils/derefType.js"; | ||||||
import { ObjectType } from "./Type/ObjectType.js"; | ||||||
import { IntersectionType } from "./Type/IntersectionType.js"; | ||||||
|
||||||
export class ExposeNodeParser implements SubNodeParser { | ||||||
public constructor( | ||||||
|
@@ -22,7 +26,7 @@ | |||||
public createType(node: ts.Node, context: Context, reference?: ReferenceType): BaseType { | ||||||
const baseType = this.subNodeParser.createType(node, context, reference); | ||||||
|
||||||
if (!this.isExportNode(node)) { | ||||||
if (!this.isExportNode(node) || this.isFromLib(node) || this.shouldInline(node, baseType, context)) { | ||||||
return baseType; | ||||||
} | ||||||
|
||||||
|
@@ -38,7 +42,7 @@ | |||||
return false; | ||||||
} | ||||||
|
||||||
const localSymbol: ts.Symbol = (node as any).localSymbol; | ||||||
Check warning on line 45 in src/ExposeNodeParser.ts
|
||||||
return localSymbol ? "exportSymbol" in localSymbol : false; | ||||||
} | ||||||
|
||||||
|
@@ -49,4 +53,49 @@ | |||||
|
||||||
return argumentIds.length ? `${fullName}<${argumentIds.join(",")}>` : fullName; | ||||||
} | ||||||
|
||||||
private isFromLib(node: ts.Node): boolean { | ||||||
const sourceFile = node.getSourceFile(); | ||||||
if (!sourceFile) { | ||||||
return false; | ||||||
} | ||||||
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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 And do you think it's worth exploring a way to rename such definitions to a clearer name like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
if (!ts.isTypeAliasDeclaration(node)) { | ||||||
return false; | ||||||
} | ||||||
if (!(type instanceof AliasType)) { | ||||||
return false; | ||||||
} | ||||||
if (!node.typeParameters?.length) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, with this, some other tests fail since |
||||||
return false; | ||||||
} | ||||||
|
||||||
const localSymbol: ts.Symbol = (node as any).localSymbol; | ||||||
Check warning on line 76 in src/ExposeNodeParser.ts
|
||||||
const isExported = localSymbol ? "exportSymbol" in localSymbol : false; | ||||||
|
||||||
const actual = derefAliasedType(type.getType()); | ||||||
const hasStructuralArg = context | ||||||
.getArguments() | ||||||
.some((arg) => /(structure|object|alias|def-alias)-/.test(arg?.getName() ?? "")); | ||||||
|
||||||
if (isExported && !hasStructuralArg) { | ||||||
return false; | ||||||
} | ||||||
|
||||||
if (isDeepLiteralUnion(actual)) { | ||||||
return true; | ||||||
} | ||||||
|
||||||
if (!isExported && (actual instanceof ObjectType || actual instanceof IntersectionType)) { | ||||||
return true; | ||||||
} | ||||||
|
||||||
if (hasStructuralArg) { | ||||||
return true; | ||||||
} | ||||||
return false; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { OverrideProperties } from "./util"; | ||
|
||
export type Base = { | ||
foo: string; | ||
bar: number; | ||
}; | ||
|
||
export type MyType = OverrideProperties< | ||
Base, | ||
{ | ||
bar: string; | ||
} | ||
>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
{ | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"definitions": { | ||
"Base": { | ||
"additionalProperties": false, | ||
"properties": { | ||
"bar": { | ||
"type": "number" | ||
}, | ||
"foo": { | ||
"type": "string" | ||
} | ||
}, | ||
"required": [ | ||
"foo", | ||
"bar" | ||
], | ||
"type": "object" | ||
}, | ||
"MyType": { | ||
"additionalProperties": false, | ||
"properties": { | ||
"bar": { | ||
"type": "string" | ||
}, | ||
"foo": { | ||
"type": "string" | ||
} | ||
}, | ||
"required": [ | ||
"bar", | ||
"foo" | ||
], | ||
"type": "object" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// types are from https://github.com/sindresorhus/type-fest | ||
|
||
export type Simplify<T> = { [KeyType in keyof T]: T[KeyType] } & {}; | ||
|
||
export type PickIndexSignature<ObjectType> = { | ||
[KeyType in keyof ObjectType as {} extends Record<KeyType, unknown> ? KeyType : never]: ObjectType[KeyType]; | ||
}; | ||
|
||
export type OmitIndexSignature<ObjectType> = { | ||
[KeyType in keyof ObjectType as {} extends Record<KeyType, unknown> ? never : KeyType]: ObjectType[KeyType]; | ||
}; | ||
|
||
type SimpleMerge<Destination, Source> = { | ||
[Key in keyof Destination as Key extends keyof Source ? never : Key]: Destination[Key]; | ||
} & Source; | ||
|
||
export type Merge<Destination, Source> = Simplify< | ||
SimpleMerge<PickIndexSignature<Destination>, PickIndexSignature<Source>> & | ||
SimpleMerge<OmitIndexSignature<Destination>, OmitIndexSignature<Source>> | ||
>; | ||
|
||
export type OverrideProperties< | ||
TOriginal, | ||
TOverride extends Partial<Record<keyof TOriginal, unknown>> & { | ||
[Key in keyof TOverride]: Key extends keyof TOriginal ? TOverride[Key] : never; | ||
}, | ||
> = Merge<TOriginal, TOverride>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export const RuntimeObject = { | ||
FOO: "foo-val", | ||
BAR: "bar-val", | ||
} as const; | ||
|
||
export type ValueOf<T> = T[keyof T]; | ||
|
||
export type MyType = ValueOf<typeof RuntimeObject>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"$ref": "#/definitions/MyType", | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"definitions": { | ||
"MyType": { | ||
"enum": [ | ||
"foo-val", | ||
"bar-val" | ||
], | ||
"type": "string" | ||
} | ||
} | ||
} |
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
As far as I can see, yes, an example path to one of the TypeScript lib
.d.ts
files is:However, this will probably lose relevance if we implement a different approach, as @domoritz suggested above. To be able to keep definitions even with auto-generated generic names when they are reused more than once, we will need to process them after all definitions have been collected, as an additional step in
SchemaGenerator.createSchemaFromNodes
, I guess. At that point, we have no information about which file the type of a definition came from.So we would need to rely only on detecting names like
object-...
,alias-...
, and possibly identifying generics simply by observing<
in their names (if we agree to add the latter, some of the existing tests will need to update the expected schema, since now having a generic-name definition is expected, even if it's used only once, and few tests reflect it, including vega-lite).I have some draft solutions but had little time to finish them. I'll come back to this soon, but if you have any comments on what I said above, they're very welcome.
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.
You might be able to add the path information in an intermediate step before producing the final json so we don't have to rely on reverse engineering from names.