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

Node: transitive dependency bin taking priority over direct dependency #221

Closed
tinybeachthor opened this issue Aug 1, 2022 · 10 comments · Fixed by #227
Closed

Node: transitive dependency bin taking priority over direct dependency #221

tinybeachthor opened this issue Aug 1, 2022 · 10 comments · Fixed by #227
Labels
bug Something isn't working

Comments

@tinybeachthor
Copy link
Contributor

tinybeachthor commented Aug 1, 2022

Intro

I have a direct binary dev dependency svgo@2 in package.json.
Another direct dependency depends on an earlier version svgo@1.
Dream2Nix resolves the linked binary in $out/node_modules/.bin to the earlier version - the transitive dependency - instead of the direct dependency.

Details

While packaging Castopod with dream2nix:

package.json:

"devDependencies": {
    "svgo": "^2.2.2",
    "cssnano": "^4.1.10"
},

With cssnano -> svgo@1.

@tinybeachthor tinybeachthor changed the title Node: transient dependency bin taking priority over direct dependency Node: transitive dependency bin taking priority over direct dependency Aug 1, 2022
@tinybeachthor
Copy link
Contributor Author

Seems like the issue is in src/subsystems/nodejs/builders/granular/install-deps.py

@wmertens
Copy link
Contributor

wmertens commented Aug 8, 2022

I think this is fixed in #195 - can you give that a try? (nix flake lock --override-input dream2nix github:wmertens/dream2nix/devShells I think)

Transitive dependencies shouldn't be part of bin/ IMHO.

@tinybeachthor
Copy link
Contributor Author

tinybeachthor commented Aug 9, 2022

@wmertens yes, that seems to make it better, but not quite fix it

{
  inputs.dream2nix.url = "github:wmertens/dream2nix/devShells";
  outputs = inp: let
    pkgs = inp.dream2nix.inputs.nixpkgs.legacyPackages.x86_64-linux;
    package_json = builtins.toFile "package.json" ''
      {
        "name": "anything",
        "devDependencies": {
          "svgo": "^2.2.2",
          "cssnano": "^4.1.10"
        }
      }
    '';
  in
    inp.dream2nix.lib.makeFlakeOutputs {
      systemsFromFile = ./nix_systems;
      config.projectRoot = ./.;
      source = pkgs.runCommand "src" {} ''
        mkdir $out
        cp ${package_json} $out/package.json
      '';
    };
}

From #195, nix develop gets me the following:

> ls -la node_modules/.bin
total 4
dr-xr-xr-x 1 root root  8 Dec 31  1969 .
dr-xr-xr-x 1 root root 38 Dec 31  1969 ..
lrwxrwxrwx 2 root root 85 Dec 31  1969 svgo -> /nix/store/fh4rpdx5q8myx9m996ardj5yx9bwxvfz-svgo-2.8.0/lib/node_modules/svgo/bin/svgo

From non-nix npm install:

> ls -la node_modules/.bin
total 32
drwxr-xr-x 1 martin users  138 Aug  9 11:52 .
drwxr-xr-x 1 martin users 4618 Aug  9 11:52 ..
lrwxrwxrwx 1 martin users   22 Aug  9 11:52 browserslist -> ../browserslist/cli.js
lrwxrwxrwx 1 martin users   32 Aug  9 11:52 browserslist-lint -> ../update-browserslist-db/cli.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:52 cssesc -> ../cssesc/bin/cssesc
lrwxrwxrwx 1 martin users   25 Aug  9 11:52 esparse -> ../esprima/bin/esparse.js
lrwxrwxrwx 1 martin users   28 Aug  9 11:52 esvalidate -> ../esprima/bin/esvalidate.js
lrwxrwxrwx 1 martin users   25 Aug  9 11:52 js-yaml -> ../js-yaml/bin/js-yaml.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:52 mkdirp -> ../mkdirp/bin/cmd.js
lrwxrwxrwx 1 martin users   16 Aug  9 11:52 svgo -> ../svgo/bin/svgo

From #227:
inputs.dream2nix.url = "github:tinybeachthor/dream2nix/fix-nodejs-link-direct-bins";

> ls -la node_modules/.bin
total 32
drwxr-xr-x 1 martin users  138 Aug  9 11:56 .
drwxr-xr-x 1 martin users 4590 Aug  9 11:56 ..
lrwxrwxrwx 1 martin users   22 Aug  9 11:56 browserslist -> ../browserslist/cli.js
lrwxrwxrwx 1 martin users   32 Aug  9 11:56 browserslist-lint -> ../update-browserslist-db/cli.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:56 cssesc -> ../cssesc/bin/cssesc
lrwxrwxrwx 1 martin users   25 Aug  9 11:56 esparse -> ../esprima/bin/esparse.js
lrwxrwxrwx 1 martin users   28 Aug  9 11:56 esvalidate -> ../esprima/bin/esvalidate.js
lrwxrwxrwx 1 martin users   25 Aug  9 11:56 js-yaml -> ../js-yaml/bin/js-yaml.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:56 mkdirp -> ../mkdirp/bin/cmd.js
lrwxrwxrwx 1 martin users   16 Aug  9 11:56 svgo -> ../svgo/bin/svgo

@wmertens
Copy link
Contributor

wmertens commented Aug 9, 2022

Hmm interesting, it seems that npm flattens the tree as much as possible, only moving dependencies up if they would clash. This then provides the binaries you see.

IMHO if you're not asking for binaries you shouldn't be getting them either, so I'm more of a fan of the purity that pnpm gives you.

@tinybeachthor
Copy link
Contributor Author

I think you're right, but then nodejs never made much sense to begin with.
I believe it's best to make dream2nix behave consistently with the non-nix approach.

@wmertens
Copy link
Contributor

wmertens commented Aug 9, 2022

I'll make it optional like pnpm does. Yarn does things differently too.
The pure version is the most efficient one, so I'd like to keep it as an option

@tinybeachthor
Copy link
Contributor Author

@wmertens

I get the following with both npm and yarn:

> ls -la node_modules/.bin
total 32
drwxr-xr-x 1 martin users  138 Aug 10 08:45 .
drwxr-xr-x 1 martin users 4612 Aug 10 08:45 ..
lrwxrwxrwx 1 martin users   22 Aug 10 08:45 browserslist -> ../browserslist/cli.js
lrwxrwxrwx 1 martin users   32 Aug 10 08:45 browserslist-lint -> ../update-browserslist-db/cli.js
lrwxrwxrwx 1 martin users   20 Aug 10 08:45 cssesc -> ../cssesc/bin/cssesc
lrwxrwxrwx 1 martin users   25 Aug 10 08:45 esparse -> ../esprima/bin/esparse.js
lrwxrwxrwx 1 martin users   28 Aug 10 08:45 esvalidate -> ../esprima/bin/esvalidate.js
lrwxrwxrwx 1 martin users   25 Aug 10 08:45 js-yaml -> ../js-yaml/bin/js-yaml.js
lrwxrwxrwx 1 martin users   20 Aug 10 08:45 mkdirp -> ../mkdirp/bin/cmd.js
lrwxrwxrwx 1 martin users   16 Aug 10 08:45 svgo -> ../svgo/bin/svgo

With pnpm:

> ls -la node_modules/.bin
total 4
drwxr-xr-x 1 martin users   8 Aug 10 08:47 .
drwxr-xr-x 1 martin users  66 Aug 10 08:47 ..
-rwxr-xr-x 1 martin users 520 Aug 10 08:47 svgo

I feel like the default behavior of dream2nix should be the same as npm.

@wmertens
Copy link
Contributor

Right, I hear you - I think can make that work.

@wmertens
Copy link
Contributor

wmertens commented Aug 10, 2022

ok fixed. Not sure if I like it though, for me it's a huge set of binaries that gets added to path, including many dumb ones like rc, and resolve which doesn't even work.

@wmertens
Copy link
Contributor

I think I prefer putting it behind a flag. It's a tiny change - 39245db

tinybeachthor added a commit to tinybeachthor/dream2nix that referenced this issue Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants