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

Upgrade to typescript 2.4.2 #356

Merged
merged 13 commits into from
Sep 27, 2017

Conversation

tomv564
Copy link
Contributor

@tomv564 tomv564 commented Sep 21, 2017

Upgrading directly to 2.5 yielded even more issues, this smaller bump should be more manageable.

Compile errors remaining:

  • Removed types/fields in ast.ts
  • Element type in Observables is now strictly checked causing issues with Observable<never>s to which we concat Observable<DesiredResultType>

Test issues remaining:

  • Fix tests around imports
  • Fix test and runtime issues around imports/references

@felixfbecker
Copy link
Contributor

Removed types/fields in ast.ts

I don't know tbh why ast.ts doesn't simply use getChildren().

@tomv564
Copy link
Contributor Author

tomv564 commented Sep 21, 2017

@felixfbecker: Only the typing issue with Observables remains, might be an easy fix for you? I got lost in RxJS operators last night, perhaps replacing concat with toArray().mergeMap() works?

Some notes for future review:

  • Needed to ensureSourceFile on resolved references as TS's symbol lookups were not being populated by ensureReferencedFiles anymore. Would be good peace-of-mind to know what changed.
  • Tests pass with getChildren() in ast.ts, is the test coverage sufficient to move on?

These files were not pre-loaded by ensure, so addSourceFile only added empty content. They were present in TS, but had no symbols.
@tomv564
Copy link
Contributor Author

tomv564 commented Sep 23, 2017

An update on the imports issue:

I added ensureSourceFile to ensureReferencedFiles earlier as TS was no longer parsing references automatically.
This introduced a timing issue where TS starts asking for other references (eg. thenable.d.ts) before ensureReferencedFiles has reached them.

I have two ideas left to pursue:

  • Investigate what changed in TS so it doesn't parse referenced files automatically as it did pre-2.4
  • Run addSourceFile on all the referenced files after the whole tree is resolved.

The importing logic in createProgram was refactored before 2.4-RC to handle dynamic imports (issue microsoft/TypeScript#14495) initially merged in PR microsoft/TypeScript#14774

compilerHost now assumes fileExists and readFile are implemented on LanguageServiceHost.
@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #356 into master will increase coverage by 14.36%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #356       +/-   ##
===========================================
+ Coverage    60.2%   74.56%   +14.36%     
===========================================
  Files          14       14               
  Lines        2151     1671      -480     
  Branches      351      313       -38     
===========================================
- Hits         1295     1246       -49     
+ Misses        706      279      -427     
+ Partials      150      146        -4
Impacted Files Coverage Δ
src/fs.ts 88.88% <ø> (ø) ⬆️
src/connection.ts 81.57% <ø> (ø) ⬆️
src/typescript-service.ts 72.57% <100%> (ø) ⬆️
src/packages.ts 85.91% <100%> (ø) ⬆️
src/ast.ts 83.33% <100%> (+71.72%) ⬆️
src/project-manager.ts 82.99% <50%> (-0.23%) ⬇️
src/diagnostics.ts 69.23% <80%> (-0.77%) ⬇️

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 6087974...832c560. Read the comment docs.

@tomv564
Copy link
Contributor Author

tomv564 commented Sep 25, 2017

ping @felixfbecker: I think we are ready for a review on this.

Today's update:

  • Found out import resolution was failing because we didn't implement the optional fileExists / readFile on LanguageServerHost -> created an issue about this (linked above).
  • Removed the ensureSourceFile hack and other unneeded test fixes.

@tomv564
Copy link
Contributor Author

tomv564 commented Sep 25, 2017

Upgrading to TS 2.5.2 showed new compile errors (packages.ts also needs the pairOf fix), but a few completion tests started failing, too. Not a freebie, sadly.

@felixfbecker
Copy link
Contributor

Awesome, I'll take a look when I find time

@felixfbecker felixfbecker merged commit 122329d into sourcegraph:master Sep 27, 2017
@tomv564 tomv564 deleted the upgrade-to-typescript-2.4.2 branch September 27, 2017 05:47
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