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

Emitting .tsbuildinfo when using watch api #1017

Merged
merged 5 commits into from
Sep 27, 2019

Conversation

sheetalkamat
Copy link
Contributor

Updated from #1012

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 26, 2019

Hey @sheetalkamat,

Amazing work! I've been thinking about a good way forward from here. My initial thoughts were that it would be good to get tests running using both watch API and not

I started work on 1. and after making some progress I've had a rethink. Whilst we could get the tests working in both modes, doing so would come at the cost of increased complexity in the test packs. The complexity in the test packs is already higher than I'd like and so I'm somewhat reticent to proceed with that. Hence, rethink 😄

I've taken the execution tests for a spin with incremental mode on and by the looks of it they're all green with 2 exceptions: optionsCaching and pnpm.

They don't fail meaningfully on CI but at least on Windows they don't appear to be reporting. Travis / Linux is fine. Could you have a quick look at those please?

They trigger a weird JScript blocking popup on Windows mentioning a character error in webpack.js - very esoteric.

Once those are resolved one way or the other my thoughts are:

  1. Let's get this merged and released as 6.2.0
  2. I'll subsequently look to do a breaking changes release (v7.0.0 probably) which renames the flag from experimentalWatchApi to watchApi and makes that the default experience for ts-loader. This will mean people use this unless they opt out. I'm rather expecting that this could end up surfacing some issues - that's fine. If the issues are truly problematic then people have a way back.

There's a possibility that as I'm putting together a PR for 2. problems may present; but hopefully not. If you can help out in the event that that happens that would be great.

How does this sound to you?

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 26, 2019

I am not that familiar with execution tests so don't know how to make test run in both modes, but I tried running optionsCaching and pnpm on windows machine and even without experimentalWatchAPI and they pass (so I haven't made any changes to the tests) but in the end they open node_modules\webpack\bin\webpack.js file in VS which leads me to believe that there is something wrong with the execSync command since on my machine its set to open the js file and not run it..
I fixed that part with 62bfb2a and then I ran both the tests without any change and they passed.
Later I manually changed the webpack.config.js for both tests to run with the experimentalWatchApi: true and they pass too. ( i am running it on windows machine)

@johnnyreilly
Copy link
Member

Cool - thanks @sheetalkamat; sounds good. By the looks of it the appveyor (windows) tests seem happy now which is a good sign.

I'm in a 24 hour hackathon right now and so may not be too active online for a while. (Am learning the hard way that react native is hard work 😄 )

I'll try and check on this later this weekend if I can. By the looks of this we may be ready to merge and release as 6.2.0. Awesome job! ❤️ 🌻

@johnnyreilly johnnyreilly merged commit ce39c25 into master Sep 27, 2019
@johnnyreilly johnnyreilly deleted the incrementalUsingWatchApi branch September 27, 2019 17:08
@johnnyreilly
Copy link
Member

Right - once #1018 is merged (GitHub actions migration to new syntax PR) we'll look to get this shipped. I'll look to make some noise about this on Twitter to encourage people to opt in into trying to use this.

@johnnyreilly
Copy link
Member

Shipped with 6.2.0 - thanks @sheetalkamat!

I'd love to write a blog post to promote this and get people to opt into using this before doing the 7.0.0 switchover of defaulting people to use the watch API.

On that I just wanted to clarify the following points with you before I write the post. As I understand it, with this release:

  1. ts-loader will both emit and consume the .tsbuildinfo artefact when run using the experimentalWatchApi: true option.

  2. This applies both when using project references and not.

  3. The net result of people using experimentalWatchApi: true should be faster cold starts in build time where a previous compilation has taken place.

Is that about right? Please do correct / adjust as necessary!

Thanks again!

@sublimator
Copy link

Woohoo, thanks @sheetalkamat !

@sheetalkamat
Copy link
Contributor Author

@johnnyreilly Yes you have covered it correctly. Thanks for getting this in.

@johnnyreilly
Copy link
Member

Excellent - thanks so much @sheetalkamat! I'll put out a blog and try to get people to give this a whirl. Thanks for all your hard work ❤️🌻

@johnnyreilly
Copy link
Member

Blog out: https://blog.johnnyreilly.com/2019/09/start-me-up-ts-loader-meet-tsbuildinfo.html 😄

@johnnyreilly
Copy link
Member

Hey @sheetalkamat

I got a comment on the blog from https://www.twitter.com/danielgut

