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

Add JSON schema generation. #358

Merged
merged 12 commits into from
Apr 20, 2024
Merged

Add JSON schema generation. #358

merged 12 commits into from
Apr 20, 2024

Conversation

jheer
Copy link
Member

@jheer jheer commented Apr 6, 2024

This PR uses ts-json-schema-generator to automatically generate a JSON schema definition:

  • Generate JSON Schema from TypeScript types
  • Update tests to include schema validation checks

@jheer
Copy link
Member Author

jheer commented Apr 6, 2024

Schema generation currently fails (see below)... @domoritz please let me know if you can diagnose or have ideas on how to fix!

The Mosaic spec typings make use of intersection types. I don't see those explicitly included as a ts-json-schema-generator feature, so might those be an issue?

> ts-json-schema-generator -f tsconfig.json -p src/spec/Spec.ts -t Spec --no-type-check --no-ref-encode > dist/mosaic-schema.json

/mosaic/node_modules/ts-json-schema-generator/dist/ts-json-schema-generator.js:99
        throw error;
        ^

TypeError: Cannot read properties of undefined (reading 'kind')
    at Object.isEnumDeclaration (/mosaic/node_modules/ts-json-schema-generator/node_modules/typescript/lib/typescript.js:26675:17)
    at TypeofNodeParser.createType (/mosaic/node_modules/ts-json-schema-generator/dist/src/NodeParser/TypeofNodeParser.js:28:34)
    at ChainNodeParser.createType (/mosaic/node_modules/ts-json-schema-generator/dist/src/ChainNodeParser.js:28:54)
    at /mosaic/node_modules/ts-json-schema-generator/dist/src/NodeParser/IntersectionNodeParser.js:24:72
    at Array.map (<anonymous>)
    at IntersectionNodeParser.createType (/mosaic/node_modules/ts-json-schema-generator/dist/src/NodeParser/IntersectionNodeParser.js:24:34)
    at ChainNodeParser.createType (/mosaic/node_modules/ts-json-schema-generator/dist/src/ChainNodeParser.js:28:54)
    at AnnotatedNodeParser.createType (/mosaic/node_modules/ts-json-schema-generator/dist/src/NodeParser/AnnotatedNodeParser.js:30:47)
    at /mosaic/node_modules/ts-json-schema-generator/dist/src/NodeParser/InterfaceAndClassNodeParser.js:103:134
    at Array.map (<anonymous>)

@jheer jheer requested a review from domoritz April 6, 2024 21:01
@domoritz
Copy link
Member

domoritz commented Apr 7, 2024

It looks like there are two unhandled cases. First, Lowercase and second is CSSStyleDeclaration.

The first one took me a bit to find but it's related to the comment on the type we pass to Lowercase. Lowercase is an intrinsic type which I hadn't have to deal much with before. See vega/ts-json-schema-generator#1907.

@domoritz
Copy link
Member

domoritz commented Apr 8, 2024

CSSStyleDeclaration is a super complicated type that through parentRule goes to Window and globalThis which the schema generator chokes on. Let me see whether I can convince it to not crash.

Screenshot 2024-04-08 at 11 14 18

@jheer
Copy link
Member Author

jheer commented Apr 9, 2024

CSSStyleDeclaration is a super complicated type that through parentRule goes to Window and globalThis which the schema generator chokes on. Let me see whether I can convince it to not crash.
Screenshot 2024-04-08 at 11 14 18

Hmm, maybe we can simplify the typings then? I think all we need for the Mosaic JSON spec is an object that has valid CSS property names as keys and their corresponding values. We don't need methods or other DOM bits.

If we use something like Omit<CSSStyleDeclaration, unneededProps> will the generator work, or will it still follow the path that causes it to crash?

Alternatively, it looks we can copy that (large) collection of properties from TS libs if needed: https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts ...though it would be nice to not have to maintain our own list!

@domoritz
Copy link
Member

domoritz commented Apr 9, 2024

I got it working locally but it generates hundreds of thousands of lines for just that type. I had to leave to see the eclipse but will finish up the fix soon and then we can evaluate what makes sense here.

@domoritz
Copy link
Member

I made some significant updates to the schema generator so it handles functions more gracefully. It now compiles the schema for mosaic. I haven't looked into whether it's correct but at least it compiles. And yes, Omit<CSSStyleDeclaration, unneededProps> should work well now.

@jheer
Copy link
Member Author

jheer commented Apr 15, 2024

Thanks @domoritz! I added omitted properties for the CSSStyleDeclaration and that cleans up the result and reduces the size of the output tremendously. For the most part, the generated schema is looking good.

Remaining Issues:

  • We follow Plot's lead and sometimes include (value & Record<never, never>) as part of the types, which is added as a workaround for TypeScript autocomplete issues (Literal String Union Autocomplete microsoft/TypeScript#29729). The corresponding JSON Schema has weird entries when this usage coincides with string template values, resulting in a long list of empty objects. In particular, see the output for LiteralTimeInterval, and ReducePercentile. If I remove the & Record<never, never> bits, the resulting schema looks reasonable. Maybe the generator can recognize and ignore this particular intersection type pattern?

  • It looks like the handling of intersection types can result in significant schema duplication. For example, the top-level Spec repeats all of the component properties rather than referencing prior definitions. The current duplication should not effect correctness, but does result in untidy and less interpretable schema output. Instead, I'd expect to see something like Spec: { allOf: [ {$ref: SpecHead}, {$ref: Component} ] }.

@domoritz
Copy link
Member

Preserving string literals would be nice. I filed vega/ts-json-schema-generator#1921. There were some related asks around this earlier. We already seem to handle export type MyType = "foo" | "bar" | (string & Record<never, never>); correctly (as in treat it as string). I need to see why LiteralTimeInterval, and ReducePercentile don't look nice when the isolated case works.

allOf is not the same as intersection types unfortunately (e.g. vega/ts-json-schema-generator#62 (comment)) if we treat the semantics strictly. This is a router one to improve upon until JSON schema has semantics that match TS better.

"lint": "eslint src test",
"schema": "ts-json-schema-generator -f tsconfig.json -p src/spec/Spec.ts -t Spec --no-type-check --no-ref-encode --functions hide > dist/mosaic-schema.json",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this fails on a fresh clone of the repo bc the dist/ directory doesn't exist

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yeah, this is still WIP, but also schema is not meant to typically be run in isolation; it's now tied in to both the prebuild and pretest hooks.

@domoritz domoritz self-requested a review April 19, 2024 02:41
@domoritz
Copy link
Member

LiteralTimeInterval is tricky because it has templates with numbers and strings. I don't think these can be represented in JSON schema right now.

Screenshot 2024-04-19 at 21 47 47

Same with ReducerPercentile. I can improve the schema generator not to generate a ton of these, though

{
          "properties": {},
          "type": "object"
        }

@domoritz
Copy link
Member

I tested with the latest from vega/ts-json-schema-generator#1927 just now.

@domoritz
Copy link
Member

domoritz commented Apr 20, 2024

Ah, there was a bug that I fixed: vega/ts-json-schema-generator#1929. I'll make a release and update here.

@domoritz domoritz marked this pull request as ready for review April 20, 2024 02:45
@jheer jheer merged commit ae7bca6 into main Apr 20, 2024
2 checks passed
@jheer jheer deleted the jh/json-schema branch April 20, 2024 17:11
@domoritz domoritz mentioned this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants