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

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

2 participants