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

fix(deps): update webpack to v5.94.0, terser to v5.33.0 #1213

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

theoludwig
Copy link
Contributor

@theoludwig theoludwig commented Sep 10, 2024

Copy link

socket-security bot commented Sep 10, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Looks like macos is failing to install dependencies.

We might need to update canvas as well.

@theoludwig
Copy link
Contributor Author

Looks like macos is failing to install dependencies.

We might need to update canvas as well.

Thank you for running CI, and helping for fixing this issue. 😄

I've updated canvas to https://github.com/Automattic/node-canvas/releases/tag/v3.0.0-rc2, hopefully it fixes issues with macOS, but for "Node 20 and ubuntu-latest", it seems like there is another issue.

@styfle
Copy link
Member

styfle commented Sep 11, 2024

Looks like updating canvas didn't help. Perhaps we need to install pangocairo maybe others:

Automattic/node-canvas#1050 (comment)

@styfle
Copy link
Member

styfle commented Sep 11, 2024

test/integration.test.js Outdated Show resolved Hide resolved
test/integration.test.js Outdated Show resolved Hide resolved
@theoludwig theoludwig requested a review from styfle September 14, 2024 13:30
@styfle styfle changed the title build(deps): update webpack to v5.94.0, terser to v5.32.0 fix(deps): update webpack to v5.94.0, terser to v5.32.0 Sep 16, 2024
@styfle
Copy link
Member

styfle commented Sep 16, 2024

Are you able to run coverage locally?

The failure makes it seem related to this PR.

  ● should generate correct output for browser-mainfield
    TypeError: Cannot read properties of undefined (reading 'then')
      at node_modules/webpack/lib/FileSystemInfo.js:1908:19
      at processQueue (node_modules/webpack/lib/util/processAsyncTree.js:56:4)

Perhaps you could start with a new PR that doesn't upgrade webpack and only gets CI passing?

@theoludwig
Copy link
Contributor Author

When I first started this PR, I was able to run yarn test locally without issues, then I focused on the CI problems, and haven't ran tests locally since.
I never tried yarn test-coverage locally, my bad, I should have ran it, it seems like it is also part of the main issue since the beginning.

Perhaps you could start with a new PR that doesn't upgrade webpack and only gets CI passing?

Yeah, that's a good idea, to start simple and "small" as possible. Priority is to first fix CI, then we'll see.
However, I tried rm -rf node_modules .cache tmp, git checkout main, yarn, yarn test, and the same issue seem to appear:

      at node_modules/webpack/lib/FileSystemInfo.js:1676:22
      at processQueue (node_modules/webpack/lib/util/processAsyncTree.js:50:4)
/home/theoludwig/Documents/open-source/ncc/node_modules/webpack/lib/FileSystemInfo.js:1677
							lexer.init.then(() => {
							           ^

TypeError: Cannot read properties of undefined (reading 'then')
    at /home/theoludwig/Documents/open-source/ncc/node_modules/webpack/lib/FileSystemInfo.js:1677:19
    at processQueue (/home/theoludwig/Documents/open-source/ncc/node_modules/webpack/lib/util/processAsyncTree.js:50:4)
    at processTicksAndRejections (node:internal/process/task_queues:77:11)

I will check later, when I have time, what could be the root cause of the issue.

@styfle
Copy link
Member

styfle commented Sep 17, 2024

Yes, ideally you could run yarn test-coverage locally and not have to wait on me to approve CI. That should help you iterate faster. Thanks for looking into it!

@theoludwig
Copy link
Contributor Author

theoludwig commented Sep 17, 2024

Yes, ideally you could run yarn test-coverage locally and not have to wait on me to approve CI. That should help you iterate faster. Thanks for looking into it!

Yep! (was just blindlessly thinking it was only related to CI config problems 😅)

After debugging for a while, I found that the error might be related to a race condition happening when process.exit(0); is called in coverage mode in integration.test.js but watcher.test.js tests are still running.

I first tried to run each test file separately, like: yarn test-coverage test/integration.test.js, yarn test-coverage test/watcher.test.js, etc. And everything was working fine locally.

So I tried with jest --runInBand CLI option, and now yarn test-coverage works locally. The leveldown.js test on macOS still need to be skipped, and OS dependencies still need to be installed.

Just to be 100% sure, that the CI problem is resolved, I enabled GitHub Actions on my fork, and ran it here: https://github.com/theoludwig/ncc/pull/1

And good news, it is working:
image

The PR is here: #1216
It's focusing only on fixing the CI and tests, when it will be merged, I will update this one, to update webpack and terser (most importantly, terser, if the webpack update doesn't work).

styfle pushed a commit that referenced this pull request Sep 20, 2024
@styfle
Copy link
Member

styfle commented Sep 20, 2024

@theoludwig Nice find! 🕵️

The other PR was merged so feel free to update this one, thanks! 🎉

@theoludwig theoludwig changed the title fix(deps): update webpack to v5.94.0, terser to v5.32.0 fix(deps): update webpack to v5.94.0, terser to v5.33.0 Sep 21, 2024
Copy link

Report too large to display inline

View full report↗︎

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Great work, thanks! 🎉

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.

Outdated Terser causes a syntax issue when minify-ing code
2 participants