-
Notifications
You must be signed in to change notification settings - Fork 73
feat(typescript): upgrade to typescript 2.6.2 #430
Conversation
2391bac
to
da09421
Compare
Looks like there are some tests which depend on code locations in a specific version of TypeScript, will get those fixed momentarily. |
Yes, some tests assert on go-to-definition locations in lib.d.ts, which can change between versions. |
Fixing this now, will have an update pushed soon! |
39238e8
to
22553ad
Compare
OK, I fixed the broken tests! Turns out that there was another test that had an issue with TypeScript 2.6:
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 Report
@@ 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
Continue to review full report at Codecov.
|
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
22553ad
to
159f2f9
Compare
We release automatically with semantic-release, so yeah this would be released after merge |
[`unit`, CompletionItemKind.Unit], | ||
[`value`, CompletionItemKind.Value], | ||
[`variable`, CompletionItemKind.Variable], | ||
]) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
Thanks a lot! |
🎉 No problem, and thank you too! |
Btw, is there a reason why you didn't update to 2.7.2 (latest stable)? |
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! |
Yeah that would be amazing |
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 :) |
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 |
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