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
felixfbecker merged 1 commit into
sourcegraph:masterfrom
daviwil:typescript-2.6
Feb 16, 2018
Merged

feat(typescript): upgrade to typescript 2.6.2#430
felixfbecker merged 1 commit into
sourcegraph:masterfrom
daviwil:typescript-2.6

Conversation

@daviwil

@daviwil daviwil commented Feb 15, 2018

Copy link
Copy Markdown
Contributor

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

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@daviwil

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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

codecov Bot commented Feb 16, 2018

Copy link
Copy Markdown

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.

Comment thread package.json Outdated
"semaphore-async-await": "^1.5.1",
"string-similarity": "^1.1.0",
"typescript": "2.4.2",
"typescript": "~2.6.2",

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

Comment thread src/typescript-service.ts
[`unit`, CompletionItemKind.Unit],
[`value`, CompletionItemKind.Value],
[`variable`, CompletionItemKind.Variable],
])

Copy link
Copy Markdown
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?

@daviwil daviwil Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor

Thanks a lot!

@daviwil

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

🎉 No problem, and thank you too!

@felixfbecker

Copy link
Copy Markdown
Contributor

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

@daviwil

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Yeah that would be amazing

@daviwil

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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

daviwil commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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