-
Notifications
You must be signed in to change notification settings - Fork 240
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
Comments
We are generating the relevant enum type ( devtools-protocol/types/protocol.d.ts Lines 7954 to 7959 in ea8402f
@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? |
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 😄 |
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? |
Hey folks, was there any further discussion on this topic? |
Hey folks! I'm still fishing for updates here 😆 I have to jump through weird hoops to work around this problem right now: |
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:
I'm generating code from the JSON definitions, but I'm facing 2 problems with this at the moment:
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 beMouseEventType
. This problem would affect any language that doesn't support string union types (in my case, Kotlin).dispatchMouseEvent
andemulateTouchFromMouseEvent
), 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.
The text was updated successfully, but these errors were encountered: