Make sortText on completions be enum-typed

[uniqueiniquity]

We've had a couple issues in the past where the value of a member of the SortText enum has changed when adding a new member. This has caused bugs in editors that use TSServer.

Here, we ensure that the sortText property of completion entries is specifically typed with the SortText enum, so that it's properly represented in the protocol.

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[amcasey]

Why are some "member" and others "members"?

[weswigham]

So if VS relies on specific string-numbers for the sort text, does that mean that VS/vscode couldn't handle LS plugins that provide arbitrary alphanumeric content for the sortText? I always assumed the content was an arbitrary relative "string to sort by".

[weswigham]

For reference: The angular language service plugin definitely passes full on arbitrary strings. The sort they map into sortText is the full alphanumeric name of a thing. I imagine that's currently fine, since they don't mix their template completions in with normal completion results?

[mjbvz]

@weswigham VS code uses string sorting logic: https://github.com/microsoft/vscode/blob/9f474febfee5330f7a9a7456862ef73d0b6f0bf5/src/vs/editor/contrib/suggest/suggest.ts#L229

So you could use 'a', 'b', 'c', ... instead of '0', '1', ...

[weswigham]

Right - all that matters is that whoever is sending the sortText is internally consistent in that case. @uniqueiniquity how is it used elsewhere? And why is it not used as an opaque sortable string in those cases? As I've pointed out - the angular LS plugin does make use of it as not-internal-numbering-scheme, so this protocol change would definitely break it.

[sheetalkamat]

agree with @weswigham here.. this needs to be string so that any string value can be passed in.. Its enum in services layer internally so that it can easily be labeled instead of making them "1" or "2"

[uniqueiniquity]

@weswigham @sheetalkamat we don't generally care about the text - we specifically care about the JavaScriptIdentifiers entries, as since they're basically guesses, we want to handle those a little differently than the others. In all other cases, interpreting it as "a string to sort by" is fine.

My goal here is to either prevent these values from being changed or, if that's not feasible, to at least have a way to get notified when they get changed. I'd also be open to an additional property saying "this entry is a js-only guess" if that's preferred.

[sheetalkamat]

I think making a property for js-only guess is better than having enum for the sorting text

[uniqueiniquity]

Superseded by #36063.