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.

Sign up for GitHub

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

Jump to bottom

Stop computing outdated diagnostics with CancellationToken #2332

Merged
merged 7 commits into from
Oct 30, 2020
Merged

Stop computing outdated diagnostics with CancellationToken #2332

merged 7 commits into from
Oct 30, 2020

Conversation

yoyo930021
Copy link
Member

Fixed #1263
Fix a part of #2192

@yoyo930021 yoyo930021 marked this pull request as ready for review September 27, 2020 08:59
@yoyo930021
Copy link
Member Author

yoyo930021 commented Sep 27, 2020 •
edited
Loading

I think we can't forward that cancellation token to TS Server for completion.
I only find a cancelling TS server all response method.
https://github.com/microsoft/TypeScript/blob/88c24e6b0d0efb868088ed6da359ef55e6f1c6b6/src/services/services.ts#L1236

In addition, getCompletionsAtPosition can't cancel for top method.
https://github.com/microsoft/TypeScript/blob/88c24e6b0d0efb868088ed6da359ef55e6f1c6b6/src/services/services.ts#L1532

yoyo930021
yoyo930021 commented Sep 27, 2020
View reviewed changes
server/src/modes/script/javascript.ts
...program.getDeclarationDiagnostics(sourceFile, cancellationToken?.tsToken)
];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ref: https://github.com/microsoft/TypeScript/blob/88c24e6b0d0efb868088ed6da359ef55e6f1c6b6/src/services/services.ts#L1504

@yoyo930021 yoyo930021 mentioned this pull request Sep 28, 2020
Closed
3 tasks
@yoyo930021 yoyo930021 mentioned this pull request Oct 5, 2020
Open
3 tasks
@yoyo930021 yoyo930021 force-pushed the impl-cancellationToken branch 2 times, most recently from dedc2a5 to 5834eb2 Compare October 7, 2020 15:29
@yoyo930021
Copy link
Member Author

yoyo930021 commented Oct 9, 2020 •
edited
Loading

When this PR merged:

  • Replace CLIEngine to Eslint for eslint deprecated Node api.
  • Add getSuggestionDiagnostics support. Like deprecated, unused feature.
  • Add Fix all feature.

This was referenced Oct 9, 2020
Merged
Merged
@yoyo930021 yoyo930021 marked this pull request as draft October 17, 2020 11:01
@yoyo930021
Copy link
Member Author

yoyo930021 commented Oct 17, 2020 •
edited
Loading

This PR is broken VTI diagnostic.
I need more research.

@yoyo930021 yoyo930021 force-pushed the impl-cancellationToken branch from 5834eb2 to 5c43502 Compare October 17, 2020 12:01
@yoyo930021 yoyo930021 marked this pull request as ready for review October 17, 2020 12:01
@yoyo930021
Copy link
Member Author

I made a retarded mistake, and I've fixed it.

@yoyo930021 yoyo930021 force-pushed the impl-cancellationToken branch 2 times, most recently from 3f8fc71 to 290f03c Compare October 19, 2020 09:54
octref
octref requested changes Oct 29, 2020
View reviewed changes
server/src/utils/cancellationToken.ts Outdated
get token(): VCancellationToken {
const token = super.token as VCancellationToken;
// tslint:disable-next-line variable-name
const Exception = this.tsModule.OperationCanceledException;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to do this assignment + TSLint rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be my mistake.
I read the tslint rules wrong.

server/src/services/vls.ts Outdated
@@ -176,7 +179,7 @@ export class VLS {
this.lspConnection.onRequest('$/getDiagnostics', params => {
const doc = this.documentService.getDocument(params.uri);
if (doc) {
return this.doValidate(doc);
return this.doValidate(doc).then(result => result ?? []);
Copy link
Member

Choose a reason for hiding this comment

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

Make this callback async for readability

Copy link
Member Author

Choose a reason for hiding this comment

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

Example add async in function?

server/src/utils/cancellationToken.ts Outdated
}
}

export function isVCancellationTokenCancel(token?: VCancellationToken) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this isVCancellationRequested (or isVCancellationRequestedOnToken if you prefer)?

Copy link
Member Author

@yoyo930021 yoyo930021 Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

isVCancellationRequested is great.
Thanks.

server/src/modes/script/javascript.ts Outdated
];

const compilerOptions = program.getCompilerOptions();
if (!!(compilerOptions.declaration || compilerOptions.composite)) {
Copy link
Member

Choose a reason for hiding this comment

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

If the result is undefined this block won't execute, so I think it's unnecessary convert it to Boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember it is resolve type error.

@octref octref force-pushed the impl-cancellationToken branch from 290f03c to 3affd7d Compare October 30, 2020 01:11
@octref octref merged commit e21eae3 into vuejs:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@octref octref octref approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send Cancellation Token to TypeScript Language Server
2 participants
@yoyo930021 @octref