Why is node-pre-gyp a bundled dependency?

[eatrocks]

This isn't a bug nor an issue, however I'm curious why you chose to make node-pre-gyp a bundled dependency.

I noticed because despite only one occurrence of node-pre-gyp in my dependency tree, npm (v3+) didn't flatten it into the root of my node_modules. I'm assuming that's because it's a bundled dependency.

I found this same non-flattening behavior with jstransform and esprima-fb but in that case esprima-fb is not a bundled dependency of jstransform, so there must be a different cause in that case.

thanks.

[bnoordhuis]

I'm not sure, to be honest. Might have just been an oversight in #59. @es128?

[es128]

I had thought it was necessary for it to work, but I can't say with any certainty that my assumption was right or that it hasn't changed.

May be worth reevaluating

[es128]

npm i https://github.com/strongloop/fsevents/tarball/unbundle

It does seem to work smoothly. I guess as a next step we could try publishing to npm with a prerelease tag to help make sure.

@indieisaconcept @springmeyer do either of you have any insight regarding why this has been a bundled dependency so far?

[springmeyer]

I would recommend sticking with bundling. If you don't bundle then downstream installs that also include node-pre-gyp (or a module that uses is) may trigger npm's dedupe behavior. Then, if node-pre-gyp is deduped it may not be available in time to be used to install fsevents (originally discovered at mapbox/node-pre-gyp#61 (comment)). This is a really tricky issue because it only manifests downstream. There are some proposed workarounds to this, like using preinstall to install node-pre-gyp but those seem to suffer from bugs, notably TryGhost/node-sqlite3#720 (which impacted windows users after I tried unbundling node-pre-gyp from node-sqlite3).

[es128]

@eatrocks does that answer your question?

[eatrocks]

Yes, that's what I was hoping to learn. Thanks @springmeyer , @bnoordhuis , and @es128

[bnoordhuis]

Thanks for chiming in @springmeyer, I learned something today.

(Also, that's a pretty bad flaw in npm.)

[dunnock]

Thanks for details @springmeyer , looks like nasty bug in npm, has it been reported there?

[springmeyer]

@dunnock - it boils down to: node-pre-gyp must be bundled. If it is should not be a problem or npm bug that is involved. This ticket was about why the bundling is needed. As far as I know fsevents has always bundled node-pre-gyp. So there is no npm bug to blame. If you are hitting an install problem with fs-events being installed with node-pre-gyp, then the problem is likely something different that what is discussed above.

[dunnock]

@springmeyer thanks for clarification, I get yarn check warning which is present across all of projects using fsevents:

$ yarn check
yarn check v0.21.3
warning "\u001b[2mchokidar#\u001b[22mfsevents#node-pre-gyp@^0.6.29" could be deduped from "0.6.33" to "node-pre-gyp@0.6.33"

[springmeyer]

@dunnock is there another package in your tree that depends on node-pre-gyp? Also, is that a fatal error or just a warning?

[OliverJAsh]

I am also seeing this Yarn warning.

In my yarn.lock you can see the dependency fsevents has on node-pre-gyp:

fsevents@^1.0.0:
  version "1.1.2"
  resolved "https://registry.yarnpkg.com/fsevents/-/fsevents-1.1.2.tgz#3282b713fb3ad80ede0e9fcf4611b5aa6fc033f4"
  dependencies:
    nan "^2.3.0"
    node-pre-gyp "^0.6.36"

fsevents already bundles node-pre-gyp, but Yarn will install its own copy nevertheless, leading to the warning.

Can we remove it from dependencies in package.json? Surely it doesn't need to be there if it's already bundled? This should fix the Yarn warning lots of people are complaining about: yarnpkg/yarn#2287 (comment)

[bnoordhuis]

@OliverJAsh I think you misunderstand. That yarn warning means there is more than one package in your dependencies that depends on node-pre-gyp, not that fsevents doesn't have to depend on node-pre-gyp.

[OliverJAsh]

I have double checked and this is the only package depending on that module.

…

On Sat, 30 Sep 2017, 12:55 Ben Noordhuis, ***@***.***> wrote: @OliverJAsh <https://github.com/oliverjash> I think you misunderstand. That yarn warning means there is more than one package in your dependencies that depends on node-pre-gyp, not that fsevents doesn't have to depend on node-pre-gyp. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4QCQcGo-tN8PsfhO1EXkwHcAHgxl3zks5sniwlgaJpZM4LF68t> .

[bnoordhuis]

Unlikely, otherwise why would yarn print that error? Note that it could be a order n dependency, i.e., from somewhere in the full dependency graph, not just what you have listed in your package.json.

[OliverJAsh]

You can try this in an empty Yarn project:

# chokidar depends on fsevents
$ yarn add chokidar
$ yarn check
warning "chokidar#fsevents#node-pre-gyp@^0.6.36" could be deduped from "0.6.38" to "node-pre-gyp@0.6.38"

If I inspect yarn.lock I can see that the only dependency (including any transitive dependencies) which depends on node-pre-gyp is fsevents. You can try searching the yarn.lock for node-pre-gyp and you will find no other dependency references.

fsevents@^1.0.0:
  version "1.1.2"
  resolved "https://registry.yarnpkg.com/fsevents/-/fsevents-1.1.2.tgz#3282b713fb3ad80ede0e9fcf4611b5aa6fc033f4"
  dependencies:
    nan "^2.3.0"
    node-pre-gyp "^0.6.36"

To demonstrate that I have two versions in my Node modules:

~/Development/temp/yarn-fsevents-warning
❯ ls node_modules/node-pre-gyp
CHANGELOG.md	LICENSE		README.md	appveyor.yml	bin		contributing.md	lib		node_modules	package.json

~/Development/temp/yarn-fsevents-warning
❯ ls node_modules/fsevents/node_modules/node-pre-gyp
CHANGELOG.md		README.md		bin			contributing.md		package.json
LICENSE			appveyor.yml		cloudformation		lib			template-name.diff

I could be missing something, but it looks like there is nothing else relying on node-pre-gyp.

If nothing else depends on node-pre-gyp then I'm not sure why Yarn decides to install node-pre-gyp at the root node_modules. Maybe because it can't place it in fsevents’ node_modules folder because of the pre-bundled modules? This is why I suggested removing node-pre-gyp from the package dependencies.

[bnoordhuis]

Ah okay, I think I understand now. Is this what you are requesting we change?

diff --git a/package.json b/package.json
index 2ed9082..5b3a185 100644
--- a/package.json
+++ b/package.json
@@ -4,8 +4,7 @@
   "description": "Native Access to Mac OS-X FSEvents",
   "main": "fsevents.js",
   "dependencies": {
-    "nan": "^2.3.0",
-    "node-pre-gyp": "^0.6.36"
+    "nan": "^2.3.0"
   },
   "os": [
     "darwin"
@@ -44,6 +43,7 @@
   ],
   "homepage": "https://github.com/strongloop/fsevents",
   "devDependencies": {
+    "node-pre-gyp": "^0.6.36",
     "tap": "~0.4.8"
   }
 }

[OliverJAsh]

@bnoordhuis I think that would work!

[OliverJAsh]

Can we re-open this?

[bnoordhuis]

Reopened but see my comment in #187.

[OliverJAsh]

#187 was fixed, so can we fix this issue now?

[springmeyer]

I previously said:

I would recommend sticking with bundling.

And

it boils down to: node-pre-gyp must be bundled.

Recently I found this is no longer the case with recent npm. I can no longer trigger the problems I describe above with node >= 4. So I've now unbundled node-pre-gyp from most of the deps I maintain that use it, and removed the bundling recommendations at mapbox/node-pre-gyp#403

So, I think it is 👍 for fsevents to no longer bundle node-pre-gyp and rather just list it in the normal dependencies and expect npm to have it on PATH in time for its use no matter where it gets deduped (since it seems like this works now). If someone finds a problem with this method (of no longer bundling) please report to https://github.com/mapbox/node-pre-gyp/issues and /cc @springmeyer and @mapsam

[pipobscure]

We'll remove node-pre-gyp as part of moving to N-API. This is in #237