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

Dev dependencies missing from node_modules #147

Open
paulyoung opened this issue Oct 22, 2018 · 31 comments
Open

Dev dependencies missing from node_modules #147

paulyoung opened this issue Oct 22, 2018 · 31 comments

Comments

@paulyoung
Copy link
Contributor

paulyoung commented Oct 22, 2018

I generated nix expressions with the following command:

$ nixfromnpm -f ./package.json -o nixfromnpm --dev-depth 1

Despite dev dependencies appearing in ./nixfromnpm/nodePackages/default.nix and ./project.nix, they are missing from the node_modules directory when I build using the generated default.nix file.

How can I make sure they are present in node_modules?

@paulyoung
Copy link
Contributor Author

I think the answer probably lies in skipDevDependencyCleanup = true; as suggested in #73 (comment), but I'm not sure where to do that just yet.

@paulyoung
Copy link
Contributor Author

I cloned the project and pointed to my local copy. After doing some sensible things to make sure dev dependency cleanup is skipped I still can't find dev dependencies in node_modules.

@paulyoung
Copy link
Contributor Author

@adnelson @ixmatus any suggestions?

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 7, 2018

Are you sure they are not in the node_modules? nixfromnpm constructs a hierarchical node_modules so it could be that the dev dependencies are in the package's node_modules that depends on them. If it is at the top-level I would expect to see it at the top-level (at least we do for our usage of it with Awake's front-end javascript application).

Is this a package you can share the source to? Or at least the package.json's dependencies?

@paulyoung
Copy link
Contributor Author

My package.json looks like this:

{
  "private": true,
  "name": "my-project",
  "version": "0.1.0",
  "dependencies": {
    "borc": "^2.0.4"
  },
  "devDependencies": {
    "css-loader": "^1.0.0",
    "style-loader": "^0.23.0",
    "webpack": "^4.21.0",
    "webpack-cli": "^3.1.2",
    "webpack-dev-server": "^3.1.9"
  }
}

With the above, there's only a borc directory, no .bin directory so things like . ${nodeModules}/.bin/webpack-dev-server fail.

I'm currently doing this as a workaround:

{
  "private": true,
  "name": "my-project",
  "version": "0.1.0",
  "_dependencies": {
    "borc": "^2.0.4"
  },
  "_devDependencies": {
    "css-loader": "^1.0.0",
    "style-loader": "^0.23.0",
    "webpack": "^4.21.0",
    "webpack-cli": "^3.1.2",
    "webpack-dev-server": "^3.1.9"
  },
  "dependencies": {
    "borc": "^2.0.4",

    "css-loader": "^1.0.0",
    "style-loader": "^0.23.0",
    "webpack": "^4.21.0",
    "webpack-cli": "^3.1.2",
    "webpack-dev-server": "^3.1.9"
  }
}

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 8, 2018

@paulyoung I will try to reproduce with this over the weekend.

@paulyoung
Copy link
Contributor Author

@ixmatus thanks. You might run into the same issue I did in adnelson/nix-node-packages#40

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 11, 2018

I don't usually use the nix-node-packages. I didn't have a chance over the weekend (I got sick...) but may have some time tonight or tomorrow night to try and reproduce and help out.

@paulyoung
Copy link
Contributor Author

I don't usually use the nix-node-packages.

@ixmatus what do you normally do?

I'm following the process in the README but terser-webpack-plugin (the package triggering the issue mentioned in adnelson/nix-node-packages#40) isn't actually in the nix-node-packages

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

@paulyoung I just don't use them. I override packages as I need to. I tried to reproduce with your package.json but ran into an issue that might be orthogonal to this one:

>>=  nix build -f ./release.nix my-project
builder for '/nix/store/9hfbgakpc8qr72dqfvmxjhdhnz43pxqq-my-project-0.1.0-nodejs-10.9.0.drv' failed with exit code 1; last 10 log lines:
  Error: This must be run in a directory with a package.json
      at Object.<anonymous> (/nix/store/xfy752w66yrlwbxin7h3nfqqsslwmjlb-node-build-tools/bin/.check-package-json-wrapped:6:9)
      at Module._compile (internal/modules/cjs/loader.js:689:30)
      at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
      at Module.load (internal/modules/cjs/loader.js:599:32)
      at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
      at Function.Module._load (internal/modules/cjs/loader.js:530:3)
      at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
      at startup (internal/bootstrap/node.js:266:19)
      at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
[11 built (1 failed), 22 copied (74.9 MiB), 30.5 MiB DL]
error: build of '/nix/store/9hfbgakpc8qr72dqfvmxjhdhnz43pxqq-my-project-0.1.0-nodejs-10.9.0.drv' failed

I'll be poking at this one throughout the day.

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

@paulyoung hilariously, unrelated to your example project, I ran into an issue with terser-webpack-plugin on our own software. The issue is that it specifies an empty bin (why?) the solution I've implemented for Awake is to override the derivation is clobber the package's package.json with one in which the bin key is removed.

@paulyoung
Copy link
Contributor Author

@ixmatus any advice on how to do this is appreciated. Perhaps we could update the README?

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

A fix (which I will put a PR out for soon) for this would be to ignore empty bin strings.

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

@paulyoung I can provide an example here. Overriding the derivations is pretty straight-forward and follows standard Nix derivation override idioms. Examples would be useful in the README though so I will see about creating a separate PR for that.

@paulyoung
Copy link
Contributor Author

A fix (which I will put a PR out for soon) for this would be to ignore empty bin strings.

Yes, I was planning to do this but haven't looked into much. As mentioned in adnelson/nix-node-packages#40, I think the intention is to do that already.

