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

feat(builtin): yarn install use --frozen-lockfile as default #2327

Merged
merged 1 commit into from
Dec 15, 2020
Merged

feat(builtin): yarn install use --frozen-lockfile as default #2327

merged 1 commit into from
Dec 15, 2020

Conversation

lukasholzer
Copy link
Collaborator

@lukasholzer lukasholzer commented Dec 2, 2020 •
edited
Loading

To be more hermetic with the install of the dependencies use the frozen lockfile flag to install the exact version from the yarn.lock file.

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

Fixes #941

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Currently the yarn_install rule will use the yarn install command and installs the latest version that is possible in the range of the packag.json

Issue Number: #941

What is the new behavior?

The new behaviour is using the frozen lock file option for yarn installs, this means that it will install the exact version from the yarn.lock file

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Dec 2, 2020
@lukasholzer lukasholzer self-assigned this Dec 2, 2020
@lukasholzer lukasholzer mentioned this pull request Dec 2, 2020
Merged
12 tasks
@lukasholzer lukasholzer force-pushed the feat/yarn-install-frozen-lockfile-default branch from 38f1d43 to 555243e Compare December 6, 2020 09:04
@lukasholzer lukasholzer force-pushed the feat/yarn-install-frozen-lockfile-default branch 6 times, most recently from d2254ea to 59da753 Compare December 6, 2020 16:35
@lukasholzer lukasholzer marked this pull request as draft December 6, 2020 16:43
@lukasholzer lukasholzer force-pushed the feat/yarn-install-frozen-lockfile-default branch from 59da753 to e61ede6 Compare December 6, 2020 17:25
@lukasholzer lukasholzer changed the base branch from stable to 3.x December 8, 2020 15:58
@lukasholzer lukasholzer force-pushed the feat/yarn-install-frozen-lockfile-default branch 2 times, most recently from 880d3b4 to 4ac7bf3 Compare December 8, 2020 19:29
@lukasholzer lukasholzer marked this pull request as ready for review December 13, 2020 14:04
alexeagle
alexeagle reviewed Dec 13, 2020
View reviewed changes
examples/worker/WORKSPACE Outdated
# builds and use the `--frozen-lockfile`. We have to disable this here as the @bazel
# scoped packages are getting replaced in the package.json with a file url and the version
# won't match with the one in the yarn.lock file then.
frozen_lockfile = False,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding these to the actual code isn't needed, it's a bunch of extra explanation for users, and when they cd into this folder to run the example, frozen_lockfile will work fine.
We have a plan to fix the integration test runner to use .tar file urls which I think will work with frozen lockfile. Therefore the bug is local to the integration test runner, so the workaround should live there too.

Here's the spot where we already modify this file as we copy it into the test fixture
https://github.com/bazelbuild/rules_nodejs/blob/3.x/internal/bazel_integration_test/test_runner.js#L222

Can you just make the change to yarn_install there? can be syntactically trivial edit like
yarn_install(\n -> yarn_install(\n frozen_lockfile=False,\n

Then we shouldn't need to change any examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexeagle Thx for the hint – yea I prefer this a lot 👍 Change already applied ;)

feat(builtin): yarn install use --frozen-lockfile as default
1a92287 
To be more hermetic with the install of the dependencies use the frozen lockfile flag to install the exact version from the `yarn.lock` file.

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

Fixes bazel-contrib#941
@lukasholzer lukasholzer force-pushed the feat/yarn-install-frozen-lockfile-default branch from 4ac7bf3 to 1a92287 Compare December 14, 2020 21:19
@alexeagle alexeagle merged commit b6a8cbb into bazel-contrib:3.x Dec 15, 2020
@alexeagle alexeagle mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeagle alexeagle alexeagle approved these changes

@gregmagolan gregmagolan Awaiting requested review from gregmagolan gregmagolan is a code owner

@mattem mattem Awaiting requested review from mattem mattem is a code owner

@soldair soldair Awaiting requested review from soldair soldair is a code owner

@alan-agius4 alan-agius4 Awaiting requested review from alan-agius4

@mrmeku mrmeku Awaiting requested review from mrmeku

Assignees

@lukasholzer lukasholzer

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

yarn_install should use --frozen-lockfile
2 participants
@lukasholzer @alexeagle