build(deps): bump node to v20

[yeikel]

Release notes :

https://nodejs.org/en/blog/announcements/v19-release-announce
https://nodejs.org/en/blog/announcements/v20-release-announce

[yeikel]

@deivid-rodriguez What do you think about this change?

We're still pinning the npm and yarn versions so it should be fine?

[deivid-rodriguez]

Hei @yeikel. I'm not sure, to be honest!

Currently we run a fixed version of nodejs, so that means we are not able to support applications with certain nodejs constraints. For example, if someone is specifying "node": "^18.12.0" in their engines package.json section, my understanding is that this PR will break that. On the other hand, if someone is specifying node 20 in there, then this PR will fix that.

I plan to do some work this week to properly surface "engine unsupported" errors to users, and that should give us better numbers of where the community is at, and the impact of this kind of PR.

Of course, ideally we would be able to use the most appropriate nodejs version for each repo, or even better, package manager would provide a way to ask "please resolve for nodejs version X, regardless of the running node". But this is a different story.

[yeikel]

Currently we run a fixed version on nodejs, so that means we are not able to support applications with certain nodejs constraints. For example, if someone specifying "node": "^18.12.0" in their engines package.json section, my understanding is that this PR will break that

As far as I was able to test, it does not seem that NPM enforces this by default

Dependencies can also define what engines they support that can lead to the following warning at installation time :

EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '^12.13.0 || ^14.15.0 || ^16.10.0 || >=17.0.0' },
npm WARN EBADENGINE   current: { node: 'v16.1.0', npm: '8.19.2' }
npm WARN EBADENGINE }

By default this is not enabled but it can be can be enforced via .npmrc setting engine-strict=true and in that case it lead to the following failure at installation time :

npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: @jest/[email protected]
npm ERR! notsup Not compatible with your version of node/npm: @jest/[email protected]
npm ERR! notsup Required: {"node":"^12.13.0 || ^14.15.0 || ^16.10.0 || >=17.0.0"}
npm ERR! notsup Actual:   {"npm":"8.19.2","node":"v16.1.0"}

As far as I can tell, we are catching that already when the user enforces it :

dependabot-core/npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb

Lines 422 to 426 in db9cb4b

	if error_message.include?("EBADENGINE")
	msg = "Dependabot uses Node.js #{`node --version`} and NPM #{`npm --version`}. " \
	"Due to the engine-strict setting, the update will not succeed."
	raise Dependabot::DependencyFileNotResolvable, msg
	end

ideally we would be able to use the most appropriate nodejs version for each repo, or even better, package manager would provide a way to ask "please resolve for nodejs version X, regardless of the running node"

I am not sure what you're thinking here, but perhaps we could include more than one version in the image with nvm and switch accordingly?

[deivid-rodriguez]

Yes, @yeikel, what you say is correct, and I did not know we were already handling this! I do know we have some cases were this kind of error ends up being raised as an unknown error. Maybe the error changes across different package managers / or different versions of the same package manager, and we're not properly capturing all error messages.

Also we should probably raise this as ToolVersionNotSupported. I don't think DependencyFileNotResolvable is a good fit. I mean, yes, Dependabot cannot resolve that dependency file, but that doesn't mean it's not resolvable, but just that Dependabot is not able to do it. I think ToolVersionNotSupported captures that better.

Regarding switching nodejs versions, yes, nvm would be a way. We've been hesitant to do this because of added complexity, bigger build time, image size, etc, but for major NodeJS versions I think it would probablt make sense. We already do it for Python and it works reasonably fine.

I'll look into our Node 18 numbers and how common this error is and decide if it's the right time for this upgrade 👍. It should probably be ok to upgrade because of what you explained (it's only a warning unless a setting is configured).

[yeikel]

@deivid-rodriguez What changed that you decided to approve it? I am just curious as your points were valid and interesting

Thank you!

[deivid-rodriguez]

Going a bit back and forth with this, but the intention is to deploy this soon-ish, because the potential issues seem quite minor, and not deploying this implies having similar issues for users that require the latest NodeJS version.

I'm still working on going through some common related errors unknown errors and raising them as informative user errors, and I'll probably deploy this once that's done.

[yeikel]

Hei!

I run out of time for working on better error handling for NPM and had to switch to other tasks. So my idea was to ship this as is.

Given your explanation in #8275 (comment) that this is by default a warning and not an error, I think it's fine to move forward with the upgrade. We should not be penalizing users of modern nodejs versions in favor of users of old nodejs versions.

Awesome, thank you for taking the time to explain.

Do you recall what was missing in terms of error handling? If you have time at some point in the near future, I'd be nice to log them as tech deb issues 🙏.

In any case, as you pointed out, this change should be ready to be merged as is. Thank you for taking the time to reply so quickly! 🙇‍♂️

@abdulapopoola Any chance you can help move this one forward?

[deivid-rodriguez]

I didn't have time to materialize it into a proper issue, but I suspected from analyzing error messages logged in our internal tracker that sometimes we were not properly handling EBADENGINE errors. Because otherwise they should be showing up as user errors and not as unknown errors in our internal tracker.

I think it's fine to merge this and wait until someone reports any issues.

[yeikel]

@jurre This one is ready as well

[basil]

Possibly related to #8896?

[xu3u4]

Any plan to merge this PR? 🙏

[yeikel]

@jurre Can you please review/help to move this one forward?

[smoookeeey]

@jurre Can you please review/help to move this one forward?

Approved

[jurre]

Thanks @yeikel! I've let this sit in prod for an hour and not observing any issues so far but we'll keep an eye out

[yeikel]

Thanks @yeikel! I've let this sit in prod for an hour and not observing any issues so far but we'll keep an eye out

Thank you! I think that the issues should be minimal because we're unfortunately still pinning to NPM 9.x

[xu3u4]

Thank you all 🙇‍♀️🙇‍♀️🙇‍♀️