Skip to content

Conversation

@haoqunjiang
Copy link
Contributor

What's the problem this PR addresses?

Continues the work on #83

How did you fix it?

By fixing the isBuilt function.


Other thoughts:

  1. Haven't checked webpack 4 compatibility. Shall I work on that or will this PR be released in a new major?
  2. The 2 failing tests are caused by the usage of memory-fs in mochapack. It stops the devServer.writeToDisk option from writing actual files to the disk. In the past, it's handled by write-file-webpack-plugin, but the plugin is not compatible with webpack 5. Shall I add an additional option to the CLI just to fix the tests?

jayaddison and others added 30 commits October 11, 2020 14:28
TODO: Determine how to retrieve failure count
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
fix exit code build problem for watch return type
The `built` flag in webpack 5 is moved:
webpack/webpack@eb63cf8
The `esModule` option is turned on by default in v3
@haoqunjiang haoqunjiang mentioned this pull request May 11, 2021
4 tasks
@larixer
Copy link
Member

larixer commented May 11, 2021

  1. Haven't checked webpack 4 compatibility. Shall I work on that or will this PR be released in a new major?

I would be grateful if webpack 4 compatibility were retained in this PR, otherwise, it will be a maintenance nightmare to keep up to date both new major and old major to support users of webpack 4 and webpack 5 at the same time

  1. The 2 failing tests are caused by the usage of memory-fs in mochapack. It stops the devServer.writeToDisk option from writing actual files to the disk. In the past, it's handled by write-file-webpack-plugin, but the plugin is not compatible with webpack 5. Shall I add an additional option to the CLI just to fix the tests?

I think env variable would be better, instead, no need to pollute CLI and its enough to have just short notice in the source code why it is needed.

Btw, why have you decided to continue work on Webpack 5 support here, what about instant-mocha, didn't it cover your needs?

@haoqunjiang
Copy link
Contributor Author

Btw, why have you decided to continue work on Webpack 5 support here, what about instant-mocha, didn't it cover your needs?

It failed my tests with very obscure error messages. I have no idea how to fix or even reproduce the issue.

On the other hand, I've had a rough idea on how to fix the previous PR for a while, so I give it a try.

@haoqunjiang
Copy link
Contributor Author

Done! All tests are passing.

In order to reduce the mental burden, though the implementations of the two versions are similar, I put them into different files:

  1. getAffectedModuleIds.ts & getBuildStats.ts for webpack 5
  2. webpack4GetAffectedModuleIds.ts & webpack4GetBuildStats.ts for webpack 4

I've also added two scripts in the TravisCI config to skip TypeScript/ESLint checking for unrelated files.

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 14.89362% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.93%. Comparing base (6240eeb) to head (b30cc73).
⚠️ Report is 27 commits behind head on master.

Files with missing lines Patch % Lines
src/webpack/util/webpack4GetAffectedModuleIds.ts 12.24% 43 Missing ⚠️
src/webpack/util/getAffectedModuleIds.ts 15.38% 33 Missing ⚠️
src/webpack/util/webpack4GetBuildStats.ts 19.44% 29 Missing ⚠️
src/webpack/util/getBuildStats.ts 0.00% 5 Missing ⚠️
src/runner/TestRunner.ts 40.00% 3 Missing ⚠️
src/webpack/loader/entryLoader.ts 0.00% 3 Missing ⚠️
src/webpack/compiler/registerInMemoryCompiler.ts 0.00% 2 Missing ⚠️
src/webpack/compiler/registerReadyCallback.ts 0.00% 2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   65.87%   60.93%   -4.95%     
==========================================
  Files          42       44       +2     
  Lines        1225     1349     +124     
  Branches      137      158      +21     
==========================================
+ Hits          807      822      +15     
- Misses        400      509     +109     
  Partials       18       18              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@larixer
Copy link
Member

larixer commented May 12, 2021

@sodatea Awesome, thank you so much! I'm going to review the PR shortly

@larixer larixer merged commit c4806e8 into sysgears:master May 12, 2021
@larixer
Copy link
Member

larixer commented May 12, 2021

@sodatea Merged your PR and published [email protected]. Thanks again for your awesome work!

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.

6 participants