Pull out inline enums as types in protocol.d.ts

[jackfranklin]

Note: this change is taken directly from this DevTools CL (we should
explore getting DevTools to depend on this module to avoid the
duplication): https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2113374. I'm happy to take that on.

This PR takes the work Simon did from DevTools into this repo - although it's me committing this is all his work!

This change is motivated by moving Puppeteer to TypeScript and wanting
to control types more strictly rather than declaring arguments as
strings even though really only a subset are supported.

I've copied the commit message from the above here:

Commands, Events and Object types can declare "inline enums" to
restrict the possible values of a 'string' field.

Example field:

  referrerPolicy: ('unsafe-url'|'...'|'...')

To enable type-checking with TypeScript and stay compatible with
existing code, we now generate explicit enums.
for the enum names is adapted from code_generator_frontend.py
and needs to always match.

Example generated enum for the above code:

  export enum RequestReferrerPolicy {
    UnsafeUrl = 'unsafe-url',
    NoReferrerWhenDowngrade = 'no-referrer-when-downgrade',
    NoReferrer = 'no-referrer',
    Origin = 'origin',
    OriginWhenCrossOrigin = 'origin-when-cross-origin',
    SameOrigin = 'same-origin',
    StrictOrigin = 'strict-origin',
    StrictOriginWhenCrossOrigin = 'strict-origin-when-cross-origin',
  }

[TimvdLippe]

Change LGTM. Before merging, can we verify that existing users of these types are not broken? Given that TS is structurally typed, I don't expect breakages, but I would like to double-check. If it does turn out to be a breaking change, we would need to up the major version number.

[jackfranklin]

I'm not sure how best to check across everything. We know this is OK in Devtools as we use it there, and I don't think it was a breaking change there.

Maybe we could ask someone from Lighthouse (@connorjclark ?) to have a look at if this causes issues there.

[TimvdLippe]

I think a simple check would be to create a dummy project and npm i this package. Then use a couple of types and use the literal values (e.g. 'xml'). Then copy your .d.ts file from this PR into your local project and see if it starts complaining or not.

[jackfranklin]

This is a breaking change. I took this code:

import { Protocol } from 'devtools-protocol'

const message: Protocol.Console.ConsoleMessage = {
  source: 'xml',
  level: 'log',
  text: 'foo'
}

Which typechecks with the master branch of this repo. But with this PR it fails:

index.ts(4,3): error TS2322: Type '"xml"' is not assignable to type 'ConsoleMessageSource'.
index.ts(5,3): error TS2322: Type '"log"' is not assignable to type 'ConsoleMessageLevel'.

I think we have two options:

1. Accept this is a breaking change and bump the version accordingly.
2. Consider using union types, e.g. `type ConsoleMessageSource = 'xml' | '...' and then we wouldn't get the error we get above. There are pros and cons to using these over enums that we should consider.

[brendankenny]

I'm not sure how best to check across everything. We know this is OK in Devtools as we use it there, and I don't think it was a breaking change there.

This does break Lighthouse, though only in a few places. I'm actually not entirely sure why it only does that and doesn't instead either break nothing or break all over the place. I'll try to figure that out :)

For future reference, testing is fairly easy:

git clone [email protected]:GoogleChrome/lighthouse.git && cd lighthouse
yarn add -D https://github.com/jackfranklin/devtools-protocol#2bb3bb0
yarn type-check

(sorry for the yarn requirement, but for some reason npm is failing on my machine for any variation on npm i -D github:jackfranklin/devtools-protocol#2bb3bb0)

[brendankenny]

Oh, I was already too late :) That's exactly the issue for Lighthouse code.

Consider using union types, e.g. `type ConsoleMessageSource = 'xml' | '...' and then we wouldn't get the error we get above. There are pros and cons to using these over enums that we should consider.

this would be ideal for us. I was actually curious about the need to move to enums in particular since string unions also meet the need to "restrict the possible values of a 'string' field". Moving to named unions would be a transparent change that I assume would meet DevTools' needs?

[jackfranklin]

In Puppeteer we've found the enum useful because we can also write JS to error if a user doesn't supply a correct value - e.g. like this: puppeteer/puppeteer@7eab7f8#diff-73da6513596563a004bf179729e919e1R131

It is useful to be able to refer to something as MyEnum.value and does add some clarity to code so I'm broadly in favour of using enums more than unions but that becomes a trade-off against breaking changes and then maybe it's not worth it?

I'm not sure in DevTools frontend what the motivation was for Enums over Union types - @TimvdLippe do you know? Or @szuend maybe?

[TimvdLippe]

For DevTools, we needed references to these values in our type annotations. E.g. we have a function that accepts a particular enum value and we wanted to type it as-is. Before it, it would be solely string. The type reference requires a separate type to exist and thus necessitates the separate enum.

I think the solution of the union of the enum and the explicit value is a good first step. We could put that in a patch release and then announce to users they should update their code over time to use the enum. Then in a later major version, we can remove the explicit enum values from the inline enum.

[brendankenny]

In Puppeteer we've found the enum useful because we can also write JS to error if a user doesn't supply a correct value - e.g. like this: puppeteer/puppeteer@7eab7f8#diff-73da6513596563a004bf179729e919e1R131

ah, yeah, enums are enumerable in ts but are just types (not values) in js :/

I'm surprised the

await this._client.send('Emulation.setEmulatedVisionDeficiency', {
  type: type || 'none',
});

lines don't run into the same issue as you mentioned in #216 (comment), though? Simplified version in the ts playground.

[jackfranklin]

@brendankenny good catch, I would expect that to fail in Puppeteer...I'll dig into that!

Edit: it's because the typedef of that type in the params is the union type of 'none' | '...' so TS is happy. If this was typed under the hood as an enum, then you're right that that line would cause a compile error.

[jackfranklin]

think the solution of the union of the enum and the explicit value is a good first step

@TimvdLippe do you mean that we should try to make it so we support either the enum form or a regular string and take either?

[TimvdLippe]

Yes basically option 2 as you described it. That would not break existing users, allows users to use the enum right away and then in a major version upgrade we allow ourselves to remove the non-enum version. This should give our users the time to gradually upgrade, rather than a single big-bang upgrade to enums.

[jackfranklin]

I don't think I was clear, my option two was to not use enums and only use union types. I hadn't considered that we could do both. I'm trying to figure out if we can even express that with TypeScript...will prototype.

[jackfranklin]

So I think we could generate TS that looks like this:

        export enum ConsoleMessageSourceEnum {
            XML = 'xml',
            Javascript = 'javascript',
            Network = 'network',
            ConsoleAPI = 'console-api',
            Storage = 'storage',
            Appcache = 'appcache',
            Rendering = 'rendering',
            Security = 'security',
            Other = 'other',
            Deprecation = 'deprecation',
            Worker = 'worker',
        }

        export type ConsoleMessageSourceUnion = 'xml'| 'javascript'| 'network'| 'console-api'| 'storage'| 'appcache'| 'rendering'| 'security'| 'other'| 'deprecation'| 'worker'

        /**
         * Console message.
         */
        export interface ConsoleMessage {
            /**
             * Message source.
             */
            source: ConsoleMessageSourceEnum | ConsoleMessageSourceUnion;

Which might work here ? And we plan long term to remove the union support.

@brendankenny what are your thoughts?

[TimvdLippe]

Oh right. Yes I mis read. Basically I was thinking doing this: Playground Link

[connor4312]

Because this is purely a .d.ts I think these should be declared as const enum so that they're compiled as strings in the code and not referenced concretely.

[jackfranklin]

@connor4312 in the case of Puppeteer though we might want to reference them concretely - e.g. puppeteer/puppeteer@7eab7f8#diff-73da6513596563a004bf179729e919e1R131 - so I think not using const enum is what we need?

I'm by no means an expert on TS enums and the variants so I may well be way off here :D

[connor4312]

With a normal enum, when you reference it in code like Foo.Bar, TS will assume there's JavaScript backing it up and try to import the corresponding compiled code at runtime. This works in puppeteer since you actually compile from .ts, which generates compiled code for the normal enum. But since this module ships only .d.ts, such an import will fail. Using a const enum, the enum value is replaced in the code at compile time. Repl.it example: https://repl.it/@ConnorPeet/RoastedAcrobaticRegister

[brendankenny]

So I think we could generate TS that looks like this:
...
Which might work here ? And we plan long term to remove the union support.

@brendankenny what are your thoughts?

I believe the enum/string-literal union would work for Lighthouse, but a long term plan to get rid of the string literal union would still break things. e.g. there's no javascript value that will satisfy a parameter or property of type ConsoleMessageSourceEnum.

I wonder if it would be possible instead to

• create the enums for their usability as types and values in typescript code
• but keep the string literal union as the type of the protocol types since enum values are assignable to string literals. e.g.

enum ConsoleMessageLevel {
  Log = 'log',
  Warning = 'warning',
}
const lvl1: ConsoleMessageLevel.Log = 'log'; // Error: Type '"log"' is not assignable to type 'ConsoleMessageLevel.Log'.
const lvl2: 'log' = ConsoleMessageLevel.Log; // Good to go.

and everything is still strictly type checked.

Here is @TimvdLippe's playground example but with the enums removed from the source and level properties. Types are still checked on message and message2 whether assigned a string literal or an enum value, and the compiled output still has the enums as real JS values (so Object.keys() etc will still work).

[jackfranklin]

Nice, I like that plan. Additionally if we take @connor4312's advice (thank you!) and make them const enum then the output is much cleaner too. We do lose the ability to iterate over them but given they are in a .d.ts file we wouldn't get that ability anyway.

So I think we'd end up with something like this:

https://www.typescriptlang.org/play/#code/MYewdgzgLgBApmArgWxgYXBEAbOBZOCCAQwHM4BlERAJ2DhgG8AoGGADTwBkYBeGAOQAPZNgEAaVjABSxAG7EIwGgEsADrH4CAVvMXL1UCVIBycKAHcQNANZ9BYc1dvG2GSDjgBBAAoBJewFQD1wAWmI1FVcYCihrMgYtaHjyaK81NWBiYAALRMEIzOy86IAlBAATOFUwUkCaSuqVWuiKOGBaFSgAT0CIds6e6IB5KDyaQJAx6uiAETg1BqyoFXBAqsX24hXwaIB1axtqwOcjmmMAX2ZmYOh4JFR3LFwCIgSuODk4bCYpLhA6lpsAD9sQaGBmoDBBYwRCWpI2ABRGg0ayBaqo84ImDzABGiChAiq+NI0T8YAAZiBAs0qZdrs0oNUKdkGE9PK8SORfmwAPQAKn5Uhg-JgnISMCwtHoADphfzeVIpXQ4AAuGAACmEogEMAAPoJdAolKoNLqDQJHJZDubBLdPOFIraBMkaAlnYUsrk4M6GmAqjVSfrBP0OqohsGBFNxs6NkttqswM7TjMAJQAbikAqFbBFYsIXIY-S+4e6ctzCqkuC+2HVWuBQYtMPBkOdGOssbgJOdtJAAgzUiZQig6uggczV1zU5gN0wsGQBYS6vZL0X3P4LDYyvo6u1Ymx1e+u4b0SHI8EVL7zCus8g87XcAATMvMByH-ZN5LqCqXyF8A+qGlOAZU4LgD0+I90FfVc3nID4axlf5SGxM9d0vARryAA

@TimvdLippe what do you think? This feels like a good middle ground that avoids a breaking change.

[szuend]

The reason we introduced it in the first place, is because the enums types are currently used in the existing DevTools JSDoc. Assuming we don't want to diverge the generated .d.ts in DevTools and puppeteer, we need to make sure that whatever we come up with, works with the existing DevTools code (meaning we need to make closure happy).

[TimvdLippe]

@jackfranklin LGTM!

[jackfranklin]

@szuend that's a good point. I'd be happy to maintain our own version in DevTools frontend until such time where Closure is less of an issue, but we are talking a long time frame. @TimvdLippe WDYT?

My hunch is that this approach would work with Closure because we still have the enum types that we can reference in Closure land but I'm not 100%.

@brendankenny are you happy with the code as it is in that TypeScript Playground link above?

[TimvdLippe]

Yes we are a far way off to getting rid of Closure. Once we are in TS-only land, I want to use this package and remove our custom integration.

The only thing we need to figure out is the Dispatcher work. E.g. the on('event-name') vs an interface with properly defined methods. That would be a breaking change here, so I am not sure what the best solution is. Let's defer that to when we are in a position to make that change.

[brendankenny]

@brendankenny are you happy with the code as it is in that TypeScript Playground link above?

Yes, LGTM!

[jackfranklin]

@TimvdLippe please could you have a look at the latest commit?

[TimvdLippe]

LGTM

[jackfranklin]

@brendankenny do you know how we go about publishing this change on npm? Thanks :)

[TimvdLippe]

This will be published automatically by a script. So it is soonTM.