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

Update webpack-stream to fix vulnerabiliy reported by npm and dedupe Webpack #11436

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Update webpack-stream to fix vulnerabiliy reported by npm and dedupe Webpack #11436

merged 1 commit into from
Dec 23, 2019

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Dec 22, 2019

https://npmjs.com/advisories/1084

webpack-stream between 4.0.3 and 5.0.0 added official support for Webpack 4. Which is nice since we ARE using Webpack 4... Also, dropped support for Node.js 4 which shouldn't be a big deal for us since we are already using packages that are incompatible with Node.js 4 (Webpack 4.x supports Node.js 6 and up).

A nice side effect is that this update removes a ton of duplicate dependencies - webpack-stream 4.0.3 included a second copy of Webpack, version 3.x, in our node_modules!

@wojtekmaj wojtekmaj changed the title Update webpack-stream to fix vulnerabiliy reported by npm Update webpack-stream to fix vulnerabiliy reported by npm and dedupe Webpack Dec 22, 2019
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/428dca951cd5dbf/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/428dca951cd5dbf/output.txt

Total script time: 0.68 mins

Published

@timvandermeij
Copy link
Contributor

I wonder why it showed success, because the build failed. From the logs:

You ran Webpack twice. Each instance only supports a single concurrent compilation at a time.

The preview commands runs gulp web, so I think this update changed something in the build process. Does this also happen locally?

@wojtekmaj
Copy link
Contributor Author

Looks like there has been a regression in webpack-stream 5.1.0 and up; current solution (perfectly satisfactory for this PR) is to upgrade to 5.0.x, not all the way.

https://npmjs.com/advisories/1084

webpack-stream between 4.0.3 and 5.0.0 added official support for Webpack 4. Which is nice since we ARE using Webpack 4... Also, dropped support for Node.js 4 which shouldn't be a big deal for us since we are already using packages that are incompatible with Node.js 4 (Webpack 4.x supports Node.js 6 and up).
@Snuffleupagus
Copy link
Collaborator

Does this also happen locally?

For future reference: When making changes that could, in one way or another, potentially break the builds you should run all of the appropriate tests locally while developing the patch.
In this case that would entail running gulp generic and checking that the build works, and obviously also running the complete test-suite with gulp test to ensure that all tests pass.

Looks like there has been a regression in webpack-stream 5.1.0 and up;

That seems to be tracked in issue shama/webpack-stream#201, and the regression is apparently PR shama/webpack-stream#109, which has been open for over a year :-(

If that regression isn't addressed, we may need to start looking for alternatives to webpack-stream...

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/02863f40c7d462e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/02863f40c7d462e/output.txt

Total script time: 1.72 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/25b3b8329f0f2a1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/f143909fee3de74/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/25b3b8329f0f2a1/output.txt

Total script time: 18.95 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/f143909fee3de74/output.txt

Total script time: 26.31 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/f143909fee3de74/reftest-analyzer.html#web=eq.log

@wojtekmaj
Copy link
Contributor Author

That failure looks invalid to me!

@timvandermeij timvandermeij merged commit 934d7cb into mozilla:master Dec 23, 2019
@timvandermeij
Copy link
Contributor

Yes, it's completely unrelated to this patch. Nice clean-up!

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.

4 participants