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

Bundle production dependencies #5013

Closed
gajus opened this Issue Jan 20, 2016 · 25 comments

Comments

Projects
None yet
8 participants
@gajus
Contributor

gajus commented Jan 20, 2016

ESLint is used only in development environment. It is a popular tool with well maintained tests suit. As such, I think it would benefit from using NPM bundledDependencies package.json property. Specifically, this would make the install time of ESLint a lot faster and would lift restrictions of the modularisation.

The build process can be automated to include all production dependencies. A simple NPM script can be used to streamline the process:

{
    "bundle-dependencies": "bundled-dependencies",
    "bundle-prepare": "npm run bundle-dependencies; git commit -m 'Updated bundle dependencies.' ./package.json; rm -fr ./node_modules; npm install --production; npm publish; npm install;"
}

An example of a package using bundledDependecies is canonical. To experience the benefits, try to install canonical version that is using bundledDependecies (3.0.0+) and compare it against a version that is not using it (2.5.0). Here are the test results on my machine:

npm install canonical@3.0.1

10.81s user
4.45s system
110% cpu
13.790 total
npm install canonical@2.5.0

13.17s user
2.77s system
27% cpu
58.136 total

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Jan 20, 2016

@gajus Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@gajus Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 20, 2016

@gajus gajus changed the title from Bundle dependencies to Bundle production dependencies Jan 20, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 20, 2016

Member

Can you explain how this could fit into our current release script? And what does the script you pasted above do?

Member

nzakas commented Jan 20, 2016

Can you explain how this could fit into our current release script? And what does the script you pasted above do?

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Jan 20, 2016

Contributor

https://www.npmjs.com/package/bundled-dependencies program reads package.json and copies names of production dependencies to package.json bundledDependencies property. It is used when the intention is to bundle all production dependencies and you do not want to maintain the bundledDependencies property by hand.

The NPM script is a simple convenience that uses bash script to execute bundled-dependencies program, remove current ./node_modules folder, re-install all dependencies using npm install --production (to the best of my knowledge, ./node_modules must not contain development dependencies when doing npm publish of a package with bundledDependencies) and publish the package, i.e.

npm run bundle-dependencies;
git commit -m 'Updated bundle dependencies.' ./package.json;
git push
rm -fr ./node_modules;
npm install --production;
npm dedupe;
npm prune --production;
npm publish;
npm install;

The difference to the build process would be that instead of npm publish user would need to run the latter script (npm run bundle-publish) and bundled-dependencies would become a development dependency.

Contributor

gajus commented Jan 20, 2016

https://www.npmjs.com/package/bundled-dependencies program reads package.json and copies names of production dependencies to package.json bundledDependencies property. It is used when the intention is to bundle all production dependencies and you do not want to maintain the bundledDependencies property by hand.

The NPM script is a simple convenience that uses bash script to execute bundled-dependencies program, remove current ./node_modules folder, re-install all dependencies using npm install --production (to the best of my knowledge, ./node_modules must not contain development dependencies when doing npm publish of a package with bundledDependencies) and publish the package, i.e.

npm run bundle-dependencies;
git commit -m 'Updated bundle dependencies.' ./package.json;
git push
rm -fr ./node_modules;
npm install --production;
npm dedupe;
npm prune --production;
npm publish;
npm install;

The difference to the build process would be that instead of npm publish user would need to run the latter script (npm run bundle-publish) and bundled-dependencies would become a development dependency.

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Feb 9, 2016

Contributor

23:52 @ilyavolodin
I know I'm not familiar with bundledDependencies, I have no idea how much it will improve installation speed, and if it's even something we should worry about
If it's easy enough to do that change locally and time it before and after, it would help a lot (no idea how, maybe through gziping eslint and doing local install)

First, without bundledDependencies:

mkdir test
cd test
npm cache clean
time npm install eslint@2.0.0-rc.0
8.08s user
1.91s system
22% cpu
44.387 total

Now, lets make a package using bundle dependencies:

git clone git@github.com:eslint/eslint.git
cd eslint
npm install
sed -i -e 's/"eslint",/"eslint-bundled",/' ./package.json
npm install bundled-dependencies --save-dev
node ./node_modules/.bin/bundled-dependencies
git commit -m 'Updated bundle dependencies.' ./package.json
rm -fr ./node_modules
npm install --production
npm dedupe
npm prune --production
npm publish

