Deprecate CommandNames in favor of protocol.CommandTypes, direct import for better bundler output#52208
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| host.checkTimeoutQueueLength(0); | ||
| session.executeCommandSeq<ts.server.protocol.GeterrForProjectRequest>({ | ||
| command: ts.server.CommandNames.Geterr, | ||
| command: ts.server.protocol.CommandTypes.GeterrForProject, |
There was a problem hiding this comment.
Here's one bug caught by this PR.
| ts.server.CommandNames.ApplyChangedToOpenFiles, | ||
| ts.server.CommandNames.EncodedSemanticClassificationsFull, | ||
| ts.server.CommandNames.Cleanup, | ||
| ts.server.CommandNames.OutliningSpans, |
There was a problem hiding this comment.
Here's another caught bug; this should be GetOutliningSpans.
| ts.server.protocol.CommandTypes.GetOutliningSpans, | ||
| ts.server.protocol.CommandTypes.GetOutliningSpansFull, |
There was a problem hiding this comment.
These are the new fixed ones. But, it's weird to me that we encode this huge list rather than just iterating over the enum; seems trivial to do it that way instead and never have this drift or be wrong.
There was a problem hiding this comment.
Yeah, this "just works":
const allCommandNames: ts.server.protocol.CommandTypes[] = Object.values((ts.server.protocol as any).CommandTypes);I'm curious if this would be a better idea in the long term.
There was a problem hiding this comment.
@sheetalkamat Since you're back, do you have any opinion one way or another here? I think the baselines are enough to indicate that something changed, so I think just using values here is probably going to be better in the long term to not forget than the comments.
There was a problem hiding this comment.
Well, the above isn't needed for the beta; I think I'll merge this now and send a followup PR for discussion instead, that way the deprecation is seen.
| /** @deprecated use ts.server.protocol.CommandTypes */ | ||
| export type CommandNames = protocol.CommandTypes; | ||
| /** @deprecated use ts.server.protocol.CommandTypes */ | ||
| export const CommandNames = (protocol as any).CommandTypes; |
There was a problem hiding this comment.
I wonder if these could have just been declared as import CommandNames = protocol.Commandtypes in the first place. It probably would preserve the enum inlining issue you commented on, but at this point, I wouldn't worry about it.
There was a problem hiding this comment.
That's true, this could be export import CommandNames = protocol.CommandTypes; I'll have to see if it bundles alright as that does help a bit.
But, I do think that if we're switching to direct imports, no reason not to use the real deal.
There was a problem hiding this comment.
Hah, so this is syntax I never bothered to implement in the dts bundler since I removed all instances during the conversion. Oops.
DanielRosenwasser
left a comment
DanielRosenwasser
left a comment
There was a problem hiding this comment.
Seems like you fixed the mismatches correctly given the surrounding code
4 participants
In helping make a test for #50161 and trying to statically type the request handlers for #49929, I noticed that
CommandNamesisanyand turns out to not have any completions, checking, etc, even though it is written astype CommandNames = protocol.CommandTypes.The way that it's forwarded also makes it suboptimal in the output bundle; in the output, uses are always an explicit access to a
CommandNamesobject rather than inlined constants.This PR switches all uses of
CommandNamestoCommandTypes, then does a selective direct import of the protocol module in order to improve esbuild's code gen (it has troubles looking past one level of reexports; evanw/esbuild#2636).In doing so, this exposed a couple of cases where the use of CommandNames was wrong! For example, it's not
OutliningSpans, it'sGetOutliningSpansandGetOutliningSpansFull. There was also a unit test with a baseline that usedGeterrinstead ofGeterrForProject.Partially for #51443.