FormidableLabs / inspectpack

An inspection tool for Webpack frontend JavaScript bundles.
MIT License
592 stars 20 forks source link

Feature: Make `better-sqlite3` / caching feature an optional dependency #49

Closed gartz closed 7 years ago

gartz commented 7 years ago

The latest version added a package better-sqlite3 that depends on integer@1.0.1 that requires python to run a script after install.

Can we keep only JS dependencies, please? It makes easier to be cross-compatible.

ryan-roemer commented 7 years ago

Can you explain more? My understanding is that better-sqlite3 is fully cross-compatible on all OSes.

python is the basis of https://github.com/nodejs/node-gyp which has been around since forever IIRC as the basis of native bindings building. Are you saying that you'd like us to scrub all potential dependencies to make sure there are no native bindings anywhere?

And as a potentially helpful tip, I'd generally recommend that you never actually install software on your running servers. Just on your CI server, which builds all packages -- then tar that up, archive the artifact, and deploy the artifact to your servers -- to ensure that you never are at the mercy of realtime installs where things could go wrong / need to be shrinkwrap/package-lock'ed, etc.

/cc @tptee

gartz commented 7 years ago

It installs optional dependencies that goes to package-lock that only works on the os-x, then it fails to build in the CI, because the package-lock try to install the OS X optional dependency and exit 1.

My CI uses a Linux alpine with node only. I need the dev packages to create the build. I do not build in production, also I do not have python or other unnecessary stuff in production.

Anyway so far the solution is not to use this project as dev dependency for now.

You would be surprised how far you can go without node-gyp. I have been working in a few huge projects that does not have it as dependency.

On Sep 20, 2017, 9:34 PM +0900, Ryan Roemer notifications@github.com, wrote:

Can you explain more? My understanding is that better-sqlite3 is fully cross-compatible on all OSes. python is the basis of https://github.com/nodejs/node-gyp which has been around since forever IIRC as the basis of native bindings building. Are you saying that you'd like us to scrub all potential dependencies to make sure there are no native bindings anywhere? And as a potentially helpful tip, I'd generally recommend that you never actually install software on your running servers. Just on your CI server, which builds all packages -- then tar that up, archive the artifact, and deploy the artifact to your servers -- to ensure that you never are at the mercy of realtime installs where things could go wrong / need to be shrinkwrap/package-lock'ed, etc. /cc @tptee — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

ryan-roemer commented 7 years ago

@gartz -- Hmmm... while it does seem awkward that you need to reach out to any project that has any transitive dependency on anything using node-gyp, in our case, we use sqlite as a caching tool, so it's not strictly necessary. If we moved that to optionalDependencies would that work for you?

(For example, chokidar which comes up all the time and I'd be surprised if you didn't have a transitive dependency on with any of the collection of modern JS build tools has an optionalDependencies on fsevents. The fsevents things has come up specifically with dev's trying to do locking on a different OS than the one that's expected -- definitely not a recommended practice 😉 )

ryan-roemer commented 7 years ago

Driving home the chokidar example more concretely -- it's needed for webpack:

(nvm v8.3.0) rye@small ~/Desktop/temp-wp $ npm install webpack

> fsevents@1.1.2 install /Users/rye/Desktop/temp-wp/node_modules/fsevents
> node install

[fsevents] Success: "/Users/rye/Desktop/temp-wp/node_modules/fsevents/lib/binding/Release/node-v57-darwin-x64/fse.node" already installed
Pass --update-binary to reinstall or --build-from-source to recompile

# ... SNIPPED ...

(nvm v8.3.0) rye@small ~/Desktop/temp-wp $ find node_modules | grep binding.gyp
node_modules/fsevents/binding.gyp

... and I'm strongly intuiting your build uses webpack given you're using this project 😉

gartz commented 7 years ago

I need to double check that, because before you add that to your project my webpack project wasn’t installing fsevents. It started with this update.

From all the dependencies I use here the only one that have fsevents is PM2, but that is available in the server and doesn’t need to be as dependency in the project.

About node-gyp, the last project that was using it was scss, but we don’t use that nowadays.

After you add this SQLite dependency, it came with fsevents, not sure why your webpack is installing fsevents, but here it’s not. It would break the build in the CI, it does everything it goes to the package-lock.

On Sep 21, 2017, 1:28 AM +0900, Ryan Roemer notifications@github.com, wrote:

Driving home the chokidar example more concretely -- it's needed for webpack: (nvm v8.3.0) rye@small ~/Desktop/temp-wp $ npm install webpack

fsevents@1.1.2 install /Users/rye/Desktop/temp-wp/node_modules/fsevents node install

[fsevents] Success: "/Users/rye/Desktop/temp-wp/node_modules/fsevents/lib/binding/Release/node-v57-darwin-x64/fse.node" already installed Pass --update-binary to reinstall or --build-from-source to recompile

... SNIPPED ...

(nvm v8.3.0) rye@small ~/Desktop/temp-wp $ find node_modules | grep binding.gyp node_modules/fsevents/binding.gyp ... and I'm strongly intuiting your build uses webpack given you're using this project 😉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ryan-roemer commented 7 years ago

Because fsevents is an optionalDepedency in chokidar. That's why I'm proposing that as a potential solution to help just your bespoke CI constraints.

Yarn shows the deps:

chokidar@^1.7.0:
  version "1.7.0"
  resolved "https://registry.yarnpkg.com/chokidar/-/chokidar-1.7.0.tgz#798e689778151c8076b4b360e5edd28cda2bb468"
  dependencies:
    anymatch "^1.3.0"
    async-each "^1.0.0"
    glob-parent "^2.0.0"
    inherits "^2.0.1"
    is-binary-path "^1.0.0"
    is-glob "^2.0.0"
    path-is-absolute "^1.0.0"
    readdirp "^2.0.0"
  optionalDependencies:
    fsevents "^1.0.0"

===> 

watchpack@^1.4.0:
  version "1.4.0"
  resolved "https://registry.yarnpkg.com/watchpack/-/watchpack-1.4.0.tgz#4a1472bcbb952bd0a9bb4036801f954dfb39faac"
  dependencies:
    async "^2.1.2"
    chokidar "^1.7.0"
    graceful-fs "^4.1.2"

===> 

webpack@^3.6.0:
  version "3.6.0"
  resolved "https://registry.yarnpkg.com/webpack/-/webpack-3.6.0.tgz#a89a929fbee205d35a4fa2cc487be9cbec8898bc"
  dependencies:
    acorn "^5.0.0"
    acorn-dynamic-import "^2.0.0"
    ajv "^5.1.5"
    ajv-keywords "^2.0.0"
    async "^2.1.2"
    enhanced-resolve "^3.4.0"
    escope "^3.6.0"
    interpret "^1.0.0"
    json-loader "^0.5.4"
    json5 "^0.5.1"
    loader-runner "^2.3.0"
    loader-utils "^1.1.0"
    memory-fs "~0.4.1"
    mkdirp "~0.5.0"
    node-libs-browser "^2.0.0"
    source-map "^0.5.3"
    supports-color "^4.2.1"
    tapable "^0.2.7"
    uglifyjs-webpack-plugin "^0.4.6"
    watchpack "^1.4.0"
    webpack-sources "^1.0.1"
    yargs "^8.0.2"

So if you're planning on using and eventually upgrading webpack to this one, you'll have an optionaldep on fsevents. Honestly, I'm guessing that has been there a very, very long time.

ryan-roemer commented 7 years ago

And fsevents is not part of the inspectpack dependency tree:

$ yarn add inspectpack
$ find node_modules | grep binding.gyp
node_modules/better-sqlite3/binding.gyp
node_modules/farmhash/binding.gyp
node_modules/integer/binding.gyp
$ cat yarn.lock | grep fsevents
# ... NO OUTPUT ...

Side note: farmhash is already an optionalDependencies

gartz commented 7 years ago

Indeed actually there are other dependencies that include fsevents as optional and it is present in my package-lock.json. However they don't fail during the build. Let me show you this:

npm info lifecycle integer@1.0.1~install: integer@1.0.1

> integer@1.0.1 install /code/node_modules/integer
> node tools/install

gyp info it worked if it ends with ok
gyp info using node-gyp@3.6.2
gyp info using node@8.5.0 | linux | x64
gyp ERR! configure error
gyp ERR! stack Error: Can't find Python executable "python", you can set the PYTHON env variable.
gyp ERR! stack     at PythonFinder.failNoPython (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:483:19)
gyp ERR! stack     at PythonFinder.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:397:16)
gyp ERR! stack     at F (/usr/local/lib/node_modules/npm/node_modules/which/which.js:68:16)
gyp ERR! stack     at E (/usr/local/lib/node_modules/npm/node_modules/which/which.js:80:29)
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/which.js:89:16
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/node_modules/isexe/index.js:42:5
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/node_modules/isexe/mode.js:8:5
gyp ERR! stack     at FSReqWrap.oncomplete (fs.js:153:21)
gyp ERR! System Linux 4.9.49-moby
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /code/node_modules/integer
gyp ERR! node -v v8.5.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok
npm info lifecycle integer@1.0.1~install: Failed to exec install script
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! integer@1.0.1 install: `node tools/install`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the integer@1.0.1 install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

This is what happens after:

npm install webpack-dashboard@next --save-dev

Now let's see without it by running:

npm uninstall webpack-dashboard --save-dev

To be sure the only thing I removed was this project, because inspectpack is the dependency that is breaking webpack-dashboard latest version.

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 1814 packages in 24.598s
npm info ok

Unsupported dependencies are trying to install in the linux build. No errors or warnings about the node-gyp yes it's a webpack project.

fsevents is in my package-lock.json, but kcheck this out:

    "fsevents": {
      "version": "1.1.2",
      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.1.2.tgz",
      "integrity": "sha512-Sn44E5wQW4bTHXvQmvSHwqbuiXtduD6Rrjm2ZtUEGbyrig+nUH3t/QD4M4/ZXViY556TBpRgZkHLDx3JxPwxiw==",
      "dev": true,
      "optional": true,
      "requires": {
        "nan": "2.7.0",
        "node-pre-gyp": "0.6.36"
      },

As you mentioned, it is an optional, so no conflicts.

I think that if you move those dependencies to optional, it would be great because I want to install in my OS-X but I don't need them to build the project in my controlled build environment on the CI server.

Thank you for checking that and inspect all those packages. This is a great project and I like to use it if I can.