To install the package that uses bundled dependencies:

mkdir test
cd test
npm cache clean
time npm install eslint-bundled@2.0.0-rc.0
5.92s user
2.29s system
97% cpu
8.403 total

To summarise: eslint-bundled installs 5.28 times faster than eslint (difference between 44.387 and 8.403 seconds).

Contributor

gajus commented Feb 9, 2016

23:52 @ilyavolodin
I know I'm not familiar with bundledDependencies, I have no idea how much it will improve installation speed, and if it's even something we should worry about
If it's easy enough to do that change locally and time it before and after, it would help a lot (no idea how, maybe through gziping eslint and doing local install)

First, without bundledDependencies:

mkdir test
cd test
npm cache clean
time npm install eslint@2.0.0-rc.0
8.08s user
1.91s system
22% cpu
44.387 total

Now, lets make a package using bundle dependencies:

git clone git@github.com:eslint/eslint.git
cd eslint
npm install
sed -i -e 's/"eslint",/"eslint-bundled",/' ./package.json
npm install bundled-dependencies --save-dev
node ./node_modules/.bin/bundled-dependencies
git commit -m 'Updated bundle dependencies.' ./package.json
rm -fr ./node_modules
npm install --production
npm dedupe
npm prune --production
npm publish

To install the package that uses bundled dependencies:

mkdir test
cd test
npm cache clean
time npm install eslint-bundled@2.0.0-rc.0
5.92s user
2.29s system
97% cpu
8.403 total

To summarise: eslint-bundled installs 5.28 times faster than eslint (difference between 44.387 and 8.403 seconds).

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Feb 9, 2016

Contributor

The biggest and the only downside is that you bundle the dependencies, e.g. if you have a dependency on lodash ^4.0.0 and ^4.0.1 comes out, users who install ESLint will get the version thats bundled with the package.

However, given a regular release cycle of ESLint, I see this more of a benefit (release stability does not depend on possible new bugs in the dependencies) than an burden.

Contributor

gajus commented Feb 9, 2016

The biggest and the only downside is that you bundle the dependencies, e.g. if you have a dependency on lodash ^4.0.0 and ^4.0.1 comes out, users who install ESLint will get the version thats bundled with the package.

However, given a regular release cycle of ESLint, I see this more of a benefit (release stability does not depend on possible new bugs in the dependencies) than an burden.

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Feb 9, 2016

Member

I think the benchmark is a little bit unrealistic. Normally you would install eslint in a project which has a lot of other dependencies and the possibility is quite high that the project has some modules in common with eslint (I guess glob or lodash is in almost every dependency tree).

I’ve measured the time installing eslint-bundled in an existing project and it turns out that it takes longer than installing the non-bundled version.

$ cd existing-project/
$ npm cache clean
$ time npm i eslint@v2.0.0-rc.0
npm i eslint@v2.0.0-rc.0  11.49s user 1.05s system 72% cpu 17.269 total
$ npm cache clean
$ time npm i eslint-bundled@v2.0.0-rc.0
npm i eslint-bundled@v2.0.0-rc.0  16.74s user 3.31s system 89% cpu 22.446 total
Member

lo1tuma commented Feb 9, 2016

I think the benchmark is a little bit unrealistic. Normally you would install eslint in a project which has a lot of other dependencies and the possibility is quite high that the project has some modules in common with eslint (I guess glob or lodash is in almost every dependency tree).

I’ve measured the time installing eslint-bundled in an existing project and it turns out that it takes longer than installing the non-bundled version.

$ cd existing-project/
$ npm cache clean
$ time npm i eslint@v2.0.0-rc.0
npm i eslint@v2.0.0-rc.0  11.49s user 1.05s system 72% cpu 17.269 total
$ npm cache clean
$ time npm i eslint-bundled@v2.0.0-rc.0
npm i eslint-bundled@v2.0.0-rc.0  16.74s user 3.31s system 89% cpu 22.446 total
@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Feb 9, 2016

Contributor

Is this benchmark done using npm 2 or npm 3?

On Feb 9, 2016, at 02:47, Mathias Schreck notifications@github.com wrote:

I think the benchmark is a little bit unrealistic. Normally you would install eslint in a project which has a lot of other dependencies and the possibility is quite high that the project has some modules in common with eslint (I guess glob or lodash is in almost every dependency tree).

