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

Replace fork-ts-checker-webpack-plugin with faster alternative #13529

Merged
merged 4 commits into from
May 29, 2020

Conversation

Timer
Copy link
Member

@Timer Timer commented May 29, 2020

This removes fork-ts-checker-webpack-plugin and instead directly calls the TypeScript API.

This is approximately 10x faster.

Base build: 7s (no TypeScript features enabled)

  • [email protected]: 90s, computer sounds like an airplane
  • [email protected]: 84s, computer did not sound like an airplane
  • [email protected]: 90s, regressed
  • npx tsc -p tsconfig.json --noEmit: 12s (time: 18.57s user 0.97s system 169% cpu 11.525 total)
  • This PR: 22s, expected to get better when we run this as a side-car

All of these tests were run 3 times and repeat-accurate within +/- 0.5s.

@Timer Timer added this to the 9.4.5 milestone May 29, 2020
@Timer Timer force-pushed the enhancement/fast-type-check branch from 1783384 to 27fbf43 Compare May 29, 2020 03:27
@Timer Timer marked this pull request as ready for review May 29, 2020 04:40
@Timer Timer requested review from ijjk, lfades and timneutkens as code owners May 29, 2020 04:40
@johnnyreilly
Copy link

johnnyreilly commented May 29, 2020

You will no doubt will (and should!) pick the approach that works best for you. Couple of things to note in case there are properties of the fork-ts-checker-webpack-plugin that are directly useful to the project (eg the "webpack awareness" / support for eslint etc):

Of course, please make the calls that work for your project, I just wanted to let you know some possibilities 🤗

@Timer
Copy link
Member Author

Timer commented May 29, 2020

@johnnyreilly Thanks for reaching out!

We were pretty happy with the plugin overall, but there's a few things other than speed that motivated this change:

  • We do not run type checking in development, only in production (so we use sync mode, not async, and no watching).
  • We want to run type checking before the webpack build, and skip it [webpack] if checks fail. This gives a faster turnaround time IMO.
  • We firmly believe in keeping the webpack build's responsibilities as small as possible. We're going to start moving as many things as we can into side-cars, completely off the Node.js event loop (this PR is the first step).

Thanks for mentioning that newer versions may have seen speed improvements!
Since this work is already done, we'll probably stick with it.

To elaborate more on the speed we were observing:
next build + fork-ts-checker-webpack-plugin = 86s
next build with no TS checks = 6s
tsc -p tsconfig.json = 10s

I may have some spare time tomorrow to see what the speed is like on the new version(s).

@johnnyreilly
Copy link

johnnyreilly commented May 29, 2020

Thanks for the detailed response @Timer! It does sound like you're planning some pretty hefty (and cool sounding!) architectural changes. Super interesting that you don't run type checking in development - that's where I particularly appreciate it!

We want to run type checking before the webpack build, and skip it [webpack] if checks fail. This gives a faster turnaround time IMO.

This is a really interesting idea. I'm very curious as to how it works out.

We firmly believe in keeping the webpack build's responsibilities as small as possible. We're going to start moving as many things as we can into side-cars, completely off the Node.js event loop (this PR is the first step).

Super interesting - as the name suggests, the initial motivation for fork-ts-checker-webpack-plugin was exactly that: "Webpack plugin that runs TypeScript type checker on a separate process."

Sounds like you're going all in on this. Fascinating.

If you do get a chance to compare v4 and v5 alpha performance we'd love to get an idea of the numbers.

Thanks again for sharing the ideas here - really appreciate it!

@Timer
Copy link
Member Author

Timer commented May 29, 2020

@johnnyreilly I just ran some fresh tests on the latest version of our project:

Base build: 7s (no TypeScript features enabled)

  • [email protected]: 90s, computer sounds like an airplane
  • [email protected]: 84s, computer did not sound like an airplane
  • [email protected]: 90s, regressed
  • npx tsc -p tsconfig.json --noEmit: 12s (time: 18.57s user 0.97s system 169% cpu 11.525 total)
  • This PR: 22s, expected to get better when we run this as a side-car

All of these tests were run 3 times and repeat-accurate within +/- 0.5s.


