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.

Sign up for GitHub

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

Jump to bottom

Feature request: support npm ci #159

Closed
mmorearty opened this issue Mar 15, 2018 · 10 comments · Fixed by #2328
Closed

Feature request: support npm ci #159

mmorearty opened this issue Mar 15, 2018 · 10 comments · Fixed by #2328
Assignees
@lukasholzer
Labels
enhancement package:builtin not in any of the npm packages
Milestone
3.0

Comments

@mmorearty
Copy link
Contributor

See this blog post for an introduction to npm ci. It would be nice if rules_nodejs had a npm_ci command in addition to its npm_install command.
@alexeagle
Copy link
Collaborator

Is there any reason we would want both? Just for updating the package.lock file? In that case you could run bazel run @nodejs//:npm
I think we should just change the behavior of npm_install to run npm ci
@gregmagolan ?

@mmorearty
Copy link
Contributor Author

I'm not aware of a reason we would want both, but I haven't thought about it too deeply. Certainly it seems that Bazel is the kind of context where you might be fine with just npm ci, and not having npm install at all.

@gregmagolan
Copy link
Collaborator

gregmagolan commented Mar 16, 2018 •
edited

Yes. I agree that npm ci is a better fit for bazel. Not sure about changing the behavior of npm_install as users may expect it to run npm install. If its going to break compatibility anyway (since the package_json attribute will be removed and the package_lock_json attribute made mandatory) then may as well just make an npm_ci rule and possibly remove npm_install or mark it as deprecated for future removal.

@gregmagolan
Copy link
Collaborator

npm ci was added recently with npm v5.7.0. This doesn't ship with node yet but it could be updated in Bazel with npm update -g npm@5.7.1.

@gregmagolan
Copy link
Collaborator

Not as easy as I hoped to update npm to 5.7.1 within Bazel. Also, there are some issues with npm 5.7.1 which kept node from including it. They are waiting for npm 5.8.0 before including it in node 8.x LTS. See nodejs/node#19271. That should be coming soon.

@zkat
Copy link

zkat commented Mar 21, 2018

npm ci is very new, so one reason you might want to support both is so you're not stuck with npm ci if you find an incompatibility (while we fix it)

@alexeagle
Copy link
Collaborator

alexeagle commented Mar 22, 2018 via email

@gregmagolan
Copy link
Collaborator

Still waiting on a node 8.x.x release with npm 5.7.x. Release list with npm versions are published here: https://nodejs.org/en/download/releases/

@alexeagle
Copy link
Collaborator

we now default to node 10 so I think this is unblocked, we should change npm_install to have the npm ci behavior

@alexeagle alexeagle removed the blocked label Nov 30, 2018
@alexeagle alexeagle added this to the 1.0 milestone Dec 18, 2018
@gregmagolan gregmagolan added the package:builtin not in any of the npm packages label Oct 8, 2019
@alexeagle alexeagle removed this from the 1.0 milestone Nov 14, 2019
@alexeagle alexeagle added this to the 3.0 milestone Oct 27, 2020
@rayman1104 rayman1104 self-assigned this Nov 3, 2020
@alexeagle alexeagle assigned lukasholzer and unassigned rayman1104 Dec 1, 2020
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 2, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
bb0989e 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
@lukasholzer lukasholzer mentioned this issue Dec 2, 2020
Merged
12 tasks
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 2, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
cddd4fe 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 8, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
dfaaa69 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 15, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
ee5b9df 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 16, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
1d187ff 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 16, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
0ec59a0 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
lukasholzer pushed a commit to lukasholzer/rules_nodejs that referenced this issue Dec 16, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
887c496 
…dules

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes bazelbuild#159
alexeagle pushed a commit that referenced this issue Dec 18, 2020
feat(builtin): use npm ci as default behaviour for installing node_mo…
1d650fb 
…dules (#2328)

To be more hermetic with the install of the dependencies use npm ci to install the exact version from the package-lock.json file.

To update a dependency use the vendored npm binary with `bazel run @nodejs//:npm install <dep-name>`.

Fixes #159
@alexeagle
Copy link
Collaborator

fixed in #2328

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

@lukasholzer lukasholzer

Labels
enhancement package:builtin not in any of the npm packages
Projects
None yet
Milestone
3.0
Development

Successfully merging a pull request may close this issue.

feat(builtin): use npm ci as default behaviour for installing node_modules lukasholzer/rules_nodejs
7 participants
@zkat @alexeagle @soldair @mmorearty @gregmagolan @rayman1104 @lukasholzer