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

Validate JSON files using protocol-schema.d.ts #211

Closed
wants to merge 1 commit into from

Conversation

themaxdavitt
Copy link

I changed the typescript compiler configuration in scripts/ to use a transformer provided by typescript-is that will let us assert that our JSON protocol files are valid compared aganst protocol-schema.

@nojvek (assuming you're the @noj mentioned in the source code)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@themaxdavitt
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@nojvek
Copy link
Contributor

nojvek commented Apr 18, 2020

Just saw this, reviewing

@@ -2,7 +2,7 @@
"version": "0.0.510771",
"license": "SEE LICENSE IN https://chromium.googlesource.com/chromium/src/+/master/LICENSE",
"scripts": {
"build-protocol-dts": "ts-node protocol-dts-generator.ts",
"build-protocol-dts": "ts-node --compiler ttypescript protocol-dts-generator.ts",
Copy link
Contributor

@nojvek nojvek Apr 18, 2020

Choose a reason for hiding this comment

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

I looked into tttypescript which seems like a fork of typescript for inline transformers, and typescript-is is the transformer plugin.

I do like the simplicity of assertType<X> but it seems like a lot of magic and dependencies that may not be maintained in the future.

I'm deffo down for upgrading TS version. Sounds like a great idea.

Since this is a static import we can use the import syntax for json and typescript will validate the types for us at compile time. When I was writing the initial code, import syntax for json wasn't available yet.

image

{
  "hello": "world",
  "foo": true,
  "bar": 0,
  "array": null
}
import testJson from './test.json';

interface Test {
  hello: string;
  foo: boolean;
  br: number;
}

const test: Test = testJson;

^ typescript will fail compilation

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of the JSON import syntax! That seems a lot cleaner. :)

I did end up getting errors after I tried implementing it though:

protocol-dts-generator.ts(7,7): error TS2322: Type '{ version: { major: string; minor: string; }; domains: ({ domain: string; description: string; deprecated: boolean; dependencies: string[]; types: { id: string; description: string; type: string; properties: ({ ...; } | ... 1 more ... | { ...; })[]; }[]; commands: { ...; }[]; events: { ...; }[]; experimental?: undef...' is not assignable to type 'IProtocol'.
  Types of property 'domains' are incompatible.
    Type '({ domain: string; description: string; deprecated: boolean; dependencies: string[]; types: { id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { ...; } | { ...; })[]; }[]; commands: { ...; }[]; events: { ...; }[];...' is not assignable to type 'Domain[]'.
      Type '{ domain: string; description: string; deprecated: boolean; dependencies: string[]; types: { id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { ...; } | { ...; })[]; }[]; commands: { ...; }[]; events: { ...; }[]; ...' is not assignable to type 'Domain'.
        Type '{ domain: string; description: string; deprecated: boolean; dependencies: string[]; types: { id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { ...; } | { ...; })[]; }[]; commands: { ...; }[]; events: { ...; }[]; ...' is not assignable to type 'Domain'.
          Types of property 'types' are incompatible.
            Type '{ id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { name: string; description: string; type: string; enum?: undefined; optional?: undefined; } | { ...; })[]; }[]' is not assignable to type 'DomainType[]'.
              Type '{ id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { name: string; description: string; type: string; enum?: undefined; optional?: undefined; } | { ...; })[]; }' is not assignable to type 'DomainType'.
                Type '{ id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { name: string; description: string; type: string; enum?: undefined; optional?: undefined; } | { ...; })[]; }' is not assignable to type '{ id: string; description?: string | undefined; } & ObjectType'.
                  Type '{ id: string; description: string; type: string; properties: ({ name: string; description: string; type: string; enum: string[]; optional?: undefined; } | { name: string; description: string; type: string; enum?: undefined; optional?: undefined; } | { ...; })[]; }' is not assignable to type 'ObjectType'.
                    Types of property 'type' are incompatible.
                      Type 'string' is not assignable to type '"object"'.
protocol-dts-generator.ts(8,7): error TS2322: Type '{ version: { major: string; minor: string; }; domains: ({ domain: string; experimental: boolean; dependencies: string[]; types: ({ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | ... 4 more ... | { ...; })[]; commands: ({ ...; } | ... 1 more ... | { ...; })[]; events?: un...' is not assignable to type 'IProtocol'.
  Types of property 'domains' are incompatible.
    Type '({ domain: string; experimental: boolean; dependencies: string[]; types: ({ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | { id: string; description: string; type: string; enum: string[]; properties?: undefined; } | { ...; } | { ...; } | { ...; } | { ...; })[]; commands:...' is not assignable to type 'Domain[]'.
      Type '{ domain: string; experimental: boolean; dependencies: string[]; types: ({ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | { id: string; description: string; type: string; enum: string[]; properties?: undefined; } | { ...; } | { ...; } | { ...; } | { ...; })[]; commands: ...' is not assignable to type 'Domain'.
        Type '{ domain: string; experimental: boolean; dependencies: string[]; types: ({ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | { id: string; description: string; type: string; enum: string[]; properties?: undefined; } | { ...; } | { ...; } | { ...; } | { ...; })[]; commands: ...' is not assignable to type 'Domain'.
          Types of property 'types' are incompatible.
            Type '({ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | { id: string; description: string; type: string; enum: string[]; properties?: undefined; } | { id: string; description: string; type: string; properties: ({ ...; } | ... 1 more ... | { ...; })[]; enum?: undefined; } | { ....' is not assignable to type 'DomainType[]'.
              Type '{ id: string; description: string; type: string; enum?: undefined; properties?: undefined; } | { id: string; description: string; type: string; enum: string[]; properties?: undefined; } | { id: string; description: string; type: string; properties: ({ ...; } | ... 1 more ... | { ...; })[]; enum?: undefined; } | { .....' is not assignable to type 'DomainType'.
                Type '{ id: string; description: string; type: string; enum?: undefined; properties?: undefined; }' is not assignable to type 'DomainType'.
                  Type '{ id: string; description: string; type: string; enum?: undefined; properties?: undefined; }' is not assignable to type '{ id: string; description?: string | undefined; } & ObjectType'.
                    Type '{ id: string; description: string; type: string; enum?: undefined; properties?: undefined; }' is not assignable to type 'ObjectType'.
                      Types of property 'type' are incompatible.
                        Type 'string' is not assignable to type '"object"'.

If I'm interpreting this right, then when importing the JSON it's using types that are too wide (e.g. inferred: type: string) for the literal types we're specifying (actual: type: "object" in ObjectType). It doesn't look like we have a good way to resolve that - see: microsoft/TypeScript#26552.

What do you propose we do?

@nojvek
Copy link
Contributor

nojvek commented Apr 18, 2020

Made a trip back to memory lane. I remembered how I got over the issue last time before import json syntax was supported.

https://github.com/nojvek/chrome-remote-debug-protocol/blob/master/src/protocolDef/browser_protocol.ts

This was original code ^ before it was ported here. downloader.js made a .ts file that inlined the json to a specific type. This was before import syntax was supported. You are right in that import syntax doesn’t import string literals and that is a PITA. Because inline json object is always typed to a const it works.

https://github.com/nojvek/chrome-remote-debug-protocol/blob/master/download_protocols.js#L5

Let me do some pondering to see if there is a way to use node -r to write a small json import transformer.

I do think typescript should export their transformers as part of ts config. Would make a lot of this much easier.

I’m not opposed to typescript-is. Ttypescript is where I’m not fully sold on.

@themaxdavitt
Copy link
Author

Yeah, it is strange that they only allow you to use transformers programmatically, and I completely understand where you're coming from with ttypescript, it along with typescript-is just seemed like the easiest way to implement this. If we're stuck with them, we could just pin the versions of our typescript dependencies to avoid future headaches from version mismatches (e.g. by using a package-lock.json file or changing the versions specified, like "ttypescript": "^1.5.10" => "ttypescript": "1.5.10"). Not a very good solution though.

Also, I tried creating a "bootstrap" file like this but it caused typescript-is to complain about how the transformer shouldn't be used in runtime:

require('ts-node').register({
    transformers: 'typescript-is/lib/transform-inline/transformer',
    transpileOnly: false,
});
require('./protocol-dts-generator.ts');

I hope you find something that works! I'll keep trying stuff too. :)

Regardless, once we find a nice way to handle this, I think we should also consider exporting the types in protocol-schema since we're asserting that the types are consistent with the JSON representations of the protocol anyways. What do you think? It should only require us to move it to types/ and change our import path in protocol-dts-generator to match AFAIK.


BTW, I'm giving you write access to themaxdavitt/devtools-protocol. Feel free to push any changes you see fit to the master branch and they'll be reflected in this PR. Alternatively, I can just pull commits from a repo you own, I don't mind.

@nojvek
Copy link
Contributor

nojvek commented Apr 18, 2020 via email

@themaxdavitt
Copy link
Author

Closing in favor of #212.

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.

None yet

3 participants