Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement getProjectVersion #963

Merged
merged 3 commits into from Mar 2, 2020
Merged

Implement getProjectVersion #963

merged 3 commits into from Mar 2, 2020

Conversation

cspotcode
Copy link
Collaborator

Due to microsoft/TypeScript#36748, TypeScript's language service always thinks the Program is out-of-date and rebuilds it 3 times for every .ts file that we load: once each for getEmitOutput, getSemanticDiagnostics, and getSyntacticDiagnostics.

We can workaround this bug by implementing getProjectVersion. If getProjectVersion stays the same, TypeScript will reuse the existing Program. It'll still need to be rebuilt every time we modify the rootFiles array, but that's only once instead of 3 times.

I also added debug log statements that double-check when the Program instance is rebuilt. This should be helpful if we ask for TS_NODE_DEBUG logs from users.

@coveralls
Copy link

coveralls commented Feb 16, 2020

Coverage Status

Coverage increased (+0.4%) to 79.231% when pulling 7a1fe84 on ab/add-getprojectversion into 3eb46e0 on master.

Copy link
Member

@blakeembrey blakeembrey left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to ignore the comments. I'm not sure how useful the debug statements are if the conditions are as mentioned in the comment - maybe better as an invariant check that throws?

if (!isFileInCache) {
rootFileNames.push(fileName)
// Modifying rootFileNames means a project change
shouldIncrementProjectVersion = true
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could just do projectVersion++ here - it's essentially the same, an extra +1 every now and then isn't a big deal.

src/index.ts Outdated
@@ -469,6 +470,7 @@ export function create (rawOptions: CreateOptions = {}): Register {

// Create the compiler host for type checking.
const serviceHost: _ts.LanguageServiceHost = {
getProjectVersion: () => `${ projectVersion }`,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Personally I'd just do String(projectVersion) since you just want to type cast.

@cspotcode
Copy link
Collaborator Author

I'm not sure how useful the debug statements are if the conditions are as mentioned in the comment - maybe better as an invariant check that throws?

Yeah, I wasn't sure about that. I really wanted to avoid ts-node crashing if new versions of the TypeScript compiler misbehaved. I figured better for us to remain functional with degraded performance and let the debug logs point out why.

@cspotcode cspotcode merged commit 35e2f92 into master Mar 2, 2020
@cspotcode cspotcode deleted the ab/add-getprojectversion branch May 9, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants