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

Expose EncodedSemanticClassificationsRequest in protocol.d.ts #42640

Merged

Conversation

orta
Copy link
Contributor

@orta orta commented Feb 4, 2021

Fixes #42587 - I think because this request used to have the same args as many other requests then it was not included in the protocol.d.ts.

Now it differs, so I've removed the internal markers 👍🏻

@orta orta self-assigned this Feb 4, 2021
@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 4, 2021
/**
* A request to get encoded semantic classifications for a span in the file
*/
interface EncodedSemanticClassificationsRequest extends FileRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren’t they in typescript/tsserver .d.ts ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently only use protocol.d.ts with VS Code. I think this should have all the types used communicating from or to tsserver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, added

@@ -868,6 +866,14 @@ namespace ts.server.protocol {
format?: "original" | "2020"
}

/** The response for a EncodedSemanticClassificationsRequest */
export interface EncodedSemanticClassificationsResponse extends Response {
body?: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull this out as EncodedSemanticClassificationsResponseBody interface

export interface EncodedSemanticClassificationsResponse extends Response {
body?: {
endOfLineState: EndOfLineState;
spans: number[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we need specific enum for classifications returned otherwise vscode would need separate list?

Copy link
Contributor Author

@orta orta Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe, the spans looks like

     * @returns a number array encoded as triples of [start, length, ClassificationType, ...].

When it's the older format, so I've included it but it's not referenced up here

@mjbvz
Copy link
Contributor

mjbvz commented Feb 23, 2021

Hi, just want to check if this is on track for TS 4.2.1. If not, could we please get this into a recovery build of TS 4.2 instead of waiting for TS 4.3? This would let us better validate the semantic highlighting support API while switching to the native TS implementation (aeschli/typescript-vscode-sh-plugin#22)

@orta
Copy link
Contributor Author

orta commented Feb 24, 2021

Just updating the baselines, I'll also submit this to the 4.2 branch 👍🏻

*/
export interface EncodedSemanticClassificationsResponseBody {
endOfLineState: EndOfLineState;
spans: ClassifiedSpan[] | number[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isnt correct. The response currently from getEncodedSemanticClassifications returns type as ts.Classifications where spans is number[].
More over you are referencing ClassifiedSpan here which is only in services and not in protocol.
This doesnt correctly reflect what we return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, I thought I reverted that, that's what I get for shipping late at night, Running baselines to update now and will double check before I ask for a re-review! Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that looks good until we add some sort of wild syntax like spans: [...[number, number, ClassificationType][] ] heh

@orta
Copy link
Contributor Author

orta commented Feb 25, 2021

Cool, I'm going to replicate this for the 4.2 branch now

@orta orta merged commit a2f09ed into microsoft:master Feb 25, 2021
orta pushed a commit to orta/TypeScript that referenced this pull request Feb 25, 2021
…oft#42640)

* Expose EncodedSemanticClassificationsRequest in protocol.d.ts

* Adds the response for encoded semantic highlights too

* Update types:

* Also include classificationtype anyway

* Fix feedback
orta pushed a commit that referenced this pull request Feb 25, 2021
#42965)

* Expose EncodedSemanticClassificationsRequest in protocol.d.ts

* Adds the response for encoded semantic highlights too

* Update types:

* Also include classificationtype anyway

* Fix feedback
This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add semantic highlighting API to Protocol
5 participants