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

feat: allow to set package-name and version #104

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Sep 13, 2021

continue #102

name and version can be missing in package-lock.json, but are required for stdenv.mkDerivation

if version is missing, use the unknown version 0.0.0

sample use:

npmlock2nix.build rec {
  src = "...";
  node_modules_attrs = {
    pname = "some-package";
    version = "1.2.3";
  };
}

internal.nix Show resolved Hide resolved
internal.nix Show resolved Hide resolved
internal.nix Outdated Show resolved Hide resolved
@milahu
Copy link
Contributor Author

milahu commented Sep 14, 2021

pname and version should be inherited from build

npmlock2nix.build rec {
  src = "...";
  pname = "some-package";
  version = "1.2.3";
  node_modules_attrs = {
    inherit pname version; # avoid this
  };
}

maybe the node_modules derivation name should be some-package-1.2.3-node_modules

internal.nix Outdated
pname =
if (args ? pname && args.pname != "") then makeValidDrvName args.pname
else if (lockfile ? name && lockfile.name != "") then lockfile.name
else ""; # pname is required, throw later
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use null here instead? It doesn't make much of a difference but I kinda like defaulting to a specific value for this that isn't a string (and thus is harder to accidentially use wrong).

Copy link
Collaborator

@andir andir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but we should make sure that we add a test case for this.

Since most of this is about error handling I could understand being a bit sloppy around those cases.

We should test that

a) a name an version from a package-lock.json just works (as before)
b) providing an alternative name works (and retains the version)
c) providing an alternative version worsk (and retains the pname)
d) The absence of both pname and version leads to the expected outcome (for version the default string and for pname a throw?)

@andir
Copy link
Collaborator

andir commented Sep 14, 2021

pname and version should be inherited from build

npmlock2nix.build rec {
  src = "...";
  pname = "some-package";
  version = "1.2.3";
  node_modules_attrs = {
    inherit pname version; # avoid this
  };
}

maybe the node_modules derivation name should be some-package-1.2.3-node_modules

I agree on the node_modules derivations name. Feel free to add a commit to this PR that adjusts that.

I also agree on the inheriting part. I think we already do that with a couple of attributes and it would make sense here. If we do that we also have to add a test case that ensures that it works as expected. Even if it is correct now I'd like to make it as risk-free as possible to refactor the implementation while keeping the external interface.

internal.nix Show resolved Hide resolved
Copy link
Collaborator

@andir andir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need a few test cases for this. Could you add them?

Perhaps this could be a starting point?

diff --git a/tests/node-modules.nix b/tests/node-modules.nix
index 1d65e1b..a6c8bb6 100644
--- a/tests/node-modules.nix
+++ b/tests/node-modules.nix
@@ -1,4 +1,4 @@
-{ npmlock2nix, testLib, runCommand, nodejs, python3 }:
+{ npmlock2nix, testLib, runCommand, nodejs, python3, writeText }:
 testLib.runTests {
   testNodeModulesForEmptyDependencies = {
     expr =
@@ -137,4 +137,54 @@ testLib.runTests {
       expr = builtins.pathExists drv.outPath;
       expected = true;
     };
+
+  testPnameOverride =
+    let
+      drv = npmlock2nix.node_modules {
+        pname = "override-pname";
+        src = ./examples-projects/no-dependencies;
+      };
+    in
+    {
+      expr = { inherit (drv) name pname; };
+      expected = {
+        pname = "override-pname";
+        name = "override-pname-1.0.0";
+      };
+    };
+
+  testVersionOverride =
+    let
+      drv = npmlock2nix.node_modules {
+        version = "23";
+        src = ./examples-projects/no-dependencies;
+      };
+    in
+    {
+      expr = { inherit (drv) name pname version; };
+      expected = {
+        pname = "no-dependencies";
+        version = "23";
+        name = "no-dependencies-23";
+      };
+    };
+
+  testNoVersion =
+    let
+      drv = npmlock2nix.node_modules {
+        src = ./examples-projects/no-dependencies;
+        packageLockJson = writeText "package-lock.json" (builtins.toJSON (
+          builtins.removeAttrs (builtins.fromJSON (builtins.readFile ./examples-projects/no-dependencies/package-lock.json)) ["version"]
+        ));
+      };
+    in
+    {
+      expr = { inherit (drv) name pname version; };
+      expected = {
+        pname = "no-dependencies";
+        version = "0.0.0";
+        name = "no-dependencies-0.0.0";
+      };
+    };
+
 }

@milahu
Copy link
Contributor Author

milahu commented Sep 17, 2021

Perhaps this could be a starting point?

thanks. i just started test.sh and it takes foreeeeeeeeever
seems to download a million packages ...

edit: just 742 paths

these paths will be fetched (989.65 MiB download, 4236.21 MiB unpacked):

crazy!

i hope there is a lighter test method, like snapshot testing

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.

3 participants