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 packages include peer dependencies #89269

Closed

Conversation

terlar
Copy link
Contributor

@terlar terlar commented May 31, 2020

Motivation for this change

Some packages, for example tsun have dependencies defined as peerDependencies. By default these won't be included by node2nix. I think there might be other packages that are suffering from the same issue. If we give the flag --include-peer-dependencies they will be included as package dependencies.

This was reported in and fixes #88046 where a user noticed you can't just run the package tsun directly but also needs to provide an environment with typescript and node within it to work.

Things done
  • Added the flag to the generation script.
  • Ran the generation script to update all the node packages.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@calbrecht calbrecht left a comment

Choose a reason for hiding this comment

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

Not sure if this is really what we want. PeerDependencies are ment to be installed next to module and not inside it's node_modules. See https://classic.yarnpkg.com/en/docs/dependency-types#toc-peerdependencies

@@ -1363,6 +1363,24 @@ let
sha1 = "cbc4b9a68981bf0b501ccd06a9058acd65309bf7";
};
};
"@discordjs/opus-0.1.0" = {
Copy link
Member

Choose a reason for hiding this comment

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

this is probably breaking without the native opus lib in the inputs. But we can keep it like so until someone actually wants to use it :)

@@ -40949,6 +41076,15 @@ let
sha1 = "77b005e32d1bfc96e560fedd5d7eedcf120f87e3";
};
};
"sodium-2.0.3" = {
Copy link
Member

Choose a reason for hiding this comment

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

this will need libsodium, but the same as above applies :).

@terlar
Copy link
Contributor Author

terlar commented Jun 2, 2020

I see, the thing is packages would need to have their peerDependencies to work, but to do it the nix way we would want those packages to be installed as their own derivations and then linked in. But in that case we would have to do something like this:
terlar@040444a
For every such package, would we prefer that instead? That was my first idea.

The drawback of doing like this is only that the package is bundled together with the package and therefore duplicated across the store in every place where that peer dependency is used. But is that really so bad?

@calbrecht
Copy link
Member

@terlar, in the case of a "cli" node package it makes totally sense, in my opinion, to have it wrapped the way you do it. Maybe if we duplicate the package under another name and give it a "-bin" suffix, or the like. To make it clear that this is bundled and executable.

That way the package could be used "bundled" as a cli application as well as a simple node-module. What do you think?

@terlar
Copy link
Contributor Author

terlar commented Jun 2, 2020

@calbrecht That is a good point, I will do that and close this PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodePackages.tsun doesn't start up
2 participants