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

Disable ts caching for typechecking #213

Merged
merged 2 commits into from
Jan 9, 2019
Merged

Disable ts caching for typechecking #213

merged 2 commits into from
Jan 9, 2019

Conversation

guybedford
Copy link
Contributor

This is the alternative fix to #210 that retains the typing errors being emitted.

The approach here is to disable caching for ts compilations entirely to ensure that the types of the local files are checked everytime.

To utilize the webpack 5 cache for this work would likely require the ts-loader project to serialize its internal memoizations using the webpack cache api while itself becoming uncacheable. //cc @johnnyreilly

@guybedford
Copy link
Contributor Author

(Note caching still applies fine to everything in node_modules of course)

@codecov-io
Copy link

Codecov Report

Merging #213 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   68.68%   68.84%   +0.15%     
==========================================
  Files          12       13       +1     
  Lines         594      597       +3     
==========================================
+ Hits          408      411       +3     
  Misses        186      186
Impacted Files Coverage Δ
src/index.js 72.09% <ø> (ø) ⬆️
src/loaders/uncacheable.js 100% <100%> (ø)

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 ae96fbb...7791c07. Read the comment docs.

@johnnyreilly
Copy link

johnnyreilly commented Jan 9, 2019

Hey @guybedford!

To utilize the webpack 5 cache for this work would likely require the ts-loader project to serialize its internal memoizations using the webpack cache api while itself becoming uncacheable. //cc @johnnyreilly

I'm afraid I'm not too close to this issue; got a lot going on right now.

From your comment am I right in understanding that you think that making the changes suggested on #208 seem unwise?

TypeStrong/ts-loader#894

Also, are you aware of https://github.com/Realytics/fork-ts-checker-webpack-plugin

This plugin works nicely in concert with ts-loader; allowing you to run ts-loader in transpileOnly mode whilst the plugin handles the typechecking. For performance reasons my default is always to use the fork-ts-checker-webpack-plugin

@rauchg rauchg merged commit 8989ee5 into master Jan 9, 2019
@rauchg rauchg deleted the ts-cache-disable branch January 9, 2019 21:44
@guybedford
Copy link
Contributor Author

Thanks for the update @johnnyreilly. Our use case for the webpack cache is having the TS internal cache persisted between runs, which I don't think either project would offer us currently? Work towards that is what I was referring to in my comment. Running the separate projects with threading definitely seems like it would be a worthwhile perf improvement though only for the cold build times (and watcher I suppose).

@johnnyreilly
Copy link

Our use case for the webpack cache is having the TS internal cache persisted between runs, which I don't think either project would offer us currently?

Totally correct yes

Running the separate projects with threading definitely seems like it would be a worthwhile perf improvement though only for the cold build times (and watcher I suppose).

Yes you should definitely see a gain in those scenarios.

Thanks for giving me a heads up BTW; had no idea ts-loader was being used in NCC 😁

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.

4 participants