doesn't seem to work for me. using ts-loader 6.2.0 and enabling experimentalWatchApi doesn't create a tsbuildinfo-file? where should this have been saved? are there some other settings in the tsconfig which need to be set (incremental? though doesn't make a difference here)

It looks like he's experiencing problems. I suspect he may not be using project references - Daniel can you confirm please? In fact a repro repo would be helpful

Should .tsbuildinfo definitely be produced / consumed when project references are not in use?

@sheetalkamat
Copy link
Contributor Author

If project has tsconfig options composite or incremental set to true, you should get .tsbuildinfo file.

@johnnyreilly
Copy link
Member

Ah cool - thanks @sheetalkamat. I'll update the blogpost

@OneCyrus
Copy link

OneCyrus commented Oct 1, 2019

can't get this to do anything at all. setting incremental or composite doesn't change anyhting. also setting tsBuildInfoFile and outDir didn't change anything. typescript is on 3.6.3

@OneCyrus
Copy link

OneCyrus commented Oct 1, 2019

executing just yarn tsc generates the tsconfig.tsbuildinfo

@johnnyreilly
Copy link
Member

Thanks for the feedback @OneCyrus. It's interesting that you get nothing at all. I wonder if you could compare your setup with some of the comparison tests that @sheetalkamat put together here: https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests

image

As you'll be able to see, these are generating .tsbuildinfo files; so understanding the difference between your setup and the examples in our test suite would be super helpful.

@OneCyrus
Copy link

OneCyrus commented Oct 1, 2019

couldn't find a incremental example there. but one thing which is different is the options: { projectReferences: true } setting in the webpack-config. is this something which is needed for incremental? at least it changes the behaviour here but somehow doesn't do anything at all in the end.

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 1, 2019

Good question .... I think @andrewbranch needed to have the projectReferences option in the early days of project references support. It may now be safe to remove but I'm not sure. Maybe he can advise?

I guess it makes sense in that this only works if project has tsconfig options composite or incremental set to true

@lorenzodallavecchia
Copy link
Contributor

lorenzodallavecchia commented Oct 1, 2019

I have the same problem: no .tsbuildinfo is created.

I am not using project references, but I have added projectReferences: true as suggested by @OneCyrus. I also enabled the experimentalWatchApi flag and set incremental: true in tsconfig.json.

I'm launching webpack via webpack-dev-server: maybe that is the culprit?

@kwokkan
Copy link

kwokkan commented Oct 1, 2019

@OneCyrus , @laurelnaiad

Do you have

const { CleanWebpackPlugin } = require("clean-webpack-plugin");

{
    plugins:[
        new CleanWebpackPlugin(),
    ]
}

or similar in your webpack.config.js?

I removed that plugin and i get a tsconfig.buildinfo in my out folder.

I also had to clean out the cache used by cache-loader.

@OneCyrus
Copy link

OneCyrus commented Oct 1, 2019

after further testing it looks like transpileOnly: true disables the tsbuildinfo here.

@OneCyrus
Copy link

OneCyrus commented Oct 1, 2019

it's still very spotty. i can't really build the info-file reliable. also couldn't get to write it with webpack-dev-server at all.

also can't see performance improvements after successive builds. the initial build is about 43s while the second build is like 42s.

@sheetalkamat
Copy link
Contributor Author

@OneCyrus Please share a repro so i can investigate whats going on.. Also please share small repro that's easier to debug.. I just commented at #1005 (comment) where the repro given is too large, but i could see that issue arises with or without transpileOnly set to true

@sheetalkamat
Copy link
Contributor Author

Ok I think I found scenario that isn't working.. 12b586c Looking into this.

@OneCyrus
Copy link

OneCyrus commented Oct 3, 2019

I'll try to create a repro. I guess a lot of the issues are around that the buildinfo-file is flowing through the regular webpack pipeline. so it is kept in memory when using webpack-dev-server or the babel-cache is keeping it from updating.

@OneCyrus
Copy link

@sheetalkamat i made a repo with some examples: https://github.com/OneCyrus/typescript-incremental

the main problem is that i can't get this to work with webpack / webpack-dev-server. with a plain tsc it writes the file.

@sheetalkamat
Copy link
Contributor Author

@OneCyrus it seems you have transpileOnly: true . That mode doesn't generate tsbuildinfo

@OneCyrus
Copy link

ok without transpileOnly the simple webpack version works. the one with webpack server still doesn't write the file to disk.

another issue i have is the requirement of noEmit: false. we usually don't emit with typescript as we just want the webpack output and don't need the additional files typescript generates.

ERROR in ./index.tsx
Module build failed (from ../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for C:\Source\typescript-incremental\src\index.tsx.
    at makeSourceMapAndFinish (C:\Source\typescript-incremental\node_modules\ts-loader\dist\index.js:80:15)
    at successLoader (C:\Source\typescript-incremental\node_modules\ts-loader\dist\index.js:68:9)
    at Object.loader (C:\Source\typescript-incremental\node_modules\ts-loader\dist\index.js:22:12)

also declaration: false doesn't work as this is a requirement for incremental.

ERROR
      TS6304: Composite projects may not disable declaration emit.

i've updated the repo with thbe transpile change.

@OneCyrus
Copy link

just tried benchmarking it. bascially running the webpack build 3 times while starting without a tsbuild-file. so basically I expected the first run to take longer than the 2nd or 3rd. but it makes no difference.

PS C:\Source\typescript-incremental> 1..3 | % { (Measure-Command { yarn webpack --mode development }).TotalSeconds }
18.8426215
17.2009899
18.7899734

@thasmo
Copy link

thasmo commented Nov 20, 2019

@johnnyreilly @sheetalkamat I'm having a similar experience as @OneCyrus using webpack 5 beta.6 - successive builds take the same time to build.

Running successive production builds with experimentalWatchApi: true, which creates the tsconfig.tsbuildinfo file on the first run, always take 20 seconds. When running webpack using --watch the build always takes 8 seconds, regardless the existence of the tsconfig.tsbuildinfo file.

Creation of the tsconfig.tsbuildinfo file seems to be working fine in watch mode when using experimentalWatchApi: true, but I'm wondering if ts-loader actually consumes it when running the typescript compiler as I can't seem to benefit of faster builds in any case.

@sheetalkamat
Copy link
Contributor Author

@thasmo your issue could be same as #1017 (comment) and I am working on the fix and currently blocked on feedback from webpack expert for some of the questions...

@thasmo
Copy link

thasmo commented Nov 20, 2019

@sheetalkamat, I understand. Thanks for the quick response.

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.

7 participants