I’ve measured the time installing eslint-bundled in an existing project and it turns out that it takes longer than installing the non-bundled version.

$ cd existing-project/
$ npm cache clean
$ time npm i eslint@v2.0.0-rc.0
npm i eslint@v2.0.0-rc.0 11.49s user 1.05s system 72% cpu 17.269 total
$ npm cache clean
$ time npm i eslint-bundled@v2.0.0-rc.0
npm i eslint-bundled@v2.0.0-rc.0 16.74s user 3.31s system 89% cpu 22.446 total

Reply to this email directly or view it on GitHub.

Contributor

gajus commented Feb 9, 2016

Is this benchmark done using npm 2 or npm 3?

On Feb 9, 2016, at 02:47, Mathias Schreck notifications@github.com wrote:

I think the benchmark is a little bit unrealistic. Normally you would install eslint in a project which has a lot of other dependencies and the possibility is quite high that the project has some modules in common with eslint (I guess glob or lodash is in almost every dependency tree).

I’ve measured the time installing eslint-bundled in an existing project and it turns out that it takes longer than installing the non-bundled version.

$ cd existing-project/
$ npm cache clean
$ time npm i eslint@v2.0.0-rc.0
npm i eslint@v2.0.0-rc.0 11.49s user 1.05s system 72% cpu 17.269 total
$ npm cache clean
$ time npm i eslint-bundled@v2.0.0-rc.0
npm i eslint-bundled@v2.0.0-rc.0 16.74s user 3.31s system 89% cpu 22.446 total

Reply to this email directly or view it on GitHub.

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Feb 9, 2016

Member

npm 3.3.6

Member

lo1tuma commented Feb 9, 2016

npm 3.3.6

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 24, 2016

Member

