Project reference support enhancements

[sheetalkamat]

Continuation from #1028
With this the solution builder writes files to memory instead and we keep track of those (just like with normal build)
Things seem to be working now.
Add all input files of the referenced projects are dependency so that any change to them triggers build which can report errors

[johnnyreilly]

This looks great @sheetalkamat! Thanks so much for your work 🌻🥰

I'm particularly interested in a review from @andrewbranch given his background with project references and ts-loader.

Question: does this change the way people would use ts-loader with project references in any way? As far as I can tell the significant change here is the "all input files of the referenced projects are dependency so that any change to them triggers build which can report errors"

Is there anything else to be aware of?

[sheetalkamat]

It builds all references in memory and hands off to webpack if asked

[andrewbranch]

Thanks for coming back to this, Sheetal!

My main question is how this interacts with the transpileOnly: true option. A majority of ts-loader users (sampled very unscientifically via Twitter poll) enable that option in combination with fork-ts-checker-webpack-plugin in order to speed up the loader pipeline, which is blocking, and then do semantic checks in another non-blocking process via the plugin.

My understanding is that with project references, you do get semantic errors reported for referenced projects even with transpileOnly enabled. If I recall correctly, this is essentially unavoidable since referenced projects must write declaration files. So, mainly my question is, what does transpileOnly mean in combination with project references?

• Do you get semantic error reporting for the root project (assuming it has files of its own and is not just a solution project)?
• Whatever the situation is, does transpileOnly provide any benefit? Or should it not be allowed with project references?

Second question: I see that Webpack is aware of generated tsbuildinfo and declaration files, but I can’t tell whether they’re actually written to disk or not. I think one of the main reasons people are excited about project references with ts-loader is startup performance—with tsc --build, we avoid rebuilding projects that are up-to-date by looking at the timestamps of files on disk. Do ts-loader projects benefit from that too?

[sheetalkamat]

@andrewbranch:
I don't think I am confident in what needs to happen in transpileOnly mode,. Currently if the file is from referenced project, the output from the solution builder is given instead of transpiling the file.. As you said I am not sure if this needs to be banned or its ok to give the output from the project reference. But currently i think giving output from solution builder is preferred because that's what projectReference means. Using project references means that the referenced file can be compiled using different compiler options than the one you have and that's what 0561a9f shows.. Where I added picking up output from solutionbuilder for the file from referenced project.
It also reports those errors since if there are errors there is no output for the file from referenced project so its important to report those errors.

On tsbuildInfo and tsc -b optimizations:
The initial speed cannot be achieved because .js files are not written to disk anymore. They are compiled in memory so there is no way to to do upto dateness check and yet give .js file back to webpack. Instead since .tsbuildinfo and other .js and .d.ts files are in memory, any change to file ensures that we are only building related projects incrementally (just like tsc -b does) but except only on all the referenced project.,, The outer project is built normally and will use .d.ts output from the solution builder for the referenced project files. we do give .d,ts and .tsbuildinfo files as assets to webpack just like normal program does when it has --d and or --incremental enabled.

Here is more detailed explaination of why .tsbuildinfos from the disk cannot be used for the initial build:

• Assume you ran webpack on your project and wrote .tsbuildinfo on the disk.
• Next time you run webpack again, if you read .tsbuildinfo from the disk, you would think that you need to build again because there is no .js file which is correct thing to do, but if you reused the .tsbuildinfo to create incremental program, builder will think there are no changes so there wont be any output emitted, resulting in no lib/index.js

[andrewbranch]

Re transpileOnly: So the option does still affect the root project, but not referenced projects. Some users could still get a substantial speed increase if their root project was large, but then, they’d want fork-ts-checker-webpack-plugin to type check just the root project to avoid redundancy. For users whose root project is a solution project, transpileOnly is meaningless and they definitely should not use fork-ts-checker-webpack-plugin. This seems like a confusing decision matrix to put on users, and I’m unsure whether it’s even possible to coordinate the necessary changes with fork-ts-checker-webpack-plugin to avoid the duplicate checking of referenced projects. It’s tempting to say that project references is mutually exclusive with fork-ts-checker and transpileOnly. @johnnyreilly what are your thoughts?

---

The initial speed cannot be achieved because .js files are not written to disk anymore. They are compiled in memory so there is no way to to do upto dateness check and yet give .js file back to webpack.

In a future update, would it be possible (in addition to giving them to Webpack) to write these files to some cache location to improve startup time? So on a subsequent Webpack build, we could perform an up-to-date check, and if it passed, just give the content from the cached JS files to Webpack? Do we have the APIs to support that?

[sheetalkamat]

@andrewbranch Actually original issue in writing and reading files for referenced projects to disk was they were written when building and that would confuse webpack.. Now with eb4b4fc it retains keeps them memory and hands off to webpack to write to disk so everything works out correctly as expected with additional benefit of working like tsc -b where we have upto dateness checks and fast builds

[johnnyreilly]

Amazing work @sheetalkamat!

For users whose root project is a solution project, transpileOnly is meaningless and they definitely should not use fork-ts-checker-webpack-plugin. This seems like a confusing decision matrix to put on users, and I’m unsure whether it’s even possible to coordinate the necessary changes with fork-ts-checker-webpack-plugin to avoid the duplicate checking of referenced projects. It’s tempting to say that project references is mutually exclusive with fork-ts-checker and transpileOnly. @johnnyreilly what are your thoughts?