I still think the fix would need to be made to nix-node-packages though, right?

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

@paulyoung here is a minimal working example of what we do, the following Nix expression is found in a file named <my-package>/default.nix and the directory <my-package> contains a directory nodePackages which was generated by nixfromnpm (i.e. nixfromnpm -f my-package.json -o my-package/nodePackages):

{ callPackage, coreutils, haskellPackages, makeWrapper,
  nodejs-10_x, pkgs, runCommand }:
let
  clobberPackageJSON = path: oldAttrs: {
    derivationOverrides.patchPhase = (oldAttrs.patchPhase or "") + ''
      cp ${path} package.json
    '';
  };
in
(import ./nodePackages {
  nodejs = nodejs-10_x;
  inherit pkgs;
}).override { overrides = nodePackagesNew: nodePackagesOld: rec {

    # We clobber package.json files for the following node packages instead of
    # using patch, because 1-line patchfiles over the entire contents is pretty
    # much the same thing (when a package.json file isn't pretty-printed). Most
    # of these hacks are to remove stuff like:
    #
    # "bin": "",
    #
    # from the toplevel.

    terser-webpack-plugin       = terser-webpack-plugin_1-1-0;
    terser-webpack-plugin_1-1-0 = nodePackagesOld.terser-webpack-plugin_1-1-0.overrideNodePackage
      (clobberPackageJSON ./terser-webpack-plugin_1-1-0.json);

  };
}

... note that we had to "clobberPackageJSON" for the following packages:

  • file-loader
  • mini-css-extract-plugin
  • url-loader
  • terser-webpack-plugin

@paulyoung
Copy link
Contributor Author

FWIW I think I'm familiar enough with overriding fields in nix attribute sets and derivations but perhaps a bit confused since I followed the README and used nix-node-packages.

@paulyoung
Copy link
Contributor Author

Super helpful, thanks!

@paulyoung
Copy link
Contributor Author

Still going to keep this issue open since it's actually about missing dev dependencies 😄

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

And terser-webpack-plugin_1-1-0.json is the package.json for that package minus the "bin": "", line, it's the full file but the diff of the file looks like this:

--- package.json   2018-12-13 16:48:17.000000000 -0600
+++ package.json      2018-12-13 16:41:02.000000000 -0600
@@ -7,7 +7,6 @@
   "author": "webpack Contrib Team",
   "homepage": "https://github.com/webpack-contrib/terser-webpack-plugin",
   "bugs": "https://github.com/webpack-contrib/terser-webpack-plugin/issues",
-  "bin": "",
   "main": "dist/cjs.js",
   "engines": {
     "node": ">= 6.9.0 <7.0.0 || >= 8.9.0"

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 13, 2018

Yes leave this open, I still have not reproduced your original issue description.

@paulyoung
Copy link
Contributor Author

@ixmatus I saw your comment about clobbering vs patching and I think I might want/need to patch anyway. How were you getting the non-pretty printed package.json file?

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 18, 2018

@paulyoung I was going to the package's github project and the tagged version, then copying the package.json and pretty printing it in Emacs before writing it.

@paulyoung
Copy link
Contributor Author

I must be doing something wrong since patching that way seems to have no effect.

@paulyoung
Copy link
Contributor Author

I'm doing this:

terser-webpack-plugin_1-1-0 = nodePackagesOld.terser-webpack-plugin_1-1-0.overrideNodePackage (_: {
  derivationOverrides.apply = [
    ../patches/terser-webpack-plugin_1-1-0.patch
  ];
});

@ixmatus
Copy link
Collaborator

ixmatus commented Dec 20, 2018

Shouldn't it be:

terser-webpack-plugin_1-1-0 = nodePackagesOld.terser-webpack-plugin_1-1-0.overrideNodePackage (oldDrv: {
  derivationOverrides.patches = (oldDrv.patches or []) ++ [
    ../patches/terser-webpack-plugin_1-1-0.patch
  ];
});

@paulyoung
Copy link
Contributor Author

@ixmatus I know it's been a while but were you able to reproduce the issue?

@paulyoung
Copy link
Contributor Author

And to answer your previous questions, the docs say this:

# An 'overrideNodePackage' attribute, which will call
# `buildNodePackage` with new arguments produced by the given
# arg-override function. The function consumes the original
# argument set.
#
# N.B: the legacy behavior of accepting a set is preserved but
# the preferred usage-pattern is to supply a function that
# discards its argument; e.g:
#
# overrideNodePackage (_: { ... })

@paulyoung
Copy link
Contributor Author

paulyoung commented Mar 6, 2019

I think the issues with patch is this:

# These are the arguments that we will pass to `stdenv.mkDerivation`.
mkDerivationArgs = removeAttrs args attrsToRemove // {
inherit
buildPhase
checkPhase
configurePhase
doCheck
dontStrip
fullName
installPhase
meta
patchPhase
nodejsSources
src;

@adnelson
Copy link
Owner

adnelson commented Mar 7, 2019

@paulyoung sorry I've been inactive on this, but how about using postPatch as in this example? https://github.com/adnelson/nix-node-packages/blob/master/nodePackages/detect-character-encoding/0.2.1.nix

Removing patchPhase like this is probably not a great idea, especially without a warning! Also it's pretty poorly documented.

@paulyoung
Copy link
Contributor Author

@adnelson I used patchPhase (as in @ixmatus' clobberPackageJSON example) except to accept a .patch file that I apply manually instead.

The real issue here is the one relating to missing dev dependencies.

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

No branches or pull requests

3 participants