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

Feature: Support incremental build #999

Closed
mc0 opened this issue Sep 12, 2019 · 15 comments
Closed

Feature: Support incremental build #999

mc0 opened this issue Sep 12, 2019 · 15 comments
Labels

Comments

@mc0
Copy link

mc0 commented Sep 12, 2019

Description

There has been some excellent work by @sheetalkamat and @andrewbranch on adding incremental/projectReferences API support to TypeScript [0] and ts-loader [1]. There are still some missing pieces of functionality for the incremental support in ts-loader even after a related PR was closed [2].

The primary issue is that if the base project that has Webpack in it is built with tsconfig.json containing the incremental or composite compiler options, the incremental program is never initialized and used. It's possible that this is blocked by needing support for passing in an oldProgram to TypeScript's languageService or rewriting ts-loader to stop using the languageService. I've dug into this quite a bit and thought it might be useful to pass on my thoughts.

With SolutionBuilder

I've been able to locally tweak ts-loader to run .build() on a solutionBuilder it creates/uses the tsBuildInfoFile as expected. Using solutionBuilder for ts-loader has downsides. Namely, if webpack has multiple entry values for a given project, the references will be built multiple times. Adding .build() in place of languageService would just make that problem worse.

// getEmitOutput...
            if (solutionBuilder) {
               const exitCode = solutionBuilder.build(instance.configFilePath);
               if (exitCode !== 0) {
                   throw new Error("Failed to build");
               }
               // presumably we could read the correct file and return it if we wanted
            } else {
              // ... use program.emit
            }

With LanguageService

Using LanguageService would require the ability to pass in an oldProgram (at minimum, it might need to allow a createProgram func). This seems like the most straightforward change for ts-loader but requires TypeScript changes. It is difficult to really know which API to choose at this point but it seems that the incremental support was not added to languageService.

Minimal Repo

Not a repo, but a gist: https://gist.github.com/mc0/e19f69b2b26f6b612423c17722250503

0: microsoft/TypeScript#31432
1: #935
2: #913

@johnnyreilly
Copy link
Member

Thanks for digging in and investigating and writing this up @mc0 - where do you want to take this?

@mc0
Copy link
Author

mc0 commented Sep 12, 2019

@johnnyreilly I'm hoping to start a conversation and solicit feedback on paths forward since this might pose a significant change. I'm not against contributing/helping once a path is selected. I'm also optimistic there are other paths forward here that I didn't see.

@sheetalkamat
Copy link
Contributor

sheetalkamat commented Sep 12, 2019

I think if you have repro showing why solutionBuilder.build is not working, we can take a look and come up with action.
Also note that we (typescript) recommends using watch api over the language service for build like scenarios.. Language service apis is more for editor like scenarios.
Note we are thinking about getEmitOutput and project references as part of microsoft/TypeScript#33270

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 12, 2019

Details to add in: #757

We have watch API support in ts-loader behind the experimentalWatchApi provided by @sheetalkamat in fact!

The plan was always to graduate this to the default behaviour, however there were occasional reports of issues (unfortunately with reliable repros) which made me nervous of ever taking that step. Perhaps we should do a breaking change that defaults it to on, that might flush out a reliable repro. If indeed there is an issue.

The one problem there is Vue support. I don't think anyone has succeeded in getting the watch API working with Vue. @phry has certainly tried on the fork-ts-checker-webpack-plugin: TypeStrong/fork-ts-checker-webpack-plugin#231

(The flag there is misnamed useTypescriptIncrementalApi - should really be useTypescriptWatchApi)

@mc0, do you want to propose some potential paths forward? That could prompt some useful discussion perhaps.

@mc0
Copy link
Author

mc0 commented Sep 14, 2019

@sheetalkamat I don't have reproduction steps for why solution builder's .build wouldn't work. I can enumerate a few problem areas when it comes to ts-loader if it helps:

  • Solution builder builds all files in a project (just like tsc --project) and webpack/ts-loader work on files. I believe this means the current call in ts-loader to .buildReferences will run multiple times if there are multiple Webpack entry values present [0].
  • Each file that runs through a program needs their writeFile and transformers to be set by ts-loader. It looks like this can be achieved via the host or getNextInvalidatedProject but some structure would need to be created to keep track of each emitted file.
  • The current createSolutionBuilderWithWatch can't be used for .build when not in watch mode. I'm not sure why that was selected over createSolutionBuilder.