@andrewbranch I must confess I got confused even reading the paragraph 😉

Quite seriously I think considering the developer experience is important. If it's too complicated then people either won't use it, or won't use it right and will report "bugs" that really represent confusion.

At present, if you're using transpileOnly then ts-loader transpiles only, all typechecking is handled by the fork-ts-checker-webpack-plugin. My instinct is that, if possible, this same demarcation of responsibilities would make sense to users in a projectReferences context too.

I'd love it if we could find a way forward here. Also, we some things that may help us too. fork-ts-checker-webpack-plugin is (as of last year) part of the same TypeStrong family as ts-loader. Not only that, the very excellent @piotr-oles has been thinking about this very problem and may have useful thoughts to share:

TypeStrong/fork-ts-checker-webpack-plugin#187 (comment)

We are working on the major refactor of the plugin internals as a 5.0.0 release. The new implementation should support project references out of the box :)

If there's collaboration required between ts-loader and fork-ts-checker-webpack-plugin required to reach a solution, I think that's definitely possible 🥰

cc @MLoughry @Ivaylo-Lafchiev

[johnnyreilly]

I see that @sheetalkamat is still working on this which is tremendous! We got some failing builds on node 8 in Travis which I've requeued. Incidentally I'm considering dropping support for node 8 now it's reached end of life.

If you'd like to do that as part of this PR you have my blessing @sheetalkamat 🤗; just delete this line of code:

ts-loader/.travis.yml

Line 7 in d0c4b41

- "8"

I'm glad we've started the transpileOnly / fork-ts-checker-webpack-plugin discussion here. I think I should say that having started the discussion, it doesn't need to be solved in this PR.

I'm very much a believer in shipping incremental improvements as often as you can. On that, just shipping this PR would be a tremendous win for the community.

We could provide an advisory note along the lines of "you can expect this to work as hoped where transpileOnly: false, how this might work with transpileOnly: true, alongside fork-ts-checker-webpack-plugin has no guarantees as yet and may change in future"

Just wanted to add this into the mix.

[sheetalkamat]

@johnnyreilly completely agree with getting this in.. I am just trying to get all the tests to pass. Have ran into multiple issues with comparison tests esp when applying patches..

[johnnyreilly]

I've requeued the failing jobs a bunch of times in Travis. The behaviour is strange. My observations:

• node 10 comparison tests are clean as a whistle - failures always occur on node 12
• failures always appear to relate to project references transpile tests. I've seen projectReferencesMultiple fail and different project references tests too.

We used to have something in place that reran failing comparison tests repeatedly given that they are flaky and will often fail and then pass. That doesn't seem to be in place now so we're seeing failures straight away.

The node 10 / node 12 difference is super curious to me. My assumption is that you're developing with node 10? I'm curious if by switching to node 12 you'd get different output? i.e. is there genuinely different behaviour depending upon node version?

That's possible and may even be legitimate. We won't be able to make comparison tests work on node 10 and 12 if that's the case. If it's different behaviour but crucially still correct / functional we may want to consider running some tests conditionally based upon the node version.

I'm slightly getting ahead of myself here, but I wanted to feed in some options / thoughts

Great work! 🤗

[sheetalkamat]

@johnnyreilly I am using node 12. Thats why its surprising to see it fail.. I dont seem to see that failure any time at all.. May be i should just remove the timeout i added in 9d62b44 I did that to make sure that tests have same baseline as what actually happens but it is prone to timing issues.. (Without that tests dont show the correct behaviour eg. changing libFile doesnt show change in bundle.js in transpile because it happens in two rounds of compilation - one provides updated lib.js as asset to webpack and next one updates the bundle.js with that content but since we were only capturing first one, our baseline bundle.js doesnt have changed lib file content which is what i was trying to fix in that commit)

[johnnyreilly]

We have passing tests! Celebrate 🥰🌻

@sheetalkamat is there any more you want to add to this PR?

@andrewbranch unless Sheetal says otherwise, I believe we're pretty much there, when you have a moment could you give this a once over please?

Following on from the discussion earlier, I think this PR represents getting ts-loader to a good state with project references in a transpileOnly: false world. Just that. There may be more to do / behaviour change around transpileOnly: true connected with fork-ts-checker-webpack-plugin in future, but that will be in a subsequent PR(s).

If everyone is happy I'd propose we release this. In terms of specifics it's probably good to bump the major version and mention we're not supporting node 8 anymore.

To do that we'll just need this to jump to 7.0.0:

ts-loader/package.json

Line 3 in d0c4b41

"version": "6.2.2",

And an entry in the CHANGELOG.md as well ☺️

[piotr-oles]

It's great to hear that ts-loader will support project references 👍

I'm still working on the rewrite of the fork-ts-checker-webpack-plugin but I think I will be able to publish an alpha version in a few days. One of the goals of the next major release is to support project references - I believe we can make it work. Great that I will have a reference to a working solution :)

[sheetalkamat]

@johnnyreilly No nothing more. Note that to get the tests pass, i reverted tests to old behaviour that means in some cases they dont represent correct behavior (7b0ec7d)

[johnnyreilly]

Just published https://github.com/TypeStrong/ts-loader/releases/tag/v7.0.0

Should appear on npm shortly! Thanks for your work @sheetalkamat and @andrewbranch 🥰🌻

[johnnyreilly]

See also @piotr-oles work in progress on project references support in fork-ts-checker-webpack-plugin here: TypeStrong/fork-ts-checker-webpack-plugin#404 (review)