Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

feat(typescript): upgrade to typescript 2.6.2 #430

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

daviwil
Copy link
Contributor

@daviwil daviwil commented Feb 15, 2018

This change updates TypeScript to 2.6.2, the latest stable version. We had to make some changes to address API breaking changes and a compiler change which required the code here to change to avoid the use of constructor as an object key.

Please let us know if there's anything else we can change to get this accepted!

Resolves #359

/cc @damieng

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Looks like there are some tests which depend on code locations in a specific version of TypeScript, will get those fixed momentarily.

@felixfbecker
Copy link
Contributor

Yes, some tests assert on go-to-definition locations in lib.d.ts, which can change between versions.

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Fixing this now, will have an update pushed soon!

@daviwil daviwil force-pushed the typescript-2.6 branch 2 times, most recently from 39238e8 to 22553ad Compare February 16, 2018 20:21
@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

OK, I fixed the broken tests! Turns out that there was another test that had an issue with TypeScript 2.6:

  6) TypeScriptService
       rootUri file:///foo/bar/
         textDocumentCompletion()
           produces completions for referenced symbols:
     Error: Debug Failure. False expression.
      at computePositionOfLineAndCharacter (node_modules\typescript\lib\typescript.js:5749:22)

This was caused by a more strict assert for text document positions added in the 2.6 series, it caught a bad character position in the test code.

By the way, any chance of a new version of the language server being published once this gets merged? We'd love to include it in Atom's ide-typescript update later today!

@codecov
Copy link

codecov bot commented Feb 16, 2018

Codecov Report

Merging #430 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   83.16%   83.26%   +0.09%     
==========================================
  Files          15       15              
  Lines        2038     2038              
  Branches      415      482      +67     
==========================================
+ Hits         1695     1697       +2     
+ Misses        341      339       -2     
  Partials        2        2
Impacted Files Coverage Δ
src/typescript-service.ts 85.15% <100%> (ø) ⬆️
src/project-manager.ts 87.79% <0%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 982bf69...159f2f9. Read the comment docs.

package.json Outdated
@@ -51,7 +51,7 @@
"rxjs": "^5.5.0",
"semaphore-async-await": "^1.5.1",
"string-similarity": "^1.1.0",
"typescript": "2.4.2",
"typescript": "~2.6.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we can guarantee that TS will still work on patch version upgrades. We also don't currently have a lockfile so this would be very risky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! I'll remove the tilde.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@felixfbecker
Copy link
Contributor

We release automatically with semantic-release, so yeah this would be released after merge

[`unit`, CompletionItemKind.Unit],
[`value`, CompletionItemKind.Value],
[`variable`, CompletionItemKind.Variable],
])
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the reason to make this a Map, given it never changes?

Copy link
Contributor Author

@daviwil daviwil Feb 16, 2018

Choose a reason for hiding this comment

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

Using constructor as an object property name caused type system problems with CompletionItemKind, somehow it inferred that Function was a part of the union of numbers that CompletionItemKind is aliased to. I couldn't find another way to escape the known keyword of constructor so using a map seemed to be the next best option. Happy to change it back if you know of a better way!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes total sense!

@felixfbecker felixfbecker changed the title chore(package): upgrade to typescript 2.6.2 feat(typescript): upgrade to typescript 2.6.2 Feb 16, 2018
@felixfbecker felixfbecker merged commit 2cac6cb into sourcegraph:master Feb 16, 2018
@daviwil daviwil deleted the typescript-2.6 branch February 16, 2018 21:04
@felixfbecker
Copy link
Contributor

Thanks a lot!

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

🎉 No problem, and thank you too!

@felixfbecker
Copy link
Contributor

Btw, is there a reason why you didn't update to 2.7.2 (latest stable)?

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Hmmm, perhaps I was under a mistaken assumption that even number patch releases were stable and odd numbers were unstable. Happy to open another PR to get us up to 2.7.2 if you like!

@felixfbecker
Copy link
Contributor

Yeah that would be amazing

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Looks like a couple more tests are broken with the 2.7.2 update so it may be later today before I send the PR, but I'll get it done :)

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Ahhh now I remember why we didn't use 2.7.2, it's because I started working on bumping the TS version last week before 2.7.1 was made stable, didn't think to check if there was a stable release this week :) PR is imminent

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants