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

Why is `node-pre-gyp` a bundled dependency? #157

Closed
eatrocks opened this Issue Dec 6, 2016 · 23 comments

Comments

Projects
None yet
7 participants
@eatrocks

eatrocks commented Dec 6, 2016

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

This comment has been minimized.

Member

bnoordhuis commented Dec 7, 2016

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

@es128

This comment has been minimized.

Member

es128 commented Dec 7, 2016

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

This comment has been minimized.

Member

es128 commented Dec 7, 2016

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

This comment has been minimized.

springmeyer commented Dec 7, 2016

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 mapbox/node-sqlite3#720 (which impacted windows users after I tried unbundling node-pre-gyp from node-sqlite3).

@es128

This comment has been minimized.

Member

es128 commented Dec 7, 2016

@eatrocks does that answer your question?

@eatrocks

This comment has been minimized.

eatrocks commented Dec 7, 2016

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

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Dec 8, 2016

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

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

@dunnock

This comment has been minimized.

dunnock commented Mar 28, 2017

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

@dunnock dunnock referenced this issue Mar 28, 2017

Closed

node-pre-gyp ERR! #15104

0 of 12 tasks complete
@springmeyer

This comment has been minimized.

springmeyer commented Mar 30, 2017

@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

This comment has been minimized.

dunnock commented Mar 30, 2017

@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

This comment has been minimized.

springmeyer commented Mar 30, 2017

@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

This comment has been minimized.

OliverJAsh commented Sep 30, 2017

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

This comment has been minimized.

Member

bnoordhuis commented Sep 30, 2017

@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

This comment has been minimized.

OliverJAsh commented Sep 30, 2017

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 30, 2017

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

This comment has been minimized.

OliverJAsh commented Sep 30, 2017

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 fseventsnode_modules folder because of the pre-bundled modules? This is why I suggested removing node-pre-gyp from the package dependencies.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 30, 2017

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

This comment has been minimized.

OliverJAsh commented Sep 30, 2017

@bnoordhuis I think that would work!

@OliverJAsh

This comment has been minimized.

OliverJAsh commented Oct 26, 2017

Can we re-open this?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 27, 2017

Reopened but see my comment in #187.

@OliverJAsh

This comment has been minimized.

OliverJAsh commented Aug 11, 2018

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

@springmeyer

This comment has been minimized.

springmeyer commented Aug 12, 2018

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

This comment has been minimized.

Contributor

pipobscure commented Nov 8, 2018

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

@pipobscure pipobscure closed this Nov 8, 2018

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