Given recent events (http://www.theregister.co.uk/2016/03/23/npm_left_pad_chaos/), I think we should strongly consider doing this. It would scratch two itches:

  1. Locking down dependencies so the ones we use while publishing are the ones people get
  2. Preventing deletion of a dependency from breaking our users

It looks like there are some unclear perf implications to this, but even so, it's starting to seem like this is the right choice for keeping our users safe.

@eslint/eslint-team other thoughts?

Member

nzakas commented Mar 24, 2016

Given recent events (http://www.theregister.co.uk/2016/03/23/npm_left_pad_chaos/), I think we should strongly consider doing this. It would scratch two itches:

  1. Locking down dependencies so the ones we use while publishing are the ones people get
  2. Preventing deletion of a dependency from breaking our users

It looks like there are some unclear perf implications to this, but even so, it's starting to seem like this is the right choice for keeping our users safe.

@eslint/eslint-team other thoughts?

@xjamundx

This comment has been minimized.

Show comment
Hide comment
@xjamundx

xjamundx Mar 24, 2016

Contributor

Looks really interesting, we have a ton of dependencies right now, so this may be worth doing.

~/dev/eslint (fix-indent-autofix) $ npm ls
eslint@2.0.0 /Users/jamuferguson/dev/eslint
├─┬ chalk@1.1.1
│ ├── ansi-styles@2.1.0
│ ├── escape-string-regexp@1.0.3
│ ├── has-ansi@2.0.0
│ ├── strip-ansi@3.0.0
│ └── supports-color@2.0.0
├─┬ concat-stream@1.5.1
│ ├── inherits@2.0.1
│ ├─┬ readable-stream@2.0.4
│ │ ├── core-util-is@1.0.1
│ │ ├── process-nextick-args@1.0.3
│ │ ├── string_decoder@0.10.31
│ │ └── util-deprecate@1.0.2
│ └── typedarray@0.0.6
├─┬ debug@2.2.0
│ └── ms@0.7.1
├─┬ doctrine@1.1.0
│ ├── esutils@1.1.6
│ └── isarray@0.0.1
├─┬ es6-map@0.1.3
│ ├── d@0.1.1
│ ├── es5-ext@0.10.11
│ ├── es6-iterator@2.0.0
│ ├── es6-set@0.1.4
│ ├── es6-symbol@3.0.2
│ └── event-emitter@0.3.4
├─┬ escope@3.4.0
│ ├── es6-weak-map@2.0.1
│ └─┬ esrecurse@3.1.1
│   └── estraverse@3.1.0
├─┬ espree@3.0.1
│ ├── acorn@2.7.0
│ └─┬ acorn-jsx@2.0.1
│   └── acorn@2.7.0
├── esprima@2.7.0
├── esprima-fb@15001.1001.0-dev-harmony-fb
├── estraverse@4.1.1
├── estraverse-fb@1.3.1
├── esutils@2.0.2
├─┬ file-entry-cache@1.2.4
│ ├─┬ flat-cache@1.0.10
│ │ ├─┬ del@2.0.2
│ │ │ ├─┬ globby@3.0.1
│ │ │ │ ├─┬ array-union@1.0.1
│ │ │ │ │ └── array-uniq@1.0.2
│ │ │ │ ├── arrify@1.0.0
│ │ │ │ └── glob@5.0.15
│ │ │ ├── is-path-cwd@1.0.0
│ │ │ ├─┬ is-path-in-cwd@1.0.0
│ │ │ │ └── is-path-inside@1.0.0
│ │ │ ├── pify@2.3.0
│ │ │ ├─┬ pinkie-promise@1.0.0
│ │ │ │ └── pinkie@1.0.0
│ │ │ └─┬ rimraf@2.4.3
│ │ │   └── glob@5.0.15
│ │ ├── graceful-fs@4.1.2
│ │ ├─┬ read-json-sync@1.1.0
│ │ │ └── graceful-fs@3.0.8
│ │ └── write@0.2.1
│ └── object-assign@4.0.1
├─┬ glob@6.0.4
│ ├─┬ inflight@1.0.4
│ │ └── wrappy@1.0.1
│ ├─┬ minimatch@3.0.0
│ │ └─┬ brace-expansion@1.1.1
│ │   ├── balanced-match@0.2.1
│ │   └── concat-map@0.0.1
│ └── once@1.3.2
├── globals@8.18.0
├── ignore@2.2.19
├─┬ inquirer@0.11.0
│ ├── ansi-escapes@1.1.0
│ ├── ansi-regex@2.0.0
│ ├─┬ cli-cursor@1.0.2
│ │ └─┬ restore-cursor@1.0.1
│ │   ├── exit-hook@1.1.1
│ │   └── onetime@1.0.0
│ ├── cli-width@1.1.0
│ ├── figures@1.4.0
│ ├── lodash@3.10.1
│ ├─┬ readline2@1.0.1
│ │ ├─┬ code-point-at@1.0.0
│ │ │ └── number-is-nan@1.0.0
│ │ ├── is-fullwidth-code-point@1.0.0
│ │ └── mute-stream@0.0.5
│ ├── run-async@0.1.0
│ └── rx-lite@3.1.2
├─┬ is-my-json-valid@2.12.2
│ ├── generate-function@2.0.0
│ ├─┬ generate-object-property@1.2.0
│ │ └── is-property@1.0.2
│ ├── jsonpointer@2.0.0
│ └── xtend@4.0.1
├─┬ is-resolvable@1.0.0
│ └── tryit@1.0.2
├─┬ js-yaml@3.5.2
│ └─┬ argparse@1.0.3
│   ├── lodash@3.10.1
│   └── sprintf-js@1.0.3
├─┬ json-stable-stringify@1.0.0
│ └── jsonify@0.0.0
├── lodash@4.3.0
├─┬ mkdirp@0.5.1
│ └── minimist@0.0.8
├─┬ optionator@0.8.1
│ ├── deep-is@0.1.3
│ ├── fast-levenshtein@1.1.3
│ ├── levn@0.3.0
│ ├── prelude-ls@1.1.2
│ ├── type-check@0.3.2
│ └── wordwrap@1.0.0
├── path-is-absolute@1.0.0
├── path-is-inside@1.0.1
├── pluralize@1.2.1
├── progress@1.1.8
├── resolve@1.1.6
├── shelljs@0.5.3
├── strip-json-comments@1.0.4
├─┬ table@3.7.8
│ ├── bluebird@3.2.2
│ ├── slice-ansi@0.0.4
│ ├── string-width@1.0.1
│ ├── tv4@1.2.7
│ └── xregexp@3.0.0
├── text-table@0.2.0
├── through@2.3.8
└─┬ user-home@2.0.0
  └── os-homedir@1.0.1
Contributor

xjamundx commented Mar 24, 2016

Looks really interesting, we have a ton of dependencies right now, so this may be worth doing.

~/dev/eslint (fix-indent-autofix) $ npm ls
eslint@2.0.0 /Users/jamuferguson/dev/eslint
├─┬ chalk@1.1.1
│ ├── ansi-styles@2.1.0
│ ├── escape-string-regexp@1.0.3
│ ├── has-ansi@2.0.0
│ ├── strip-ansi@3.0.0
│ └── supports-color@2.0.0
├─┬ concat-stream@1.5.1
│ ├── inherits@2.0.1
│ ├─┬ readable-stream@2.0.4
│ │ ├── core-util-is@1.0.1
│ │ ├── process-nextick-args@1.0.3
│ │ ├── string_decoder@0.10.31
│ │ └── util-deprecate@1.0.2
│ └── typedarray@0.0.6
├─┬ debug@2.2.0
│ └── ms@0.7.1
├─┬ doctrine@1.1.0
│ ├── esutils@1.1.6
│ └── isarray@0.0.1
├─┬ es6-map@0.1.3
│ ├── d@0.1.1
│ ├── es5-ext@0.10.11
│ ├── es6-iterator@2.0.0
│ ├── es6-set@0.1.4
│ ├── es6-symbol@3.0.2
│ └── event-emitter@0.3.4
├─┬ escope@3.4.0
│ ├── es6-weak-map@2.0.1
│ └─┬ esrecurse@3.1.1
│   └── estraverse@3.1.0
├─┬ espree@3.0.1
│ ├── acorn@2.7.0
│ └─┬ acorn-jsx@2.0.1
│   └── acorn@2.7.0
├── esprima@2.7.0
├── esprima-fb@15001.1001.0-dev-harmony-fb
├── estraverse@4.1.1
├── estraverse-fb@1.3.1
├── esutils@2.0.2
├─┬ file-entry-cache@1.2.4
│ ├─┬ flat-cache@1.0.10
│ │ ├─┬ del@2.0.2
│ │ │ ├─┬ globby@3.0.1
│ │ │ │ ├─┬ array-union@1.0.1
│ │ │ │ │ └── array-uniq@1.0.2
│ │ │ │ ├── arrify@1.0.0
│ │ │ │ └── glob@5.0.15
│ │ │ ├── is-path-cwd@1.0.0
│ │ │ ├─┬ is-path-in-cwd@1.0.0
│ │ │ │ └── is-path-inside@1.0.0
│ │ │ ├── pify@2.3.0
│ │ │ ├─┬ pinkie-promise@1.0.0
│ │ │ │ └── pinkie@1.0.0
│ │ │ └─┬ rimraf@2.4.3
│ │ │   └── glob@5.0.15
│ │ ├── graceful-fs@4.1.2
│ │ ├─┬ read-json-sync@1.1.0
│ │ │ └── graceful-fs@3.0.8
│ │ └── write@0.2.1
│ └── object-assign@4.0.1
├─┬ glob@6.0.4
│ ├─┬ inflight@1.0.4
│ │ └── wrappy@1.0.1
│ ├─┬ minimatch@3.0.0
│ │ └─┬ brace-expansion@1.1.1
│ │   ├── balanced-match@0.2.1
│ │   └── concat-map@0.0.1
│ └── once@1.3.2
├── globals@8.18.0
├── ignore@2.2.19
├─┬ inquirer@0.11.0
│ ├── ansi-escapes@1.1.0
│ ├── ansi-regex@2.0.0
│ ├─┬ cli-cursor@1.0.2
│ │ └─┬ restore-cursor@1.0.1
│ │   ├── exit-hook@1.1.1
│ │   └── onetime@1.0.0
│ ├── cli-width@1.1.0
│ ├── figures@1.4.0
│ ├── lodash@3.10.1
│ ├─┬ readline2@1.0.1
│ │ ├─┬ code-point-at@1.0.0
│ │ │ └── number-is-nan@1.0.0
│ │ ├── is-fullwidth-code-point@1.0.0
│ │ └── mute-stream@0.0.5
│ ├── run-async@0.1.0
│ └── rx-lite@3.1.2
├─┬ is-my-json-valid@2.12.2
│ ├── generate-function@2.0.0
│ ├─┬ generate-object-property@1.2.0
│ │ └── is-property@1.0.2
│ ├── jsonpointer@2.0.0
│ └── xtend@4.0.1
├─┬ is-resolvable@1.0.0
│ └── tryit@1.0.2
├─┬ js-yaml@3.5.2
│ └─┬ argparse@1.0.3
│   ├── lodash@3.10.1
│   └── sprintf-js@1.0.3
├─┬ json-stable-stringify@1.0.0
│ └── jsonify@0.0.0
├── lodash@4.3.0
├─┬ mkdirp@0.5.1
│ └── minimist@0.0.8
├─┬ optionator@0.8.1
│ ├── deep-is@0.1.3
│ ├── fast-levenshtein@1.1.3
│ ├── levn@0.3.0
│ ├── prelude-ls@1.1.2
│ ├── type-check@0.3.2
│ └── wordwrap@1.0.0
├── path-is-absolute@1.0.0
├── path-is-inside@1.0.1
├── pluralize@1.2.1
├── progress@1.1.8
├── resolve@1.1.6
├── shelljs@0.5.3
├── strip-json-comments@1.0.4
├─┬ table@3.7.8
│ ├── bluebird@3.2.2
│ ├── slice-ansi@0.0.4
│ ├── string-width@1.0.1
│ ├── tv4@1.2.7
│ └── xregexp@3.0.0
├── text-table@0.2.0
├── through@2.3.8
└─┬ user-home@2.0.0
  └── os-homedir@1.0.1
@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 24, 2016

Member

@xjamundx that list includes our devDependencies

Member

alberto commented Mar 24, 2016

@xjamundx that list includes our devDependencies

@xjamundx

This comment has been minimized.

Show comment
Hide comment
@xjamundx

xjamundx Mar 24, 2016

Contributor

@alberto updated!

Contributor

xjamundx commented Mar 24, 2016

@alberto updated!

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Mar 24, 2016

Member

it's starting to seem like this is the right choice for keeping our users safe.

I'm not sure if I really agree with this. If our users are relying on NPM to download us, then bundling dependencies is only a partial solution where the real problem resides in NPM itself. I think this is something NPM should resolve, not us. Unless people complain about this kind of events more I'm 👎 for this proposal.

Member

BYK commented Mar 24, 2016

it's starting to seem like this is the right choice for keeping our users safe.

I'm not sure if I really agree with this. If our users are relying on NPM to download us, then bundling dependencies is only a partial solution where the real problem resides in NPM itself. I think this is something NPM should resolve, not us. Unless people complain about this kind of events more I'm 👎 for this proposal.

@nzakas nzakas added accepted and removed evaluating labels Mar 24, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 24, 2016

Member

then bundling dependencies is only a partial solution where the real problem resides in NPM itself.

This isn't true. Bundling dependencies is a complete solution. The problem occurs when npm tries to resolve dependencies at install time. Bundling the dependencies means dependencies are resolved at build time - that's one time for everyone who downloads it. That means we know with 100% certainty that each user is installing exactly what was released.

We've been bitten in the past by people doing fresh installs where a dependency was updated in an incompatible way - bundling dependencies also prevents that.

The only real downside is that npm can't do deduping when installed - and I don't see that as a big downside considering npm 1 and 2 didn't do deduping anyway. :)

I'm going to start working on this.

Member

nzakas commented Mar 24, 2016

then bundling dependencies is only a partial solution where the real problem resides in NPM itself.

This isn't true. Bundling dependencies is a complete solution. The problem occurs when npm tries to resolve dependencies at install time. Bundling the dependencies means dependencies are resolved at build time - that's one time for everyone who downloads it. That means we know with 100% certainty that each user is installing exactly what was released.

We've been bitten in the past by people doing fresh installs where a dependency was updated in an incompatible way - bundling dependencies also prevents that.

The only real downside is that npm can't do deduping when installed - and I don't see that as a big downside considering npm 1 and 2 didn't do deduping anyway. :)

I'm going to start working on this.

@nzakas nzakas self-assigned this Mar 24, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 24, 2016

Member

For reference, there was a discussion about this feature in the chatroom earlier today. My apologies for not transferring some of that onto this issue. You can read the relevant bits starting here:
https://gitter.im/eslint/eslint?at=56f3565a137455d638d26a92

Member

nzakas commented Mar 24, 2016

For reference, there was a discussion about this feature in the chatroom earlier today. My apologies for not transferring some of that onto this issue. You can read the relevant bits starting here:
https://gitter.im/eslint/eslint?at=56f3565a137455d638d26a92

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 25, 2016

Member

Re: deletion of a package causes issues downstream, it's going to be addressed by npm per their blog (section "What happens next")

Re: locking down deps, it could also be solved by shrinkwrap. Should we discuss the pros/cons of each approach?

Member

alberto commented Mar 25, 2016

Re: deletion of a package causes issues downstream, it's going to be addressed by npm per their blog (section "What happens next")

Re: locking down deps, it could also be solved by shrinkwrap. Should we discuss the pros/cons of each approach?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 25, 2016

Member

@alberto npm hasn't said what they will do or when they will do it, so I'm uncomfortable considering that enough to change what was decided here. I think we should proceed with bundling as planned and evaluate it for a couple of releases. It's easy to undo or change to shrink-wrapping later if we want, I just don't want to reopen the discussion at this point.

Member

nzakas commented Mar 25, 2016

@alberto npm hasn't said what they will do or when they will do it, so I'm uncomfortable considering that enough to change what was decided here. I think we should proceed with bundling as planned and evaluate it for a couple of releases. It's easy to undo or change to shrink-wrapping later if we want, I just don't want to reopen the discussion at this point.

@nzakas nzakas closed this in 646f863 Mar 25, 2016

nzakas added a commit that referenced this issue Mar 25, 2016

Merge pull request #5676 from eslint/issue5013
Build: Bundle dependencies in package.json (fixes #5013)
@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Mar 28, 2016

Contributor

If there is a need to automate the process, you can use https://github.com/gajus/bundle-dependencies.

Generates bundledDependencies package.json value using values of the dependencies property. Updates package.json definition using the generated bundledDependencies value.

Contributor

gajus commented Mar 28, 2016

If there is a need to automate the process, you can use https://github.com/gajus/bundle-dependencies.

Generates bundledDependencies package.json value using values of the dependencies property. Updates package.json definition using the generated bundledDependencies value.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 28, 2016

Member

@gajus https://github.com/eslint/eslint-release is already using it, but we are having issues with npm@2. See #5687 and #5680

Member

alberto commented Mar 28, 2016

@gajus https://github.com/eslint/eslint-release is already using it, but we are having issues with npm@2. See #5687 and #5680

@nzakas nzakas reopened this Mar 28, 2016

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 28, 2016

Member

This didn't quite work the way we had hoped, so we reverted this functionality for now. There appears to be a bug in npm 2 with regards to bundled dependencies:
#5687 (comment)

Member

nzakas commented Mar 28, 2016

This didn't quite work the way we had hoped, so we reverted this functionality for now. There appears to be a bug in npm 2 with regards to bundled dependencies:
#5687 (comment)

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Mar 30, 2016

Member

What if we used shrinkwrap + bundleDependencies? I feel like that would solve these issues.

Member

BYK commented Mar 30, 2016

What if we used shrinkwrap + bundleDependencies? I feel like that would solve these issues.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 30, 2016

Member

@BYK That's what we did. The issue is due to the known bug in NPM 2 with bundleDependancies. Shrinkwrap doesn't help with that:-(

Member

ilyavolodin commented Mar 30, 2016

@BYK That's what we did. The issue is due to the known bug in NPM 2 with bundleDependancies. Shrinkwrap doesn't help with that:-(

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 30, 2016

Member

We can definitely at least do shrinkwrap during the release, so we pin the dependencies to what was there during the release process. I don't think we can do bundling until we find a way to address that npm 2 bug.

Member

nzakas commented Mar 30, 2016

We can definitely at least do shrinkwrap during the release, so we pin the dependencies to what was there during the release process. I don't think we can do bundling until we find a way to address that npm 2 bug.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 30, 2016

Member

I think the fix might need to be on @gajus side. I'll open an issue there.

Edit: Opened an issue here: gajus/bundle-dependencies#3

Member

ilyavolodin commented Mar 30, 2016

I think the fix might need to be on @gajus side. I'll open an issue there.

Edit: Opened an issue here: gajus/bundle-dependencies#3

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 1, 2016

Member

We didn't get very far with this and there doesn't seem to be a big demand, so closing.

Member

nzakas commented Sep 1, 2016

We didn't get very far with this and there doesn't seem to be a big demand, so closing.

@nzakas nzakas closed this Sep 1, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

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