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

Rewrite to version 5.0.0 #404

Merged
merged 29 commits into from May 23, 2020
Merged

Rewrite to version 5.0.0 #404

merged 29 commits into from May 23, 2020

Conversation

piotr-oles
Copy link
Collaborator

@piotr-oles piotr-oles commented Apr 18, 2020

As I promised a few weeks ago, I've performed a rewrite of the plugin to pay off technical debt and open this plugin for future development. This is a draft PR to show you the progress.

Tasks:

This release should fix also: #309, #303, #273

Plan:

  1. release first alpha version when unit tests will be ready to have at least minimal test coverage.
  2. release second alpha version when integration test will be ready to have it stable
  3. update documentation
  4. go through the rest of checklist items, implement it in a separate PRs and release

I'm not sure if I will go through all these points before releasing stable 5.0.0 - some of these points can be implemented later 🚀

@piotr-oles piotr-oles self-assigned this Apr 18, 2020
@piotr-oles piotr-oles requested review from johnnyreilly and phryneas and removed request for johnnyreilly April 18, 2020 10:29
Copy link
Member

@johnnyreilly johnnyreilly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work so far @piotr-oles!

cc @sheetalkamat and @andrewbranch - I thought you might be interested in the work Piotr has been putting into making fork-ts-checker-webpack-plugin project references and tsbuildinfo friendly ☺️

src/error/OperationCancelledError.ts Outdated Show resolved Hide resolved
src/ForkTsCheckerWebpackPluginOptions.json Show resolved Hide resolved
src/hooks/tapDoneToAsyncGetIssues.ts Outdated Show resolved Hide resolved
src/hooks/tapAfterCompileToGetIssues.ts Outdated Show resolved Hide resolved
src/reporter/FilesChange.ts Outdated Show resolved Hide resolved
src/rpc/rpc-ipc/RpcIpcMessagePort.ts Show resolved Hide resolved
src/typescript-reporter/assertTypeScriptSupport.ts Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Member

Yeah this is looking really promising! 🤗

Release new patch versions for "refactor" and "docs" commits.
@piotr-oles piotr-oles force-pushed the feature/refactor-to-version-5 branch from 82962c7 to 5f5baab Compare May 16, 2020 15:57
)

Update code-frame dependency from v6 to v7 so that we can drop the chalk v1 dependency.

Closes #408
@piotr-oles piotr-oles force-pushed the feature/refactor-to-version-5 branch 11 times, most recently from 7deeb02 to 25e1e54 Compare May 20, 2020 15:06
@piotr-oles piotr-oles force-pushed the feature/refactor-to-version-5 branch from af85143 to 1af07ae Compare May 20, 2020 21:50
@piotr-oles piotr-oles force-pushed the feature/refactor-to-version-5 branch from b7d26d0 to 4479864 Compare May 21, 2020 19:43
@piotr-oles
Copy link
Collaborator Author

Hi @johnnyreilly , I implemented most of the points from the checklist, wrote end-to-end tests, and updated the documentation. Could you re-review this PR?

@piotr-oles piotr-oles force-pushed the feature/refactor-to-version-5 branch from 4d6b5fb to 8247ef8 Compare May 23, 2020 09:54
@piotr-oles
Copy link
Collaborator Author

As this is an only alpha release and all tests passed, I will merge it :)

@johnnyreilly , @phryneas I know it's a big PR, so please review it when you have more time and if you have any comments, I will address them in the next alpha releases.

@piotr-oles piotr-oles merged commit 1d22d62 into alpha May 23, 2020
Copy link
Contributor

@phryneas phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is huuuge 😄
Congrats to that big hunk of work you pulled off there - very impressive!

Is's not really possible to review this as a diff, so I just took a deep dive at the source code.

I really like the new architecture, it's a lot cleaner - great job on that 👍
After reading it, I have to say: I understand what's going on, but if there are any bugs, I probably missed those because I'm not too accustomed to the code base and also haven't had a lot of time to spend with webpack in the recent past. So my review comments are primarily of a more generic nature.

I'll definitely start dogfooding this as soon as I get back into our react project (doing a lot of infra at the moment) and will report back if I find any anomalies there.

src/index.ts Show resolved Hide resolved
src/ForkTsCheckerWebpackPlugin.ts Show resolved Hide resolved
src/hooks/tapStopToDisconnectReporter.ts Show resolved Hide resolved
function getChangedFiles(compiler: webpack.Compiler): string[] {
let changedFiles: string[] = [];

if ((compiler as any).modifiedFiles) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, there is a lot of compiler as any going on and that makes some stuff hard to read. Maybe it would make sense to define some interface extending webpack.Compiler and cast that at a central location once or twice?

Something like

type Webpack4Or5 = webpack.Compiler & ({
// webpack 5 stuff
} | {
// webpack 4 stuff
})

The compiler should be able to use the relevant properties as discriminator from there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try that :)

@johnnyreilly
Copy link
Member

Just wanted to share these timings that @Timer was kind enough to do here: vercel/next.js#13529 (comment)

Base build: 7s (no TypeScript features enabled)

fork-ts-checker-webpack-plugin@3.1.1: 90s, computer sounds like an airplane
fork-ts-checker-webpack-plugin@4.1.6: 84s, computer did not sound like an airplane
fork-ts-checker-webpack-plugin@5.0.0-alpha.14: 90s, regressed
npx tsc -p tsconfig.json --noEmit: 12s (time: 18.57s user 0.97s system 169% cpu 11.525 total)

@weyert
Copy link

weyert commented May 29, 2020

John, do you think this is due the rewrite to TypeScript or has more changed during the rewrite? I can imagine the TypeScript transpilling does some more runtime checks

@piotr-oles
Copy link
Collaborator Author

🎉 This PR is included in version 5.0.0-alpha.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@piotr-oles
Copy link
Collaborator Author

🎉 This PR is included in version 5.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

6 participants