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

Nested enum types have no name #244

Open
joffrey-bion opened this issue May 16, 2021 · 4 comments
Open

Nested enum types have no name #244

joffrey-bion opened this issue May 16, 2021 · 4 comments

Comments

@joffrey-bion
Copy link

joffrey-bion commented May 16, 2021

Some domain types/commands/events in the JSON protocol definitions have properties/parameters of enum types that are not references to a top-level domain enum type, but that are instead inlined on the spot. For instance here is one:

...
{
    "name": "emulateTouchFromMouseEvent",
    "description": "Emulates touch event from the mouse event parameters.",
    "experimental": true,
    "parameters": [
        {
            "name": "type",
            "description": "Type of the mouse event.",
            "type": "string",
            "enum": [
                "mousePressed",
                "mouseReleased",
                "mouseMoved",
                "mouseWheel"
            ]
...

I'm generating code from the JSON definitions, but I'm facing 2 problems with this at the moment:

  1. because these types are not defined at the top level as "domain types", they have no name so I have to either use a plain String type (which defeats the purpose of the enum) or generate a name (which might not be user-friendly). In the example above, a good name might be MouseEventType. This problem would affect any language that doesn't support string union types (in my case, Kotlin).
  2. some of these enums are implicitily reused across multiple commands/events (the example above appears in dispatchMouseEvent and emulateTouchFromMouseEvent), but there is no way to tell whether they are different types and just happen to have the same enum values (and thus might change independently), or whether they are a single reused type (and thus will evolve together and always stay in sync). This means I cannot choose systematically between declaring different enum types or just one. This problem would affect the same set of languages I believe, basically all those who have to declare enums as a named type.

I haven't found any inlined object type like this (these are only refs), I only found enums suffering from this problem.

It would be great if all enums were extracted into the domain types list and only references appeared in the properties and parameters. Some enums already are defined at the top level, just not all of them.

@TimvdLippe
Copy link
Contributor

We are generating the relevant enum type (

export const enum DispatchMouseEventRequestType {
MousePressed = 'mousePressed',
MouseReleased = 'mouseReleased',
MouseMoved = 'mouseMoved',
MouseWheel = 'mouseWheel',
}
), but we explicitly left the original value as a string to avoid a breaking change: 08875fa

@jackfranklin I wonder, now that we do generate the enums, if we can make the breaking change? Existing users should be able to use the enum as-is and port all of their code, prior to the breaking change applying.

CC @brendankenny for the above and if that indeed is possible in LightHouse today and would be a valid migration strategy?

@TimvdLippe
Copy link
Contributor

Oh and @joffrey-bion the full discussion in #216 might also be helpful to understand why we made this particular decision. Let us know if you have any other suggestions/thoughts on how to improve it 😄

@joffrey-bion
Copy link
Author

joffrey-bion commented May 17, 2021

Hi @TimvdLippe, thanks a lot for pointing me towards this discussion and the change in TS generation. I'm sorry I missed those.

The problems I'm mentioning above could be avoided in TS by keeping unnamed string unions, so I didn't think the TS generator would have generated fully-fledged enums, but I read a little bit more and found interesting reasons for actually declaring named enums even in TS, so thanks for this!

IMO, generating names is either brittle or not very user friendly, and I believe it would be better to actually update the protocol definitions themselves rather than the code generators. That's why I came here to try and change the definitions before releasing anything with generated names.

Since name generation has already been done on TS side, it would be great to find a backwards-compatible way to transition to better definitions (with all enums as top level types with explicit names).

One option I see is that the TS generator could generate deprecated typealiases from the old generated enum names to the new names from the protocol definition. That mapping could be hardcoded for a while. This way, users of old enums would still compile (but see the deprecation), and new users would just use the new enums with nice names. After some time, these old enums' typaliases and the hardcoded mapping could be removed.

Do you think something like this could be considered?

@joffrey-bion
Copy link
Author

Hey folks, was there any further discussion on this topic?

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

No branches or pull requests

2 participants