It sounds like from the wiki[1][2][3][4], from what what Sheetal said, and the reasons above regarding SolutionBuilder, the right path forward for the status quo is to complete the switch to the watch API. The downside is that the watch API doesn't write the tsBuildInfoFile out on completion. Also, ts-loader doesn't currently work on projects. Changing those two seem a bit at odds with one another.

Option A: In ts-loader: Allow users to opt-in to building projects with ts-loader instead of building files. This would require having an intermediary directory set (e.g. /build) and having ts-loader proxy the JS/map files from disk instead of in memory via writeFile. This would allow using the SolutionBuilder instead of the watch API.

Option B: In TypeScript: Allow an incomplete tsBuildInfoFile to be outputted via watch API that has any files the compiler interacted with during that instance. Maybe a method named writeTsBuildInfoFile that allows giving an output file?

0: https://webpack.js.org/concepts/entry-points/#object-syntax
1: https://github.com/microsoft/TypeScript/wiki/Architectural-Overview
2: https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API
3: https://github.com/microsoft/TypeScript/wiki/Using-the-Language-Service-API
4: https://github.com/microsoft/TypeScript-wiki/pull/225/files#diff-709351cd55688fbcb7ec0fc9973ee746

@johnnyreilly
Copy link
Member

Thanks for putting real time and thought into this @mc0. I greatly appreciate that. I'd really like to hear back from @sheetalkamat and @andrewbranch with their thoughts.

I'd like to add a few thoughts into the mix as well:

the right path forward for the status quo is to complete the switch to the watch API

I'd love to do that. As mentioned, AFAIK there's no way as yet to support Vue users with the watch API. So we'd either probably need to keep supporting the existing non watch API mechanism for that use case. We could possibly change the default behaviour to be use the watch API with the ability to fall back to the other behaviour using a feature flag. I'd love the ability to move lock, stock and barrel but that could be a first step.

Options A and B both sound interesting (option A sounds pretty bold but could have merit), I'd love to hear Sheetal and Andrew's views as well.

@andrewbranch
Copy link
Contributor

andrewbranch commented Sep 14, 2019

Sorry if you already mentioned this somewhere, but can you elaborate on the issues with Vue?

Edit: looks like maybe this PR you linked is the best place to read about it: TypeStrong/fork-ts-checker-webpack-plugin#231

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 14, 2019

Yup - I haven't actually tried using ts-loader with watch mode and Vue so I could be wrong. But I'm working on the basis of the experience @phry had with the fork-ts-checker-webpack-plugin. It seemed that special workarounds/ hacks had to be attempted to get the watch API to play ball with Vue due to the different filename issue. I don't know if that issue also presents with ts-loader but it seems likely.

I'm not a Vue user and if there's anyone out there who would like to try ts-loader with Vue and experimentalWatchApi: true and report back that'd be super helpful.

experimentalWatchApi: boolean;

Looking again at the end of the linked thread, there might actually be hope in terms of a workaround. But I'm not close enough to the detail. It looks like @phry got close with a kind of approach and @WolfspiritM provided some useful feedback. But it doesn't look like things have advanced subsequently. There may actually be a way forward here but I'm not clear.

@phry /@WolfspiritM if you've anything to add to this please do! 🥰

cc @yyx990803 just in case this thread proves to be relevant to the Vue CLI which (I think?) uses ts-loader

@yyx990803
Copy link
Contributor

@johnnyreilly thanks for tagging me. Looping in @sodatea who works on our CLI.

@stale
Copy link

stale bot commented Nov 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 14, 2019
@stale
Copy link

stale bot commented Nov 21, 2019

Closing as stale. Please reopen if you'd like to work on this further.

@stale stale bot closed this as completed Nov 21, 2019
@eamodio
Copy link

eamodio commented May 25, 2020

This still seems to be an issue for me, I am using the experimentalWatchApi and setting "incremental": true in my tsconfig.json, but am not seeing any .tsbuildinfo files being generated nor any reduction in compile times. FYI, I am using the latest fork-ts-checker 5.0.0-alpha.5 too

@johnnyreilly
Copy link
Member

I'm not totally up to speed with this, but I have an idea that .tsbuildinfo only comes into play with project references on. I'm assuming that isn't how your project works?

I have a feeling this changed with ts-loader 6.0.0

See conversation between the marvellous @sheetalkamat and @andrewbranch here:

#1076 (comment)

@eamodio
Copy link

eamodio commented May 25, 2020

Yeah, I just have a single project, no project references.

@sheetalkamat
Copy link
Contributor

am using the latest fork-ts-checker 5.0.0-alpha.5 too

From this it looks like you are using ts-loader in transpile only mode so there isnt going to be tsbuild info generated for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants