Support save-exact true #4471

ahmedelgabri · 2017-09-15T10:30:25Z

Summary

Fixes #4343. Currently there is no way to remove the package prefix inside .yarnrc file, this PR add support for save-exact in .yarnrc as discussed in #4343. Full credit goes to @jambonrose

save-exact true

One small thing, should yarn be backwards compatible with the old behavior which is save-prefix ''? We can just add an extra check here for do this. What do you think @BYK?

} else if (exact || Boolean(this.config.getOption('save-exact')) || Boolean(this.config.getOption('save-prefix'))) {

Test plan

New unit test.

StefanoMagrassi · 2017-09-15T10:50:20Z

This should be so obvious: if I set save-exact to true the prefix must be automatically set to an empty string (otherwise there is some kind of inconsistency).

The yarn parser should also allow to set save-prefix key to an empty string. Now it throwns an error ("Invalid value type in .yarnrc").

BYK · 2017-09-15T13:23:18Z

This should be so obvious: if I set save-exact to true the prefix must be automatically set to an empty string (otherwise there is some kind of inconsistency).

Agreed.

The yarn parser should also allow to set save-prefix key to an empty string. Now it throwns an error ("Invalid value type in .yarnrc").

This is not true. You can set it to an empty string.

BYK · 2017-09-15T13:23:51Z

One small thing, should yarn be backwards compatible with the old behavior which is save-prefix ''? We can just add an extra check here for do this. What do you think @BYK?

It should definitely be backwards compatible, otherwise this change would require a major version upgrade.

BYK

Thanks for the PR!

Tests are passing.

This is not enough :) You also need to add a new test(s) to test the new behavior.

BYK · 2017-09-15T13:29:09Z

src/cli/commands/add.js

@@ -83,7 +83,7 @@ export class Add extends Install {
      let prefix;
      if (tilde) {
        prefix = '~';
-      } else if (exact) {
+      } else if (exact || Boolean(this.config.getOption('save-exact'))) {


I think it is better to do

const exact = this.flags.exact || Boolean(this.config.getOption('save-exact')); const tilde = this.flags.tilde;

above. save-exact should take precedence over save-prefix option.

StefanoMagrassi · 2017-09-15T13:43:19Z

This is not true. You can set it to an empty string.

Really? My colleague got that error when tried to set save-prefix to an empty string (yarn version 1.0.2).

Maybe it was his fault or caused by some kind of other wrong configuration... In this case 👍

ahmedelgabri · 2017-09-15T14:58:04Z

This is not enough :) You also need to add a new test(s) to test the new behavior.

@BYK True :) I might need some pointers on how to test the .yarnrc, a quick look at https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/add.js & I can't seem to find anything related to testing .yarnrc, I might be wrong.

ahmedelgabri · 2017-09-15T18:25:41Z

I added a test & changed the logic to be backward compatible.

const exact =
      this.flags.exact ||
      Boolean(this.config.getOption('save-exact')) ||
      String(this.config.getOption('save-prefix')) === '';

The save-prefix check can't be Boolean(this.config.getOption('save-prefix')) becasue it will always return true for non-empty strings.

BYK

Nice, thanks a lot! Also, sorry for the late response. The patch looks good but tests need some small improvements and using a variable for the config value would be great.

After those, I don't see any reason to not merge this.

BYK · 2017-09-16T17:50:37Z

__tests__/fixtures/add/install-strict-all/.npmrc

@@ -0,0 +1 @@
+yarn-offline-mirror=./mirror-for-offline


Why a separate file? Let's move this into .yarnrc below.

Honestly, I just copied install-no-strict-all and just added .yarnrc :)

Maybe actually this is not needed at all?

BYK · 2017-09-16T17:51:50Z

__tests__/fixtures/add/install-strict-all/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "mime-types": "^2.0.0"


I don't think you need this at all since you already do yarn add left-pad in the test

I think I can remove package.json & mirror-for-offline folder too then?

BYK · 2017-09-16T17:54:00Z

src/cli/commands/add.js

+    const exact =
+      this.flags.exact ||
+      Boolean(this.config.getOption('save-exact')) ||
+      String(this.config.getOption('save-prefix')) === '';


Assigning this to a variable like configPrefix might be good since it is now used here and also below.

buildsize · 2017-09-16T18:44:16Z

This change will decrease the build size from 9.69 MB to 9.69 MB, a decrease of 1.75 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	834.67 KB	834.47 KB	-203 bytes (0%)
yarn-[version].js	3.69 MB	3.69 MB	-552 bytes (0%)
yarn-legacy-[version].js	3.74 MB	3.74 MB	-680 bytes (0%)
yarn-v[version].tar.gz	838.74 KB	838.55 KB	-201 bytes (0%)
yarn_[version]all.deb	634.81 KB	634.66 KB	-160 bytes (0%)

buildsize · 2017-09-16T18:44:16Z

This change will decrease the build size from 9.69 MB to 9.69 MB, a decrease of 1.75 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	834.67 KB	834.47 KB	-203 bytes (0%)
yarn-[version].js	3.69 MB	3.69 MB	-552 bytes (0%)
yarn-legacy-[version].js	3.74 MB	3.74 MB	-680 bytes (0%)
yarn-v[version].tar.gz	838.74 KB	838.55 KB	-201 bytes (0%)
yarn_[version]all.deb	634.81 KB	634.66 KB	-160 bytes (0%)

ahmedelgabri · 2017-09-16T21:36:46Z

@BYK can you have a look at travis, I think it's failing because of disk/memory issues or something.

all the errors are like this

Error: ENOSPC: no space left on device, mkdir <some path>

