Support for node >15 and npm>7 attempt #3

[picnoir]

This PR is built on top of https://github.com/nix-community/npmlock2nix/pull/165. Massive kudos to @tlxzilia for the head start.

Originally, I planned to:

1. Separate the project in two codebases: a v1 codebase extracted from the current master, a v2 codebase extracted from #165. This approach allow us to keep backward compatibility with the v1 lockfiles without having to bloat the v2. It'll also greatly simplify the v1 lockfiles support rollout: we'll just have to remove the internal-v1.nix codebase and its tests. Credit where it's due, this was not my idea: this approach has been suggested by @andir <3
2. Port the test suite to the v2 codebase by generating v2 lockfiles for the example-projects.

Things diverged from this plan along the way:

1. Some unit/integration tests and real-world tests revealed some bugs in the existing v2 codebase. https://github.com/nix-community/npmlock2nix/commit/74ca354196f1dfd4f023f11c16d5f508df965bc1, https://github.com/nix-community/npmlock2nix/commit/d41f9b0915e2e1e7d3ca78b47d0f840fb793b21e and https://github.com/nix-community/npmlock2nix/commit/09d8bd5780be76593053043a96bfd4b6c877bdc9 are fixing these bugs, adding automated tests when it's needed.
2. At some point, I realized npm won't complain if we provide a "pure v2" (aka. v3) lockfile. IE. a lockfile where we omit completely the v1-specific dependencies top-level attr. This simplifies a great deal the v2 codebase, since we can drop all the v1-specific machinery from it. This has been implemented in https://github.com/nix-community/npmlock2nix/commit/68196dd91c42f7bd773572a67333051477b05094
3. After using this new v2 codebase in a few real-world project, I found two more bugs: https://github.com/nix-community/npmlock2nix/commit/24b96194900de7f5c89d90a3c68bb1ae875e1aa3 and https://github.com/nix-community/npmlock2nix/commit/2917d72647afceb181ea6d2bc653ecdcc0c0bad3.

You can find more details about what I've done in the commit messages.

There's two more non-critical points to address, but at this point, the PR is already pretty massive, I'm already afraid it could be too big to be reviewed/merged. I'd like to work on those more incrementally once this gets merged for those. Namely:

1. The source-patched npm dependencies are getting tgz-ed before being added to the store. That might be problematic gc-wise: Nix won't be able to detect the store paths stored in these tarballs and create the appropriate GC-root dependencies. We probably should add these store references paths in a text file stored in $out/build-support.
2. I used a trick to simplify the computation of the Integrity field of the patched npm dependencies: I set those to null. If it is set to null, it seems like npm won't try to compute the dependency integrity. It makes things simpler for us: we don't need to perform an IFD to compute the hashes anymore. That being said, it could backfire at any point if npm decides at some point to change its mind about this undocumented behavior.

[tlxzilia]

My direction with #165 was to support building a project with lockfile v1 and compare results for nodejs 14, 16 and 18. I have no control over the package lock definition. Do you suggest that I make a preprocessor to convert v1 to v2 and then use npmlock2nix, I'm not sure which direction to go.

I do use a linked .npmrc to customize the build, note the legacy-peer-deps = true: npmrc = writeText "npmrc" '' loglevel = info offline = true engine-strict = false unsafe-perm = true cache = /tmp/npm-cache legacy-peer-deps = true '';

If it helps I could wrap up a test using one of our project package-lock.json.

Also, If it's a matter of still supporting v1 API, could we not just reintroduce the the node_modules/.hooks script and preInstallLinks? Then ignore them on nodejs >15.

The source-patched npm dependencies are getting tgz-ed before being added to the store. That might be problematic gc-wise: Nix won't be able to detect the store paths stored in these tarballs and create the appropriate GC-root dependencies. We probably should add these store references paths in a text file stored in $out/build-support.

I also wondered about nix garbage collection with packaged tar gz. Thanks for pointing out the $out/build-support mechanic.

[picnoir]

My direction with https://github.com/nix-community/npmlock2nix/pull/165 was to support building a project with lockfile v1 and compare results for nodejs 14, 16 and 18.

Ah, okay, I was confused about that part. It makes sense now :)

I have no control over the package lock definition. Do you suggest that I make a preprocessor to convert v1 to v2 and then use npmlock2nix, I'm not sure which direction to go.

Are you automating the generation of the npmlock2nix nix code or are you writing that manually?

If you write it manually, I think the best approach would be to use the npmlock2nix.v1 codebase for the v1 lockfiles, the npmlock2nix.v2 codebase for the v2/v3 lockfiles.

If you're generating that code, I guess we could either go the pre-processor[^1] route to determine what is kind of lockfile you're looking at.

Alternatively, we also could perform this pre-processor phase at the top-level of npmlock2nix. We can dynamically detect which codebase to use by parsing the lockfile and looking at the lockfileVersion field.

If it helps I could wrap up a test using one of our project package-lock.json.

If this project breaks the current codebase, it'd be great if we could to add it to the test!

Also, If it's a matter of still supporting v1 API, could we not just reintroduce the the node_modules/.hooks script and preInstallLinks? Then ignore them on nodejs >15.

The preInstallLinks are still supported in the v1 codebase. They are also ignored in the v2 codebase (ie. nodejs >15).

The sourceOverrides mechanism is supported for both codebases. I first thought about removing it for the v1 codebase, but I later realized they are already part of the master branch. Removing them might break some out of tree code :(

[^1]: A glorified name for a jq pass here :)

[tlxzilia]

my npmlock2nix code is not autogenerated. By preprocessor I meant doing a npm install outside nix to get a lockfile v2. to use npmlock2nix.v2.

Because with current v1 (master) I cannot build a v1 lockfile with npm 7 or nodejs >15.

The preInstallLinks are still supported in the v1 codebase. They are also ignored in the v2 codebase (ie. nodejs >15).

Sorry I said v1 when I was referencing #165. That's the only thing I removed, since it was supported by the API update I made.

[picnoir]

my npmlock2nix code is not autogenerated. By preprocessor I meant doing a npm install outside nix to get a lockfile v2. to use npmlock2nix.v2.

Because with current v1 (master) I cannot build a v1 lockfile with npm 7 or nodejs >15.

Right! I get it now. I did not think about that case!

I assume this particular case will only happen during this v1 -> v2 transition phase we're currently in. As far as I understand, npm coming from nodejs > 15 will override the v1 lockfile with a v2 as soon as you'll run npm install. Meaning, I assume most upstream will end up migrating to a v2 lockfile as soon as they use a nodejs > 15.

Provided nodejs 14 EOL is planned on the 2023/04/30, I think we're unlikely to see much lockfiles v1 for the still maintained projects past that date.

Dropping the v1 support for the v2 lockfiles simplifies the codebase quite a lot, reducing the potential bugs surface and easing the maintenance on the long run. I think the preprocessor approach would be preferable here.

That being said, you might not be the only one falling into that use case. It'd be probably a good idea to create a /contrib folder at the project root and add your preprocessing script in it. It could probably help somebody else.

Sorry I said v1 when I was referencing https://github.com/nix-community/npmlock2nix/pull/165. That's the only thing I removed, since it was supported by the API update I made.

Aha! Indeed, good point, I've been overlooking that. There's no reason for only supporting the sourceOverrides. I'll fix that shortly.

[flokli]

I assume this particular case will only happen during this v1 -> v2 transition phase we're currently in. As far as I understand, npm coming from nodejs > 15 will override the v1 lockfile with a v2 as soon as you'll run npm install. Meaning, I assume most upstream will end up migrating to a v2 lockfile as soon as they use a nodejs > 15.

Yes, I saw exactly that when running npm install with a more recent version of nodejs in $PATH.

Thanks to both of you. This is a massive improvement, and I'm looking forward to seeing this merged :-)

[gilligan]

@NinjaTrappeur sorry, this is way too much. I don't have the time to go through all of this - and the v1 and v2 thing is pretty horrifying ;/ i guess this is where the project reaches the point that every single something2Nix project reaches sooner or later where it gets "ugly" / complicated..

PS: kudos for the very clean code, good documentation and tests

[flokli]

I think part of this is due to the current duplication, as the code for v1 and v2 is kept separately (as per @andir 's suggestion).

Some of the diff noise might go away by first diffing the old code against the v1 parts in here, and then diffing the v1 implementation with the v2 one.

Another alternative could be to drop v1 support immediately, and ask users to use an old version of npmlock2nix if they still want to package stuff where they can't create a v2 lockfile. WDYT?

[gilligan]

@flokli to be clear, the v1/v2 separation isn’t necessarily a bad idea.. the complexity is what is horrifying to me. And that of course is nobody’s fault. It’s the result of having to deal with something that isn’t meant as a public interface 😅

i think @andir ’s reasoning is sound when he wants to separate things to avoid too much separate case handling and what not.

@flokli as for your suggestions, hm.. i don’t know from the top of my head what the npm release schedule is. Maybe there should be a clear goal/statement on what npmlock2nix strives to support and we should base that to consider what options we have. What do you think @andir ?

[gilligan]

By the way, i certainly won’t be stopping progress on this just because i can’t review a big PR. The code per se looks clean and we have good documentation and tests as per usual in this project.

I assume @andir and others are more likely to be digging through details. I’d just involve myself in talking about broader stuff like what we want to support when ;)

[andir]

I'll to review this tomorrow.

[andir]

@flokli as for your suggestions, hm.. i don’t know from the top of my head what the npm release schedule is. Maybe there should be a clear goal/statement on what npmlock2nix strives to support and we should base that to consider what options we have. What do you think @andir ?

I'm trying to be realistic here: Neither do I have the energy available to follow NPMs or NodeJS' release cadence nor do I think that at some point we will get rid of old Node projects that have old versions of lockfiles with potentially issues that (can be fixed) that we haven't encountered yet.

My vision right now is that the v1 implementation will effectively become read-only over the long time. As long as there are no further issues found with it, I'd just not touch it and keep it in here. It might come in handy if you ever have to package an older version of a package. I don't believe in pointing users to an older checkout or release version to get feature support. These are usually not fun to use.

[gilligan]

I had a look at this. One thing about the v1/v2 splitting is that there is a lot of common code which I think can and should be shared.

https://github.com/nix-community/npmlock2nix/commit/1a35ac2479ac3229dbdd0e335752e8f4fe7e7e0b

This was just a quick attempt. I think the functions around GitRev/url/ parsing and validation also seemed similar* and could perhaps be made reusable? Not entirely sure.

There is some hash function that could also be moved but i ran out of time and would have had to adjust the tests as well.

TL;DR i would like to advocate for extracting reusable code as seen in my attempt. @andir reasonable to you? I know you wanted the v1/v2 split.

[andir]

Yeah, I want the code to be refactored by I'd not do it as part of this PR. Lets get this working with proper tests first and then refactor the duplicate code.

[flokli]

The problem here seems to be node_modules/@rjsf/bootstrap-5 inside zigbee2mqtt's package-lock.json.

https://github.com/Koenkk/zigbee2mqtt/blob/03ba647dc6b5f299f8f3ab441712999fcb3a253e/package-lock.json#L2779-L2795

package-lock.json refers to a tarball uploaded into a github repository by the ?raw=true download URL: https://github.com/nurikk/fileshare/blob/main/rjsf-bootstrap-5-4.2.0.tgz?raw=true.

I assume our other github logic detection gets confused about this, and we match github URLs too eagerly in isgithubRef.

[andir]

@NinjaTrappeur @zimbatm @flokli how shall we move forward with this? Does any of you actually have time to work on those few remaining issues so we can close this chapter (for now)?

[picnoir]

Hey Andi,

It's definitely on my todolist. I have been off for better part of last month, I just came back last week and had to deal with a month-worth of professional "urgent" matter.

The todo list is unpiling slowly, pushing through this PR is the thing coming after what I'm busy-with at the moment.

You can expect some movement during this week.

[picnoir]

The issue you faced with zigbee2mqtt turned out being a consequence of Npm trying to interpret query strings for file://-based schemes…

Fixed in 673e11a (#166).

I tried building the zigbee2mqtt derivation you posted above, it now works as expected.

[picnoir]

Removed a useless file as pointed out in https://github.com/nix-community/npmlock2nix/commit/68196dd91c42f7bd773572a67333051477b05094#r87717991

[andir]

I'll merge this in as is now. CI is passing. The documentation is a bit of a mess. I've tried do bring back the v1 documentation where still relevant. We will have to do a few more passes through the docs and bug fixing before the v2 config will be the default.