Allow ${HOME} in .npmrc to work in Windows #4693

joshkel · 2017-10-12T02:27:58Z

Windows doesn't set the HOME environment variable by default, but NPM has logic to set process.env.HOME based on the current OS home directory. See here.

Yarn doesn't do this, so an .npmrc config such as prefix = ${HOME}/.npm-packages that work for NPM will cause Yarn to refuse to run; it instead exits with the following error:

yarn install v1.2.0
error An unexpected error occurred: "Failed to replace env in config: ${HOME}".
info If you think this is a bug, please open a bug report with the information provided in "C:\\src\\yarn\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

This commit updates Yarn's behavior to be closer to NPM's.

Unlike NPM, this commit only sets HOME if it's not already set, to avoid potentially incompatible changes with existing Yarn users.

Windows doesn't set the HOME environment variable by default, but NPM has logic to set process.env.HOME based on the current OS home directory. See https://github.com/npm/npm/blob/fb28e5868a9dbbe21a15f23fe8cf8b3703e8adf2/lib/config/defaults.js#L81 Yarn doesn't do this, so configs such as `prefix = ${HOME}/.npm-packages` that work for NPM will cause Yarn to refuse to run. This commit updates Yarn's behavior to be closer to NPM's. Unlike NPM, this commit only sets HOME if it's not already set, to avoid potentially incompatible changes with existing Yarn users.

buildsize · 2017-10-12T02:38:24Z

This change will decrease the build size from 9.94 MB to 9.91 MB, a decrease of 22.5 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	859.32 KB	857.36 KB	-1.96 KB (0%)
yarn-[version].js	3.78 MB	3.77 MB	-8.62 KB (0%)
yarn-legacy-[version].js	3.83 MB	3.82 MB	-8.67 KB (0%)
yarn-v[version].tar.gz	864.97 KB	863.21 KB	-1.76 KB (0%)
yarn_[version]all.deb	653.99 KB	652.49 KB	-1.5 KB (0%)

arcanis · 2017-10-12T09:57:06Z

src/rc.js