BYK · 2017-09-17T00:27:12Z

__tests__/fixtures/add/install-strict-all/.yarnrc

@@ -0,0 +1,2 @@
+yarn-offline-mirror "./mirror-for-offline"


Offline mirror is for not using the internet to install packages during test AFAIK.

BYK · 2017-09-17T00:28:48Z

@BYK can you have a look at travis, I think it's failing because of disk/memory issues or something.

This is probably due to the RAMDISK on OS X builds. If it fails again, I'll edit your patch and increase the size.

BYK · 2017-09-17T00:48:01Z

Merged! Thanks a lot for helping us with this ❤️

ahmedelgabri · 2017-09-17T08:15:33Z

Thanks @BYK 😊

@jambonrose

**Summary** Fixes yarnpkg#4343. Currently there is no way to remove the package prefix inside `.yarnrc` file, this PR add support for `save-exact` in `.yarnrc` as discussed in yarnpkg#4343. Full credit goes to @jambonrose ``` save-exact true ``` One small thing, should `yarn` be backwards compatible with the old behavior which is `save-prefix ''`? We can just add an extra check here for do this. What do you think @BYK? ```js } else if (exact || Boolean(this.config.getOption('save-exact')) || Boolean(this.config.getOption('save-prefix'))) { ``` **Test plan** New unit test.

See yarnpkg/yarn#4471

BYK requested changes Sep 15, 2017

View reviewed changes

ahmedelgabri force-pushed the add-save-exact-option branch 5 times, most recently from 9b3e035 to e0f3cab Compare September 15, 2017 18:23

ahmedelgabri force-pushed the add-save-exact-option branch from e0f3cab to ce63a90 Compare September 15, 2017 19:54

BYK requested changes Sep 16, 2017

View reviewed changes

BYK mentioned this pull request Sep 16, 2017

save-prefix false in v0.28.4 adds false keyword to the package version #4104

Closed

Add a new setting save-exact

54b7f63

ahmedelgabri force-pushed the add-save-exact-option branch from ce63a90 to 51f26e3 Compare September 16, 2017 18:25

add test for save-exact

8f39f6b

ahmedelgabri force-pushed the add-save-exact-option branch from 51f26e3 to 8f39f6b Compare September 16, 2017 18:29

BYK reviewed Sep 17, 2017

View reviewed changes

BYK approved these changes Sep 17, 2017

View reviewed changes

BYK merged commit 0e16ee9 into yarnpkg:master Sep 17, 2017

This was referenced Oct 1, 2017

Yarn ignores .npmrc settings file #2193

Closed

Respect save-exact option from .npmrc in project directory #1088

Closed

chabou added a commit to chabou/hyper that referenced this pull request Nov 5, 2017

Fix pining dependencies with .yarnrc

7131309

See yarnpkg/yarn#4471

chabou mentioned this pull request Nov 5, 2017

Fix pining dependencies with .yarnrc vercel/hyper#2430

Merged

chabou added a commit to chabou/hyper-pane that referenced this pull request Nov 5, 2017

Fix pining dependencies with .yarnrc

9552a9a

See yarnpkg/yarn#4471

timothyis pushed a commit to vercel/hyper that referenced this pull request Nov 5, 2017

Fix pining dependencies with .yarnrc (#2430)

6784d33

See yarnpkg/yarn#4471

edmorley mentioned this pull request Jan 24, 2018

Bug 1395356 - Use auth0 instead of login.taskcluster.net for SSO mozilla/treeherder#3144

Merged

rjp44 mentioned this pull request Jul 26, 2020

Auth error when scoped private package has unscoped name in package.json #8258

Open

mxdvl mentioned this pull request May 11, 2023

Install exact versions by default guardian/dotcom-rendering#7730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support save-exact true #4471

Support save-exact true #4471

ahmedelgabri commented Sep 15, 2017 •

edited by BYK

StefanoMagrassi commented Sep 15, 2017

BYK commented Sep 15, 2017

BYK commented Sep 15, 2017

BYK left a comment

BYK Sep 15, 2017

StefanoMagrassi commented Sep 15, 2017

ahmedelgabri commented Sep 15, 2017 •

edited

ahmedelgabri commented Sep 15, 2017

BYK left a comment

BYK Sep 16, 2017

ahmedelgabri Sep 16, 2017

ahmedelgabri Sep 16, 2017

BYK Sep 16, 2017

ahmedelgabri Sep 16, 2017

BYK Sep 16, 2017

buildsize bot commented Sep 16, 2017

buildsize bot commented Sep 16, 2017

ahmedelgabri commented Sep 16, 2017

BYK Sep 17, 2017

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

ahmedelgabri commented Sep 17, 2017

		@@ -0,0 +1 @@
		yarn-offline-mirror=./mirror-for-offline

Support save-exact true #4471

Support save-exact true #4471

Conversation

ahmedelgabri commented Sep 15, 2017 • edited by BYK

StefanoMagrassi commented Sep 15, 2017

BYK commented Sep 15, 2017

BYK commented Sep 15, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanoMagrassi commented Sep 15, 2017

ahmedelgabri commented Sep 15, 2017 • edited

ahmedelgabri commented Sep 15, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buildsize bot commented Sep 16, 2017

buildsize bot commented Sep 16, 2017

ahmedelgabri commented Sep 16, 2017

Choose a reason for hiding this comment

BYK commented Sep 17, 2017

BYK commented Sep 17, 2017

ahmedelgabri commented Sep 17, 2017

ahmedelgabri commented Sep 15, 2017 •

edited by BYK

ahmedelgabri commented Sep 15, 2017 •

edited