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
Fix 2 issues with experimentalWatchApi #1159
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5d6de4f
Detect if files already updated by watchAPI
appzuka f72119e
Only put files in cache if processed by loaders
appzuka 0847a68
Update comparison tests
appzuka a382d2e
Remove npm command for debugging tests
appzuka f9a478c
Remove package-lock.json
appzuka f0d6d8a
Update version & Changelog
appzuka a31d066
Merge branch 'master' into master
johnnyreilly File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 5 additions & 4 deletions
9
...sWatchRefWithTwoFilesAlreadyBuilt_Composite_WatchApi/expectedOutput-3.9/patch2/output.txt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.85 KiB main [emitted] main | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.97 KiB main [emitted] main | ||
| tsconfig.tsbuildinfo 1.72 KiB [emitted] | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 131 bytes {main} [built] | ||
| [./app.ts] 215 bytes {main} [built] | ||
| [./lib/helper.ts] 138 bytes {main} | ||
| [./lib/index.ts] 224 bytes {main} | ||
63 changes: 63 additions & 0 deletions
63
...ithTwoFilesAlreadyBuilt_Composite_WatchApi/expectedOutput-3.9/patch2/tsconfig.tsbuildinfo
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| { | ||
| "program": { | ||
| "fileInfos": { | ||
| "../../node_modules/typescript/lib/lib.d.ts": { | ||
| "version": "-10496480823", | ||
| "signature": "-10496480823", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.es5.d.ts": { | ||
| "version": "-218882352090", | ||
| "signature": "-218882352090", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.dom.d.ts": { | ||
| "version": "300634082611", | ||
| "signature": "300634082611", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.webworker.importscripts.d.ts": { | ||
| "version": "-24714112149", | ||
| "signature": "-24714112149", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.scripthost.d.ts": { | ||
| "version": "204309182321", | ||
| "signature": "204309182321", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "./lib/index.d.ts": { | ||
| "version": "11215156582", | ||
| "signature": "11215156582", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "./lib/helper.d.ts": { | ||
| "version": "7897218607", | ||
| "signature": "7897218607", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "./app.ts": { | ||
| "version": "-12553192154", | ||
| "signature": "-3531856636", | ||
| "affectsGlobalScope": false | ||
| } | ||
| }, | ||
| "options": { | ||
| "types": [], | ||
| "composite": true, | ||
| "newLine": 1, | ||
| "configFilePath": "./tsconfig.json", | ||
| "skipLibCheck": true, | ||
| "suppressOutputPathCheck": true | ||
| }, | ||
| "referencedMap": { | ||
| "./app.ts": [ | ||
| "./lib/helper.d.ts", | ||
| "./lib/index.d.ts" | ||
| ] | ||
| }, | ||
| "exportedModulesMap": {}, | ||
| "semanticDiagnosticsPerFile": [] | ||
| }, | ||
| "version": "3.9.3" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 5 additions & 4 deletions
9
...ectReferencesWatchRefWithTwoFiles_Composite_WatchApi/expectedOutput-3.9/patch1/output.txt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.83 KiB main [emitted] main | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.95 KiB main [emitted] main | ||
| tsconfig.tsbuildinfo 1.72 KiB [emitted] | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 131 bytes {main} [built] | ||
| [./app.ts] 215 bytes {main} [built] | ||
| [./lib/helper.ts] 138 bytes {main} | ||
| [./lib/index.ts] 211 bytes {main} |
63 changes: 63 additions & 0 deletions
63
...cesWatchRefWithTwoFiles_Composite_WatchApi/expectedOutput-3.9/patch1/tsconfig.tsbuildinfo
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| { | ||
| "program": { | ||
| "fileInfos": { | ||
| "../../node_modules/typescript/lib/lib.d.ts": { | ||
| "version": "-10496480823", | ||
| "signature": "-10496480823", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.es5.d.ts": { | ||
| "version": "-218882352090", | ||
| "signature": "-218882352090", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.dom.d.ts": { | ||
| "version": "300634082611", | ||
| "signature": "300634082611", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.webworker.importscripts.d.ts": { | ||
| "version": "-24714112149", | ||
| "signature": "-24714112149", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.scripthost.d.ts": { | ||
| "version": "204309182321", | ||
| "signature": "204309182321", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "./lib/index.d.ts": { | ||
| "version": "12503634626", | ||
| "signature": "12503634626", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "./lib/helper.d.ts": { | ||
| "version": "7897218607", | ||
| "signature": "7897218607", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "./app.ts": { | ||
| "version": "-12553192154", | ||
| "signature": "-3531856636", | ||
| "affectsGlobalScope": false | ||
| } | ||
| }, | ||
| "options": { | ||
| "types": [], | ||
| "composite": true, | ||
| "newLine": 1, | ||
| "configFilePath": "./tsconfig.json", | ||
| "skipLibCheck": true, | ||
| "suppressOutputPathCheck": true | ||
| }, | ||
| "referencedMap": { | ||
| "./app.ts": [ | ||
| "./lib/helper.d.ts", | ||
| "./lib/index.d.ts" | ||
| ] | ||
| }, | ||
| "exportedModulesMap": {}, | ||
| "semanticDiagnosticsPerFile": [] | ||
| }, | ||
| "version": "3.9.3" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 5 additions & 4 deletions
9
...ison-tests/projectReferencesWatch_Composite_WatchApi/expectedOutput-3.9/patch1/output.txt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.33 KiB main [emitted] main | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.37 KiB main [emitted] main | ||
| tsconfig.tsbuildinfo 1.56 KiB [emitted] | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 131 bytes {main} [built] | ||
| [./app.ts] 169 bytes {main} [built] | ||
| [./lib/index.ts] 150 bytes {main} |
57 changes: 57 additions & 0 deletions
57
.../projectReferencesWatch_Composite_WatchApi/expectedOutput-3.9/patch1/tsconfig.tsbuildinfo
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| { | ||
| "program": { | ||
| "fileInfos": { | ||
| "../../node_modules/typescript/lib/lib.d.ts": { | ||
| "version": "-10496480823", | ||
| "signature": "-10496480823", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.es5.d.ts": { | ||
| "version": "-218882352090", | ||
| "signature": "-218882352090", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.dom.d.ts": { | ||
| "version": "300634082611", | ||
| "signature": "300634082611", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.webworker.importscripts.d.ts": { | ||
| "version": "-24714112149", | ||
| "signature": "-24714112149", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "../../node_modules/typescript/lib/lib.scripthost.d.ts": { | ||
| "version": "204309182321", | ||
| "signature": "204309182321", | ||
| "affectsGlobalScope": true | ||
| }, | ||
| "./lib/index.d.ts": { | ||
| "version": "11215156582", | ||
| "signature": "11215156582", | ||
| "affectsGlobalScope": false | ||
| }, | ||
| "./app.ts": { | ||
| "version": "-16299197056", | ||
| "signature": "-3531856636", | ||
| "affectsGlobalScope": false | ||
| } | ||
| }, | ||
| "options": { | ||
| "types": [], | ||
| "composite": true, | ||
| "newLine": 1, | ||
| "configFilePath": "./tsconfig.json", | ||
| "skipLibCheck": true, | ||
| "suppressOutputPathCheck": true | ||
| }, | ||
| "referencedMap": { | ||
| "./app.ts": [ | ||
| "./lib/index.d.ts" | ||
| ] | ||
| }, | ||
| "exportedModulesMap": {}, | ||
| "semanticDiagnosticsPerFile": [] | ||
| }, | ||
| "version": "3.9.3" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 11 additions & 5 deletions
16
...ison-tests/projectReferencesWatch_Composite_WatchApi/expectedOutput-3.9/patch4/output.txt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,12 @@ | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.37 KiB main [emitted] main | ||
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.38 KiB main [emitted] main | ||
| tsconfig.tsbuildinfo 1.56 KiB [emitted] | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 169 bytes {main} [built] | ||
| [./lib/index.ts] 145 bytes {main} | ||
| [./app.ts] 186 bytes {main} [built] [1 error] | ||
| [./lib/index.ts] 145 bytes {main} | ||
|
|
||
| ERROR in app.ts | ||
| ./app.ts | ||
| [90m[tsl] [39m[1m[31mERROR[39m[22m[1m[31m in [39m[22m[1m[36mapp.ts(3,56)[39m[22m | ||
| [1m[31m TS2551: Property 'ffive' does not exist on type '{ one: number; two: number; three: number; four: number; five: number; }'. Did you mean 'five'?[39m[22m |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems incorrect from project references point of view. It should not write lib\tsconfig.tsbuildinfo file since there are errors in the build of project
libthis will cause unknown issues as project cannot have tsbuild info file if there are errors. (not in 3.9 for sure)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for commenting @sheetalkamat! Would you advise reverting this?
In a side note, @appzuka has been observing that the watch API support (which I believe you originally added 🤗) does not improve performance of
ts-loader. This is surprising given that it provides a massive performance boost tofork-ts-checker-webpack-plugin- however that does seem to be the case.I'd always hoped that the watch API usage would become the default way to run ts-loader. The idea being that it stayed behind the
experimentalWatchApiflag until it was fully functional and outperforming the servicehost approach. Then we'd drop the old approach in favour of the watch API. However, that day has not come sadly! Due to various bugs and the performance issue we're now thinking about potentially removing it.What are your thoughts? I'm mindful you put a good amount of effort into implementation in the first place (which we're super grateful for BTW!) And also, if there's a simple thing
ts-loaderis doing slightly wrong here that would make the watch API perform I'd still love to make that happen. Again I can't forget thatfork-ts-checker-webpack-plugingot turbo charged from using the watch API so there's a good chance we're doing something sub optimal.What do you think?
cc @andrewbranch in case there's anything he'd like to add to this discussion too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the commit prior to this PR, in projectReferencesWatchRefWithTwoFilesAlreadyBuilt, patch2 is supposed to use the new number added to the helper in patch 1:
ts-loader/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt/patch2/app.ts
Line 4 in a77470f
In the output bundle you can see the new number has been added:
ts-loader/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt/expectedOutput-3.9/patch2/bundle.js
Line 97 in a77470f
But in the watchAPI version it does not:
ts-loader/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt_WatchApi/expectedOutput-3.9/patch2/bundle.js
Line 97 in a77470f
And the same with the Composite_WatchApi
ts-loader/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt_Composite_WatchApi/expectedOutput-3.9/patch2/bundle.js
Line 97 in a77470f
Also note that tsconfig.tsbuildinfo is not written to the patch2 output:
https://github.com/TypeStrong/ts-loader/tree/a77470f9072bf24dc7165653c2ee86b243b65040/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt_Composite_WatchApi/expectedOutput-3.9/patch2
After the PR#1159 the output of patch2 correctly includes the new content and the tsconfig.tsbuildinfo:
ts-loader/test/comparison-tests/projectReferencesWatchRefWithTwoFilesAlreadyBuilt_Composite_WatchApi/expectedOutput-3.9/patch2/bundle.js
Line 97 in 2f6cbe8
Note that the new tsconfig.tsbuildinfo is in the top level directory not in the lib folder.
I was concerned that the PR changed the comparison test results but I believe the previous output was incorrect so the changes reflect the fixing of behaviour by the PR. The watchapi code is quite complex so there could be further issues which need to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@appzuka my bad.. i saw that tsbuildinfo as from lib project instead of root project. Change looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnnyreilly Yes watchAPI should be faster and should see benefit.. I am investigating performance for internal team that uses ts-loader with solution builder work.. i can take a issue with watch API but i will need concrete repro steps so i can investigate. Watch API is kind of similar to solution builder API (how it handles file changes etc) so current perf work i am doing might help not sure. In the mean time if you can get repro where not using watch api is faster than watch api, i will be better equipped to investigate it. without that its just analysing code which i might not have enough time to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sheetalkamat - thanks so much!
@appzuka you've bumped on the problems with the performance of the watch API the most as you've been looking into this. I'm bearing in mind your comment here:
Would you be up for providing some kind of demo repo which demonstrates performance being better with the non watch API vs the watch API?
No worries if you can't - I can try and get to it later (after my holidays)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To observe any performance change a large codebase will be needed. This will not just be a single file with many lines, but many files nested several levels, like a real, large project. I have created a repo which generates a codebase to test the various options. The repo is at:
https://github.com/appzuka/ts-loader-watchapitest.git
The Readme gives instructions to install and run and the results I get on my system.
Perhaps I have misunderstood how the new watchApi is supposed to work in ts-loader but I cannot see any reduction in build time at any stage. A simple build with expermentalWatchApi is over 2x the time using transpileOnly and fork-ts-checker-webpack-plugin and 30% slower than a normal build. Even incremental builds are much slower.
I love the --incremental feature of tsc, but it seems to me it is hard to integrate this into a webpack loader. Loaders should perform one simple task - they take the source text given to them by webpack and they return transformed text. ts-loader with transpileOnly does this very well.
Trying to implement the incremental behaviour means that the loader must consider the whole project every time webpack asks it to process every file. ts-loader handles this by caching the compiled files after the first ts file is built and recovering them from the cache for later files at the cost of greatly increasing the complexity of ts-loader.
It seems to me that incremental mode would be better implemented as a webpack plugin that compiles the entire project to a js-cache folder before webpack starts to compile the project. tsc would need to be called just once and Webpack would then build using the js files. This would still work in watch mode - tsc would update the files in the js-cache after a ts file was updated then webpack would recompile the project.
Apart from greatly simplifying the process, this would enable the performance gains of incremental mode to be used with webpack. If the js-cache persists between runs on the filesystem, subsequent builds will use the tsconfig.tsbuildinfo file to only recompile as necessary.
As a proof of concept I separated the tsc part of the build of a project from webpack. For my project I found that about 50% of the initial build time was taken by tsc and 50% by webpack. On the second build the tsc part dropped close to zero, as would be expected using incremental mode.
Perhaps fork-ts-checker-webpack-plugin works because it is a plugin - it runs just once for each build not once for every file.
There would be some disadvantages to this method. You could not use a webpack loader to transform the code before being processed by tsc. I assume fork-ts-checker-webpack-plugin suffers from this as well, but I doubt it is often a requirement in real code.
I could have a go at creating such a plugin. It would probably be separate from ts-loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @appzuka! @sheetalkamat you should find what you need here ☝️
Props on your plugin experiment; the results sound very interesting! Incidentally,
fork-ts-checker-webpack-pluginis part of the TypeStrong family; if you fancied applying some of your experiments there @piotr-oles and myself may well be grateful 🤗Funnily enough I think this was an issue and has been resolved.... I've been digging through recent PRs but I can't find the one in question. Pretty sure it's out there though!