nix-community / dream2nix

Simplified nix packaging for various programming language ecosystems [maintainer=@DavHau]
https://dream2nix.dev
MIT License
985 stars 122 forks source link

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

Closed tinybeachthor closed 2 years ago

tinybeachthor commented 2 years ago

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 commented 2 years ago

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

wmertens commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

wmertens commented 2 years ago

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 commented 2 years ago

I think I prefer putting it behind a flag. It's a tiny change - https://github.com/nix-community/dream2nix/pull/195/commits/39245db1c93443726014345cade723a2624c7ba0