Closed mdarocha closed 11 months ago
Hi @jtojnar,
This seems like an important update. I've already encountered this problem when I was developing my npm packages as they were using v3 format. I had to do $ npm install --lockfile-version 2 --package-lock-only
, which is not ideal and I'm sure I'm not the only person who is facing this issue as evidenced by this PR.
Can we merge this PR quickly to napalm? This feature could help with the adoption and I'd much rather use napalm than what author suggests here manually: https://www.nmattia.com/posts/2022-12-18-lockfile-trick-package-npm-project-with-nix/
@mdarocha seems to have created good changes that are backwards compatible. Let me know if I can help facilitate merging this any way. Thanks!
I'd like to suggest a small addition to handle a case that currently causes a 500 response from the napalm registry if certain conditions are met by the locked dependencies.
These conditions are for example met by projects that specify isaacs/cliui@8.0.1
as a dependency, which causes the following combination of transitive dependencies to be resolved: https://github.com/npm/cli/blob/latest/package-lock.json#L15575-L15609
The important bit here is that the first resolved dependency is wrap-ansi@8.1.0
and the second is wrap-ansi-cjs@7.0.0
but the latter is aliased to wrap-ansi@7.0.0
and requested as such from the registry by npm.
Using the current state of this PR, napalm parses the package-lock.json without respecting the specified alias (in this case "name": "wrap-ansi"
). Instead the dependency's key in napalm's snapshot file is wrap-ansi-cjs
with version 7.0.0. This results in a 500 response (to me it's unclear why not 404) when npm requests wrap-ansi@7.0.0
from the napalm registry.
The following patch fixes this and makes a specified name
attribute take precedence over the dependency object's name. Feel free to add this as is, or a modified version, to your PR :)
diff --git a/default.nix b/default.nix
index fa420be..7ddac09 100644
--- a/default.nix
+++ b/default.nix
@@ -141,12 +141,18 @@ let
version = ifNotNull version fallbackPackageVersion;
} // obj;
- parsePackageNameVersion = name': version': let
+ parsePackageNameVersion = name': originalObj: let
+ version' = originalObj.version;
# Version can be a pointer like “npm:vue-loader@15.10.0”.
# In that case we need to replace the name and version with the target one.
isPointer = lib.hasPrefix "npm:" version';
fragments = lib.splitString "@" (lib.removePrefix "npm:" version');
- name = if isPointer then builtins.concatStringsSep "@" (lib.init fragments) else name';
+ name = if builtins.hasAttr "name" originalObj
+ then originalObj.name
+ else
+ if isPointer
+ then builtins.concatStringsSep "@" (lib.init fragments)
+ else name';
version = if isPointer then lib.last fragments else version';
in
{ inherit name version; };
@@ -159,7 +165,7 @@ let
originalName:
originalObj:
let
- inherit (parsePackageNameVersion originalName originalObj.version) name version;
+ inherit (parsePackageNameVersion originalName originalObj) name version;
obj = originalObj // {
inherit name version;
};
@@ -192,7 +198,7 @@ let
# filter out the top-level package, which has an empty name
(lib.filterAttrs (name: _: name != ""))
(lib.mapAttrsToList (originalName: originalObj: let
- inherit (parsePackageNameVersion (pathToName originalName) originalObj.version) name version;
+ inherit (parsePackageNameVersion (pathToName originalName) originalObj) name version;
obj = originalObj // {
inherit name version;
};
Here is a small reproducer that adds cliui as a dependency to the test added in this PR (including the fix above):
Sorry, I can no longer find time for napalm so I am stepping down as maintainer.
Sorry, I can no longer find time for napalm so I am stepping down as maintainer.
Well, if you're looking for replacements, I could take the mantle. I wouldn't probably introduce many new changes and mostly focus on bug fixing and keeping up with npm compatibility.
I invited @WilliButz to be able to merge in this repository to have someone again that can merge in this project. I can add @mdarocha certainly as well in the future. It's just at the moment I don't know this user well, so I would like to either have someone vouch for them or I see some more activity (currently the contribution graph is private, so I can not really judge based on that).
@Mic92 right, I updated my profile, you can check it now
@WilliButz I incorporated your changes. Thanks!
Fixes https://github.com/nix-community/napalm/issues/55
Info on lockfile: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#lockfileversion.
Lockfile v3 is the same as v2, but contains only the
packages
field, notdependencies
. This required a new path when parsingpackages-lock.json
. This new logic can also be extended to run on v2 lockfiles, but I decided to limit it to v3 to not accidentally break somebody's project.