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

Add support for webpack v5 #83

Closed
wants to merge 26 commits into from
Closed

Add support for webpack v5 #83

wants to merge 26 commits into from

Conversation

jayaddison
Copy link
Contributor

What's the problem this PR addresses?
This pull request attempts to introduce support for webpack v5.

Status

Tests are not yet passing - it looks like there may be some remaining problems related to:

Error formatting

Failure case:

  1) code-splitting
       with static require
         runs successful test:
     Uncaught AssertionError: expected ' WEBPACK  Compiling...\n\n WEBPACK  Compiled successfully in 197ms\n\n MOCHA  Testing...\n\n\n\n  0 passing (0ms)\n\n MOCHA  Tests completed successfully\n\n' to include 'entry1.js'

Code reference:

return {
errors: ensureWebpackErrors(compilation.errors)
.map(formatError)
.map(prependError),
warnings: ensureWebpackErrors(compilation.warnings)
.map(formatError)
.map(prependWarning)
}

Exit code

In order to get the codebase to compile against an updated version of webpack, a workaround was applied which omits the failures count from the exit code of mochapack.

Commit reference:
https://github.com/jayaddison/mochapack/commit/ef3582ea97c5fa85bf2ba4d7eedfa3825b59f00d

How did you fix it?

  • Looked through the webpack v4 to v5 migration guide
  • Upgraded the webpack dependency in package.json
  • Discovered and fixed compile-time errors using npx tsc
  • Discovered and fixed test errors using npx ts-mocha ...

Resolves #82

@larixer
Copy link
Member

larixer commented Oct 11, 2020

@jayaddison Nice, thanks! Could you adjust Travis settings for our tests to include Webpack 5 as well?

@jayaddison
Copy link
Contributor Author

I have to admit to not being experienced enough with either mochapack nor webpack to really understand how to continue to move this forward; I'd appreciate any help, or be glad to see anyone else attempt it. Either way I'll try to find a bit more time soon to re-read the v5 migration instructions and the codebases in more detail.

@larixer
Copy link
Member

larixer commented Oct 29, 2020

@jayaddison Thanks for letting know!

CC
@Jack-Barry @JamesMcMahon

@skjnldsv
Copy link
Contributor

skjnldsv commented Nov 7, 2020

Hello!
How can we help move this forward? :)

@larixer
Copy link
Member

larixer commented Nov 7, 2020

@skjnldsv

Hello!
How can we help move this forward? :)

The Pull Request is needed that implements Webpack 5 support while not breaking any of the current tests. I don't know other ways, feel free to share your ideas.

@skjnldsv
Copy link
Contributor

skjnldsv commented Nov 14, 2020

The Pull Request is needed that implements Webpack 5 support while not breaking any of the current tests. I don't know other ways, feel free to share your ideas.

You mean you need help fixing the unit tests on this pr?

EDIT: fixed a few more errors here https://github.com/jayaddison/mochapack/pull/1
But it's really out of my league ;)

skjnldsv and others added 5 commits November 14, 2020 16:04
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@haoqunjiang
Copy link
Contributor

The code-splitting tests are failing because the chunks are all filtered by this condition:

'ids.filter(inManifest).forEach(run)'

So none of the tests actually runs.

The __webpackManifest__ variable is generated here:

global.__webpackManifest__ = buildStats.affectedModules // eslint-disable-line

So I guess something's wrong with the updated getAffectedModuleIds implementation.

@jayaddison
Copy link
Contributor Author

Something's also broken related to the way that the test runner is invoked; the pull request as it stands uses some poorly-understood modifications to get tsc type-checking to pass, but it isn't really a working implementation.

Taking another look at it currently; hope to learn a bit more about how this is supposed to work.

// Make sure we have a valid error message
if (err.message && typeof err.message === 'string') {
lines.push(formatErrorMessage(err.message))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skjnldsv I wasn't completely sure whether this change is required, so I've backed it out for now (along with some error-related code changes elsewhere in this file).

Copy link
Contributor

Choose a reason for hiding this comment

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

It was just a sanity check on an error I encountered. Where the error was not valid :)

@lveraszto
Copy link

lveraszto commented Feb 6, 2021

Hello!

I tried this branch and have found some things.

  1. I solved the build problem with the exit code with this code, in the src/cli/index.ts:
    .then((failures: number | void) => { exit(cliOptions.mocha.cli.exit, failures || 0) })
    So it's not a problem that watch returns Promise<void>.

  2. I tried to fix some unit tests and realised that it looks like in some cases mocha did not get the test file path.

  • forbidOnly.test.ts: if i run only this test and log out the mocha output in the callback, i see that 0 test was running.
    it.only('gets really angry if there is an only in the test', function(done) { exec(`node ${binPath} --mode development --forbid-only "${test}"`, (err, output) => { console.log(output) assert.isNotNull(err) done() }) })
    To make this test running again I added the --file to the command.
    it.only('gets really angry if there is an only in the test', function(done) { exec(`node ${binPath} --mode development --forbid-only --file "${test}"`, (err, output) => { console.log(output) assert.isNotNull(err) done() }) })
    And now the test is working and i see the proper Error: `.only` forbidden log in my console. (The exit code fix is required too for this test)
    The question is why mocha can't see the test file without --file?

  • code-splitting.test.ts: Here i logged the output again, and i get the same result, no tests are running. So i added the --file option. After this mocha tried to run the test but it failed with this error:
    TypeError: require.ensure is not a function

@jayaddison
Copy link
Contributor Author

@lveraszto Thanks a lot for helping out! If you feel like pushing your fix from item (1) to a git repo, I'd be glad to merge that into the branch; or I could commit it locally and credit you via the commit author list if that's preferable.

The --file parameter sounds like a good clue as well. I'll try to take some time during the week to debug that further.

@verasztol
Copy link
Contributor

Thanks for your work @jayaddison . I created a PR (https://github.com/jayaddison/mochapack/pull/2)

@jayaddison
Copy link
Contributor Author

Oh, sorry - I didn't spot the pull request. I'll check my notification settings. Thanks!

fix exit code build problem for watch return type
@jmariller
Copy link

Hi all

First of all many thanks for your efforts to ensure this library will get webpack 5 support, really appreciated!

Do you have any news on the progress for this, or do you need any additional help?

@jayaddison
Copy link
Contributor Author

Hi @jmariller - thanks! I've no recent updates here, and have not yet had time to review the latest ideas regarding the --file flag. Any help (either with that investigation, or more generally with the upgrade) would be much appreciated.

@jayaddison
Copy link
Contributor Author

It seems unlikely I'm going to be able to commit more time to this in the near future, sorry about that for anyone waiting.

If anyone's lurking with additional ideas or progress towards completing the upgrade process, let's try to incorporate those soon?

@jayaddison jayaddison closed this Mar 13, 2021
@jayaddison jayaddison deleted the upgrades/webpack-5 branch March 13, 2021 19:01
@danilofuchs
Copy link

Was this PR canceled entirely or will it be continued later?

@jayaddison
Copy link
Contributor Author

@danilofuchs Currently I've no plans to continue it, but that doesn't prevent anyone else picking it up. I didn't feel I had a strong enough grasp of the codebase to get this to completion.

@haoqunjiang
Copy link
Contributor

I've opened a new PR that have got most tests passing #98

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.

[Feature] Webpack v5 compatibility
8 participants