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

[BRC-85] Upgrade to latest LTS node 22 #257

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

kaareal
Copy link
Collaborator

@kaareal kaareal commented Jan 18, 2025

Worth mentioning:

I have added suppression for the "punycode module is deprecated", thats the --disable-warning=DEP0040
In web I have remove the circular-dependency-plugin due The util._extend API is deprecated. I think we can remove this safety net.

@kaareal kaareal requested a review from andrewplummer January 18, 2025 13:18
@andrewplummer
Copy link
Collaborator

This is fine for now. I feel like I'm seeing those warnings everywhere now.
Added an issue for that plugin as it's saved me more than once. If they fix it can we bring it back?
aackerman/circular-dependency-plugin#101

@andrewplummer
Copy link
Collaborator

I just bumped a package to node 22 actually and I just ran through all my scripts and added:

#!/usr/bin/env node --no-warnings

very annoying but it is what it is... we should do this for core as well

@andrewplummer
Copy link
Collaborator

If this continues to be a problem then maybe the shebang approach is how all of our scripts should work. More boilerplate but it can be copy/pasted. There doesn't seem to be a way to set NODE_OPTIONS globally and I don't want to use shell stuff to mask the issue as I want node configured exactly like it will be in the container. I still don't understand where this punycode crap comes from is there an issue you can link to so we can follow it?

@kaareal kaareal closed this Jan 18, 2025
@kaareal kaareal reopened this Jan 18, 2025
@kaareal
Copy link
Collaborator Author

kaareal commented Jan 20, 2025

Re Added an issue for that plugin as it's saved me more than once, yep lets just add it back and live with the warning. I will update the PR.

Re punycode
The issue is kind of simple, node.js used to offer a punycode library functions, but it was deprecated and split into a punycode npm modules. but there is still libraries using node js version. https://nodejs.org/api/punycode.html#punycode_punycode

doing npm ls punycode is not gonna help. Instead use the --trace-deprecation flag to find the modules that caused the warning and then do npm ls <module> to find modules that would should be fixed.

I think there 3 modules we have bedrock core api that is causing a problem, if recall correctly its related to google cloud storage and something else.

@andrewplummer
Copy link
Collaborator

I think we should be able to do: NODE_OPTIONS="--no-warnings" webpack?

@andrewplummer
Copy link
Collaborator

andrewplummer commented Jan 20, 2025

Ok this just hit me in another package and I went deep. It's just so, so stupid.

I had an even bigger problem which is that even if --no-warnings works it can't be used in an /env shebang inside a docker container, ie this hard fails in a container:

#!/usr/bin/env node --no-warnings

What makes it worse is I was relying on binaries which need to be executables so the consumer MUST include that flag (and even know how what's going on).

Looking at the deps there's a million things relying on punycode and others. I thought about opening issues with them but there's 4 places and they're deep in the chain. No one is going to fix this until node actually breaks it and meantime I don't want to look at it so I made this:

https://github.com/bedrockio/node-warnings

just require @bedrockio/node-warnings any script entrypoint and the warnings go away.
I know, it's stupid. But I don't want to think about dependencies one second longer for this bs

@kaareal
Copy link
Collaborator Author

kaareal commented Feb 1, 2025

@andrewplummer i have added back the circular-dependency-plugin + remove the disabling of warnings, after merge lets add in the https://github.com/bedrockio/node-warnings.

@kaareal kaareal merged commit 20badc5 into master Feb 3, 2025
2 checks passed
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.

2 participants