deps: vendor semver

[flakey5]

Vendor semver internally. This pr originated from #44942 but kept open incase there were other decisions made that needed this vendored.

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[mcollina]

Is this exposing semver in any way?

[mcollina]

lgtm

[flakey5]

Is this exposing semver in any way?

No, figured just to have it internal for now and later on a conversation on exposing it could be held if anyone wanted it to be

[anonrig]

Maybe I'm missing something but I'm not in favor of adding a dependency without exposing or using it. There is an issue to which this pull request is linked, but there isn't any parent pull request which depends on/references the changes in the pull request.

[flakey5]

Maybe I'm missing something but I'm not in favor of adding a dependency without exposing or using it. There is an issue to which this pull request is linked, but there isn't any parent pull request which depends on/references the changes in the pull request.

This is 1 of 2 prs related to the issue linked above regarding the version checker. The second one would be the one actually using the module, which I planned on opening after this one landed.

[tniessen]

Wasn't there a version manager working group? (cc @ljharb)

I remember multiple proposals being shot down because some group was working on a cross-platform approach to version management, and this seems to fit into that direction.

[ljharb]

There still is one; version checking is indeed something I’d expect to fall under that group. Our next step remains the same because nobody’s done the work on it yet: to catalog and unify install paths. Version checking doesn’t conflict with that, but it’s a much larger design space given versions have semver, but there’s also release lines, and also nightlies and RCs, and also unofficial builds, and also Linux distro/user customized builds.

[GeoffreyBooth]

Forgive the newbie question, and this isn’t meant to hold up or block this PR, but is there any way to include dependencies like semver (and postject in that similar PR) only at build time, without needing to include them in the node repo? We could host their repos under github.com/nodejs, and the binaries would pull them in as part of the build script.

[jasnell]

To be clear, this PR is just about adding the version check notification discussed in #44942 ... It is not about adding a version manager.

@flakey5 ... As I mentioned to you, it would be good to add a basic unit test here that validates that the dep does in fact exist and work

[flakey5]

Forgive the newbie question, and this isn’t meant to hold up or block this PR, but is there any way to include dependencies like semver (and postject in that similar PR) only at build time, without needing to include them in the node repo? We could host their repos under github.com/nodejs, and the binaries would pull them in as part of the build script.

Maybe? But also imo that's a discussion out of the scope of this pr especially given the current precedent for dependencies afaik

[mscdex]

@GeoffreyBooth Having to fetch additional resources from the internet in order to compile a normal build of node just adds a point of failure, which is something we should try to avoid.

Additionally it can make things harder to reason about when debugging related issues because you'd need to look at an external repo and figure out the version that is/was used and ensure that the code wasn't changed, etc.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47487/

[jasnell]

We should not ship this without explicit sign off from https://github.com/orgs/nodejs/teams/npm

Why? I agree that we should not expose it to users without sign off from npm but this PR doesn't do that. It's bundled internally and not exposed to users.

[jasnell]

Adding don't land labels so, if it does land, it doesn't end up in a release until it's ready.

[RaisinTen]

I see no reason to block this PR. The discussion about whether to expose the API is an entirely separate conversation. This PR does not expose the API. It has no outward impact whatsoever. If we decide ultimately not to do the version check, it can easily be removed without any impact to anyone. If there are concerns about it ending up in a release until that decision is made, we can add don't land flags on the pr.

Exactly, my point is that we don't have consensus on any use case for this commit yet. This is currently adding a dependency for something we haven't fully decided on. So in case we finally decide on not pursuing either of the use cases, this commit would get reverted and this adds more commits for no real reason IMO.

[jasnell]

Exactly, my point is that we don't have consensus on any use case for this commit yet. This is currently adding a dependency for something we haven't fully decided on.

There is, btw, precedence for this. For instance, we currently have ngtcp2 and quictls (the quic enabled fork of openssl) included as a dependency in support of the still in development quic stuff. It was added separate in order to make it easier to develop the quic stuff in smaller chunks that would hopefully be easier to review, even tho there is currently still no consensus on whether quic support actually will ultimately land. Should we decide ultimately not to pursue quic, we can very easily just remove that dependency, impacting nothing.

... this adds more commits for no real reason IMO.

Commits are cheap. There is nothing that says we have to make decisions all at once.

[lukekarrys]

A question about the implementation here:

What's the reason for including the source and a single file that is generated with esbuild? If I'm reading it right, the generated file is what gets returned from internal/deps/semver/semver. But there shouldn't be any functional difference between the generated file and semver's index.js file.

[RaisinTen]

even tho there is currently still no consensus on whether quic support actually will ultimately land

I don't think anyone was opposed to having quic in Node.js, right? I thought that quic hasn't landed because the PR is still in a WIP state and it will eventually land when the work is completed and there are approvals but in this case there are folks objecting to the use cases.

[RaisinTen]

But there shouldn't be any functional difference between the generated file and semver's index.js file.

semver's index.js file has relative require calls but the require() used in the internal files of node doesn't support that. See nodejs/undici#1183 (comment).

[RaisinTen]

Why does this require semver to be downloaded again? Why can't we run the esbuild step on deps/npm/node_modules/semver/index.js?

[flakey5]

Semver is only downloaded once, the lines you highlighted just cd into the cloned directory and run esbuild on it

[RaisinTen]

I mean why do we need to download the source of semver when it already exists inside deps/npm/node_modules/semver. Why can't we reuse that?

[flakey5]

I mean why do we need to download the source of semver when it already exists inside deps/npm/node_modules/semver. Why can't we reuse that?

Oh yeah, I didn't even think of that. Should probably put the updating semver script in the npm update script as well then

[ljharb]

It makes sense to me to use only the version of semver that npm brings in

[jasnell]

Should probably put the updating semver script in the npm update script

Do you mean the part of the script that generates the single file? +1 to that.

One thing to keep in mind, however, is that -- assuming this lands and assuming we make the decision to expose the/a semver api to users in the future -- there might come a time when npm could stop bundling npm and expect to get it just from node.js, at which point we'd need to bring this full script back.

[lukekarrys]

Should probably put the updating semver script in the npm update script as well then

Most npm PRs are automated and created with tooling from the npm/cli repo. Is there a step that could run after one of those is merged, that would look for semver and then bundle its source?

Also there is a chance that semver gets deduped in the npm tree in the future. Ideally the tool could use something like npm query to get only the version of semver that npm uses.

When run inside the npm/cli repo:

❯ npm query ':root > #semver' | jq '.[0].location'
"node_modules/semver"

[flakey5]

Do you mean the part of the script that generates the single file?

Yeah

[flakey5]

I assume semver doesn't need to be added to dep_checker anymore now that it'll be vendored through whatever version npm is using? Also, given that it wasn't already in the license file before, does it still need to be since it's being vendored? I assume yes but asking to make sure

Is there a step that could run after one of those is merged, that would look for semver and then bundle its source?

Are you referring to an action that would fire when a pr is merged with npm cli or something different?

❯ npm query ':root > #semver' | jq '.[0].location'

Currently not able to get this working for whatever reason but that would indeed be a good idea as well

[Trott]

@jasnell We weren't sure what to talk about with this at the meeting today. We think it can be removed from the TSC agenda, but if I'm wrong, please add the label back.

[jasnell]

Yes it can be removed from the agenda.