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

[READY] Support TypeScript 2.6.1 #869

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Nov 1, 2017

TypeScript 2.6.1 introduced a bug in TSServer where NPM warnings are printed on stdout at startup:

npm WARN 2.6 No description
npm WARN 2.6 No repository field.
npm WARN 2.6 No license field.

See issue microsoft/TypeScript#19660. The TypeScript completer chokes on this output as it always expects a HTTP-like response from TSServer and assumes that the server died if not. Though it's a TSServer bug, we can handle it by catching the ValueError exception raised when parsing invalid headers and by interrupting the reader loop only if the server is not running.

Also, this new version of TypeScript returns an empty list when no definition is found. Update the _GoToDefinition and _GoToType functions accordingly.

Fixes ycm-core/YouCompleteMe#2818.


This change is Reviewable

@puremourning
Copy link
Member

TBH i'd rather just restrict the version of tsserver that we use and point people having the issue at the bug, but we would at least have to fix the other part (about goto).

Are we certain that the npm messages will only appear during header parsing ? The code for parsing message body isn't very robust either, but is it possible for the npm message to appear after a valid content-length: ...\n\n but before an actual message ?

I would be surprised if it is, or if we should even worry about it, but thought i'd ask anyway.

:lgtm: assuming that and that the tests finally pass :)


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@codecov-io
Copy link

codecov-io commented Nov 3, 2017

Codecov Report

Merging #869 into master will decrease coverage by 0.06%.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##           master    #869      +/-   ##
=========================================
- Coverage   94.77%   94.7%   -0.07%     
=========================================
  Files          79      79              
  Lines        5435    5440       +5     
  Branches      171     171              
=========================================
+ Hits         5151    5152       +1     
- Misses        235     239       +4     
  Partials       49      49

@bstaletic
Copy link
Collaborator

Are we certain that the npm messages will only appear during header parsing ?

This is my concern too. If we can't be certain we better restrict tsserver version.
But if we can be certain, this is :lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Nov 3, 2017

is it possible for the npm message to appear after a valid content-length: ...\n\n but before an actual message ?

Looking at TSServer code, the whole response is written at once on stdout (see formatMessage) so it shouldn't be possible.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@puremourning
Copy link
Member

Shall we ship it then? Fire when ready!


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Nov 3, 2017

@zzbot r+


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Nov 3, 2017

📌 Commit 5f98ed4 has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Nov 3, 2017

⌛ Testing commit 5f98ed4 with merge 0c2a614...

zzbot added a commit that referenced this pull request Nov 3, 2017
[READY] Support TypeScript 2.6.1

TypeScript 2.6.1 introduced a bug in TSServer where NPM warnings are printed on stdout at startup:
```
npm WARN 2.6 No description
npm WARN 2.6 No repository field.
npm WARN 2.6 No license field.
```
See issue microsoft/TypeScript#19660. The TypeScript completer chokes on this output as it always expects a HTTP-like response from TSServer and assumes that the server died if not. Though it's a TSServer bug, we can handle it by catching the `ValueError` exception raised when parsing invalid headers and by interrupting the reader loop only if the server is not running.

Also, this new version of TypeScript returns an empty list when no definition is found. Update the `_GoToDefinition` and `_GoToType` functions accordingly.

Fixes ycm-core/YouCompleteMe#2818.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/869)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Nov 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 0c2a614 to master...

@zzbot zzbot merged commit 5f98ed4 into ycm-core:master Nov 3, 2017
@micbou micbou deleted the support-typescript-2.6.1 branch November 15, 2017 08:28
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Dec 3, 2017
[READY] Update ycmd

Include the following changes:
 - PR ycm-core/ycmd#856: update JediHTTP;
 - PR ycm-core/ycmd#860: improve diagnostics location in C-family languages;
 - PR ycm-core/ycmd#865: use Objective-C triggers for Objective-C++;
 - PR ycm-core/ycmd#869: support TypeScript 2.6.1;
 - PR ycm-core/ycmd#875: allow switching to a different JavaScript project with `RestartServer`;
 - PR ycm-core/ycmd#877: support `-idirafter` include flag in C-family languages.

Update the JavaScript documentation on how to switch to a different project. We only mention the `:YcmCompleter RestartServer` way as other methods involve restarting ycmd and losing all its data like stored identifiers.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2839)
<!-- Reviewable:end -->
snakeleon added a commit to snakeleon/YouCompleteMe-x64 that referenced this pull request Dec 4, 2017
Include the following changes:
 - PR ycm-core/ycmd#856: update JediHTTP;
 - PR ycm-core/ycmd#860: improve diagnostics location in C-family languages;
 - PR ycm-core/ycmd#865: use Objective-C triggers for Objective-C++;
 - PR ycm-core/ycmd#869: support TypeScript 2.6.1;
 - PR ycm-core/ycmd#875: allow switching to a different JavaScript project with `RestartServer`;
 - PR ycm-core/ycmd#877: support `-idirafter` include flag in C-family languages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants