-
Notifications
You must be signed in to change notification settings - Fork 13.1k
switch enums in protocol to unions of literal types #11651
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
Conversation
| if (extraDeclarations) { | ||
| protocolDts += extraDeclarations; | ||
| } | ||
| protocolDts += "\nimport protocol = ts.server.protocol;"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export as namespace protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
src/server/editorServices.ts
Outdated
| } | ||
|
|
||
| export function convertCompilerOptions(protocolOptions: protocol.ExternalProjectCompilerOptions): CompilerOptions & protocol.CompileOnSaveMixin { | ||
| protocolOptions.target = convertCompilerOptionEnum(targetCommandLineOption, protocolOptions.target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do this in a more generic way? we have the types for the options as a whole, so we can just go through them once and build a map of things we want to convert, then do this on the fly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that I understand what you mean and how it will be beneficial. We already reuse maps that we've populated for compiler options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now if i add a new compiler option, i need to add it to commandline-parser and here as well. can i do it in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
switch enums in protocol to unions of literal types
|
|
||
| const compilerOptionConverters = prepareConvertersForEnumLikeCompilerOptions(optionDeclarations); | ||
| const indentStyle = createMap({ | ||
| "none": IndentStyle.None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use the lowercase names here and call protocolOptions.indentStyle.toLowerCase() later? Why not just use "None" etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we cannot guarantee that all callers will use proper casing for these values. It can be enforced for .ts sources but other consumers cannot be checked so we try to be tolerant
// cc @dbaeumer, @mhegazy
This PR:
protocol.d.tsto modulenow generated
protocol.d.tslooks like this