+export function prepareEnv() {
+  // Allow ${HOME} in, e.g., .npmrc to work even on Windows.
+  if (rcUtil.home && !process.env.HOME) {
+    process.env.HOME = rcUtil.home;


I don't like this. We shouldn't overwrite environment variables and, if we really must, we shouldn't do it in this file.

arcanis · 2017-10-12T09:57:28Z

Why don't you just use ~?

BYK

Thanks a lot for the PR!

I've left a comment in the code. Also we need tests to ensure this doesn't break in the future and to make sure the code actually works :)

BYK · 2017-10-12T09:57:53Z

src/cli/index.js

@@ -537,6 +537,7 @@ export function main({
 }

 async function start(): Promise<void> {
+  prepareEnv();


I think this should be done in getRcConfigForCwd instead of changing the whole process.env. I don't think it is a good idea to patch/mutate process.env

Normally, I'd agree. In this case, though, I figured that setting HOME only if it's not already set should be safe (the fact that NPM does it could be evidence for its safety?), and, since Yarn is promoted as a drop-in replacement for NPM, I figured there's benefit to doing what NPM does. (My thinking here is probably colored by my running into #1538.)

I'm happy to go either way, though, so I can change the PR this evening to use getRcConfigForCwd.

Thanks for the feedback!

Upon further investigation, I don't think that getRcConfigForCwd is correct. That finds config files but doesn't process them. The environment variable changes instead need to be made within NpmRegistry. I just pushed an updated PR with these changes.

joshkel · 2017-10-12T16:47:35Z

Why don't you just use ~?

Because I didn't realize I could. 🙂 (I think I copied that config from an online source when I was first getting familiar with NPM and never thought about it again.) Thanks for the suggestion.

This reverts commit 436422d.

Windows doesn't set the HOME environment variable by default, but NPM has logic to set process.env.HOME based on the current OS home directory. See https://github.com/npm/npm/blob/fb28e5868a9dbbe21a15f23fe8cf8b3703e8adf2/lib/config/defaults.js#L81 Yarn doesn't do this, so configs such as `prefix = ${HOME}/.npm-packages` that work for NPM will cause Yarn to refuse to run. This commit updates Yarn's behavior to be closer to NPM's, by using a custom/modified environment when processing NPM configurations. Add a Flow type `Env` to represent a set of environment variables.

(A separate `describe` block seems like overkill.)

BYK · 2017-10-17T10:42:52Z

@joshkel sorry for the wait didn't realize you've updated the PR. Will review it shortly.

BYK

I think this new version looks pretty great, thanks a lot!

BYK · 2017-10-17T10:43:44Z

src/util/env-replace.js

@@ -1,7 +1,9 @@
 /* @flow */
 const ENV_EXPR = /(\\*)\$\{([^}]+)\}/g;

-export default function envReplace(value: string, env: {[key: string]: ?string} = process.env): string {
+export type Env = {[key: string]: ?string};


BYK · 2017-10-17T10:44:42Z

src/registries/npm-registry.js

@@ -206,11 +207,20 @@ export default class NpmRegistry extends Registry {
    return actuals;
  }

+  static getConfigEnv(env: Env = process.env): Env {
+    // To match NPM's behavior, HOME is always the user's home directory.
+    const overrideEnv = {


protip:

return { ...env, HOME: home, };

BYK · 2017-10-17T10:46:30Z

I'll let @arcanis do the final review and merge if there are no blockers.

* Allow ${HOME} in .npmrc to work in Windows Windows doesn't set the HOME environment variable by default, but NPM has logic to set process.env.HOME based on the current OS home directory. See https://github.com/npm/npm/blob/fb28e5868a9dbbe21a15f23fe8cf8b3703e8adf2/lib/config/defaults.js#L81 Yarn doesn't do this, so configs such as `prefix = ${HOME}/.npm-packages` that work for NPM will cause Yarn to refuse to run. This commit updates Yarn's behavior to be closer to NPM's. Unlike NPM, this commit only sets HOME if it's not already set, to avoid potentially incompatible changes with existing Yarn users. * Revert "Allow ${HOME} in .npmrc to work in Windows" This reverts commit 436422d. * Allow ${HOME} in .npmrc to work in Windows Windows doesn't set the HOME environment variable by default, but NPM has logic to set process.env.HOME based on the current OS home directory. See https://github.com/npm/npm/blob/fb28e5868a9dbbe21a15f23fe8cf8b3703e8adf2/lib/config/defaults.js#L81 Yarn doesn't do this, so configs such as `prefix = ${HOME}/.npm-packages` that work for NPM will cause Yarn to refuse to run. This commit updates Yarn's behavior to be closer to NPM's, by using a custom/modified environment when processing NPM configurations. Add a Flow type `Env` to represent a set of environment variables. * Ensure environment is restored after test (A separate `describe` block seems like overkill.)

arcanis requested changes Oct 12, 2017

View reviewed changes

BYK requested changes Oct 12, 2017

View reviewed changes

joshkel added 3 commits October 12, 2017 22:01

Revert "Allow ${HOME} in .npmrc to work in Windows"

20f5b25

This reverts commit 436422d.

Ensure environment is restored after test

4939423

(A separate `describe` block seems like overkill.)

BYK approved these changes Oct 17, 2017

View reviewed changes

arcanis approved these changes Oct 24, 2017

View reviewed changes

arcanis merged commit 1ccb710 into yarnpkg:master Oct 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ${HOME} in .npmrc to work in Windows #4693

Allow ${HOME} in .npmrc to work in Windows #4693

joshkel commented Oct 12, 2017

buildsize bot commented Oct 12, 2017 •

edited

Loading

arcanis Oct 12, 2017

arcanis commented Oct 12, 2017

BYK left a comment

BYK Oct 12, 2017

joshkel Oct 12, 2017

joshkel Oct 13, 2017

joshkel commented Oct 12, 2017

BYK commented Oct 17, 2017

BYK left a comment

BYK Oct 17, 2017

BYK Oct 17, 2017

BYK commented Oct 17, 2017

Allow ${HOME} in .npmrc to work in Windows #4693

Allow ${HOME} in .npmrc to work in Windows #4693

Conversation

joshkel commented Oct 12, 2017

buildsize bot commented Oct 12, 2017 • edited Loading

arcanis Oct 12, 2017

Choose a reason for hiding this comment

arcanis commented Oct 12, 2017

BYK left a comment

Choose a reason for hiding this comment

BYK Oct 12, 2017

Choose a reason for hiding this comment

joshkel Oct 12, 2017

Choose a reason for hiding this comment

joshkel Oct 13, 2017

Choose a reason for hiding this comment

joshkel commented Oct 12, 2017

BYK commented Oct 17, 2017

BYK left a comment

Choose a reason for hiding this comment

BYK Oct 17, 2017

Choose a reason for hiding this comment

BYK Oct 17, 2017

Choose a reason for hiding this comment

BYK commented Oct 17, 2017

buildsize bot commented Oct 12, 2017 •

edited

Loading