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 watchify package to v4.0.0 #226

Merged
merged 2 commits into from Jun 20, 2021
Merged

Update watchify package to v4.0.0 #226

merged 2 commits into from Jun 20, 2021

Conversation

albertyw
Copy link
Contributor

@albertyw albertyw commented Jun 14, 2021

watchify v3 has indirect security vulnerabilities. watchify v4 resolves those vulnerabilities and has minimal incompatibilities.

Watchify changelog

Also updates puppeteer to fix builds with node v16. Puppeteer changelog

@albertyw albertyw force-pushed the npm-update branch 2 times, most recently from afd97b8 to e857da6 Compare June 14, 2021 03:04
@m90
Copy link
Collaborator

m90 commented Jun 14, 2021

Thank you for this PR.

Also updates puppeteer to fix builds with node v16.

The last build (https://github.com/mantoni/mochify.js/runs/2743568610) passed in Node 16 using Puppeteer 9. Could you elaborate on what exactly is breaking here? Updating puppeteer in Mochify can introduce very subtle breakages from time to time...

@albertyw
Copy link
Contributor Author

Did puppeteer 9 build correctly? From https://github.com/mantoni/mochify.js/actions, build 6 and 7 were only with the watchify update and both failed. build 8 included the puppeteer update and succeeded. A weird thing I noticed for both build 6 and 7 was that

  • on npm ci, there's no Chromium (869685) downloaded to /home/runner/work/mochify.js/mochify.js/node_modules/puppeteer/.local-chromium/linux-869685
  • on npm test, there's a warning Error: Could not find expected browser (chrome) locally. Run npm install to download the correct Chromium revision (869685).

@m90
Copy link
Collaborator

m90 commented Jun 14, 2021

Did puppeteer 9 build correctly?

Ah ok, I didn't get that you were referring to this branch, sorry. I was thinking the Puppeteer upgrade was a drive-by and not a necessity in the context of the PR. I was referring to the latest build on current master.

Anyways, the situation seems mysterious: when I checkout 2a4f0a8 from your branch locally I can't even npm ci, it just stalls. When I run npm i it installs, but it will do things I do not understand to package-lock.json (i.e. changes). In this case, however, the tests pass just fine in Node 16 as well. I would assume the Puppeteer update is therefore not strictly necessary and its side effect of updating the lockfile somehow fixed the situation. npm fun 💫

Do you think it would make sense to try again with updating watchify only and having tests pass in all Node versions?

As for updating Puppeteer it would make sense as well, but I would be slightly worried about this fix puppeteer/puppeteer#7060 introducing some unwanted behavior around the topic of request interception.

@albertyw
Copy link
Contributor Author

I split the original changes in this PR into three PRs so they could be tested independently. There's #227 which is master with npm install which made some small refactoring changes in package-lock.json. #226 and #228 are rebased on top of #227 and they update watchify and pupeteer, respectively. All three PRs pass, including this one, which is a surprise to me since the last time I created the same PR, npm ci failed.

On the npm ci/npm install issues, my guess is that there was some node_modules or other cache that needed to be busted. npm ci may have failed because of If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock. (https://docs.npmjs.com/cli/v7/commands/npm-ci)

Feel free to hold on #228, given possible issues with puppeteer compatibitliy.

@m90
Copy link
Collaborator

m90 commented Jun 20, 2021

Thank you for untangling this and getting npm to behave.

I just had a another look at the changes this would introduce browserify/watchify@v3.11.1...v4.0.0 and it seems that watchify 4 also updates Browserify to v17, which currently does not behave nicely with some features in Mochify: #224

I don't think it is a problem right now (other than the duplicate install) but I would still like to double check what happens here:

var w = watchify(b);
and if this might somehow interfere with the v16 Mochify is using itself.


Strangely, looking at the code of watchify it seems, browserify is only depended upon for getting the same CLI interface here: https://github.com/browserify/watchify/blob/7b27a3c0d6bb796cef71faebfeb610ec62ad97b3/bin/args.js#L1 so I guess we should be fine here. I need to run now, but I hope we can get this merged soon. Thank you.

@m90
Copy link
Collaborator

m90 commented Jun 20, 2021

Ok, I double checked again and it seems watchify as a library never imports the browserify it requires so this will work just fine, with the only downside of installing Browserify twice when installing Mochify, which I guess is ok-ish.

Thank you!

@m90 m90 merged commit 255aaea into mantoni:master Jun 20, 2021
@m90
Copy link
Collaborator

m90 commented Jun 20, 2021

📦 Released this as 8.0.1 🚀

@albertyw albertyw deleted the npm-update branch June 22, 2021 05:07
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.

None yet

2 participants