It seems v4 helped (and doesn't burn CPU), but v5 re-regressed. This PR is still beating the plugin by 60+ seconds.

Would love to go more in-depth sometime to figure out the performance bottleneck!

@johnnyreilly
Copy link

Thanks @Timer - that's tremendously helpful! @piotr-oles it looks like there's a performance degradation with v5 - see the numbers from @Timer above.

@Timer
Copy link
Member Author

Timer commented May 29, 2020

Thank you again for reaching out! ❤️ 🚀


Super interesting that you don't run type checking in development - that's where I particularly appreciate it!

Missed this bit!

We disabled it because users (and us) experienced slower dev-time compilations, despite using async: true. The plan is to re-introduce development time checks with this new setup!

I'd be happy to help/collab to figure out the upstream issue though so we don't have to maintain this ourselves. We're also exploring ESLint in the future, so this all parlays.

@johnnyreilly
Copy link

We're also exploring ESLint in the future too, so this all parlays.

Did you use the ESLint integration in fork-ts-checker-webpack-plugin BTW? My own use case is running fork-ts-checker-webpack-plugin to surface up both type checking errors from TypeScript and lints from ESLint.

@Timer
Copy link
Member Author

Timer commented May 29, 2020

We did not: strictly the TypeScript integration. Next.js doesn't (currently) ship with any form of ESLint (not even eslint-loader).

The DX we're aiming for (in dev) is that TS/ESLint errors show up in a toast though, and not a full overlay ("compile error").


For posterity, here was our config:

new _forkTsCheckerWebpackPlugin.default(
  _pnpWebpackPlugin.default.forkTsCheckerOptions({
    typescript: typeScriptPath,
    async: false,
    useTypescriptIncrementalApi: true,
    checkSyntacticErrors: true,
    tsconfig: tsConfigPath,
    reportFiles: ["**", "!**/__tests__/**", "!**/?(*.)(spec|test).*"],
    compilerOptions: { isolatedModules: true, noEmit: true },
    silent: true,
    formatter: "codeframe",
  })
);

Just tested: removing _pnpWebpackPlugin.default.forkTsCheckerOptions did not affect the time it took to run.

@johnnyreilly
Copy link

johnnyreilly commented May 29, 2020

The DX we're aiming for (in dev) is that TS/ESLint errors show up in a toast though, and not a full overlay ("compile error").

Yeah that was exactly why I wrote https://github.com/johnnyreilly/fork-ts-checker-notifier-webpack-plugin

Though I wanted all lints / errors as notifications / toasts.

Something very lovely about having errors as toasts outside of the application. I've never really enjoyed the full overlay approach; I like to have the application running even if it does contain errors. I find value in being able to debug a broken application - particularly if it's a type system error which may not massively impair functionality.

@johnnyreilly
Copy link

johnnyreilly commented May 29, 2020

Incidentally, I've an idea that isolatedModules: true may hurt your performance and be unnecessary now. Could be wrong about that - have an idea something changed around TypeScript 3.6 / 3.7.

skipLibCheck: true could improve your performance (regardless of context).

Incidentally, skipLibCheck: true is going to become the default as of TypeScript 3.9

https://www.twitter.com/orta/status/1260583041740812288 announced by @orta

@piotr-oles
Copy link

@Timer Thanks for the benchmark, I will try to investigate why there is such a big difference between tsc and fork-ts-checker-webpack-plugin since we use the same API 😃

@timneutkens
Copy link
Member

skipLibCheck: true could improve your performance (regardless of context).

It's already true in the benchmarks @Timer did (so didn't affect the slowdown)

@kodiakhq kodiakhq bot merged commit 92a12a2 into vercel:canary May 29, 2020
@piotr-oles
Copy link

@Timer Could you tell me how can I check timings on my local machine? I cloned the repository and checkout master branch but I'm not sure which command I should run to use fork-ts-checker-webpack-plugin :)

@Timer Timer deleted the enhancement/fast-type-check branch May 29, 2020 15:12
@Timer
Copy link
Member Author

Timer commented May 29, 2020

@piotr-oles the project we're testing with is a private one (that powers our website), happy to provide anything you need to see tho (minus full repo).

@piotr-oles
Copy link

@Timer Ok, I will try on different repository :) Could you tell me which version of the typescript did you use for tests?

@timneutkens
Copy link
Member

    "typescript": "3.7.2"

@piotr-oles
Copy link

Thanks :) If someone has a project that can share and that is significantly slower with fork-ts-checker-webpack-plugin than with tsc, it would be very helpful for testing 😃

@piotr-oles
Copy link

I did some research regarding plugin performance and these are outcomes:

@timneutkens
Copy link
Member

Awesome @piotr-oles 🚀

rokinsky pushed a commit to rokinsky/next.js that referenced this pull request Jul 11, 2020
…cel#13529)

This removes `fork-ts-checker-webpack-plugin` and instead directly calls the TypeScript API.

This is approximately 10x faster.

Base build: 7s (no TypeScript features enabled)

- `[email protected]`: 90s, computer sounds like an airplane
- `[email protected]`: 84s, computer did **not** sound like an airplane
- `[email protected]`: 90s, regressed
- `npx tsc -p tsconfig.json --noEmit`: 12s (time: `18.57s user 0.97s system 169% cpu 11.525 total`)
- **This PR**: 22s, expected to get better when we run this as a side-car

All of these tests were run 3 times and repeat-accurate within +/- 0.5s.
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants