fix(@angular/cli): handle YARN_ environment variables during ng update and ng add

packages/angular/cli/utilities/package-metadata.ts

@@ -131,7 +131,7 @@ function readOptions(
     logger.info(`Locating potential ${baseFilename} files:`);
   }
 
-  let options: PackageManagerOptions = {};
+  let rcOptions: PackageManagerOptions = {};
   for (const location of [...defaultConfigLocations, ...projectConfigLocations]) {
     if (existsSync(location)) {
       if (showPotentials) {
@@ -143,25 +143,33 @@ function readOptions(
       // See: https://github.com/npm/npm-registry-fetch/blob/ebddbe78a5f67118c1f7af2e02c8a22bcaf9e850/index.js#L99-L126
       const rcConfig: PackageManagerOptions = yarn ? lockfile.parse(data) : ini.parse(data);
 
-      options = normalizeOptions(rcConfig, location);
+      rcOptions = normalizeOptions(rcConfig, location);
     }
   }
 
+  const envVariablesOptions: PackageManagerOptions = {};
   for (const [key, value] of Object.entries(process.env)) {
-    if (!value || !key.toLowerCase().startsWith('npm_config_')) {
+    if (!value) {
       continue;
     }
 
-    const normalizedName = key
-      .substr(11)
-      .replace(/(?!^)_/g, '-') // don't replace _ at the start of the key
-      .toLowerCase();
-    options[normalizedName] = value;
-  }
+    let normalizedName = key.toLowerCase();
+    if (normalizedName.startsWith('npm_config_')) {
+      normalizedName = normalizedName.substring(11);
+    } else if (yarn && normalizedName.startsWith('yarn_')) {
+      normalizedName = normalizedName.substring(5);
+    } else {
+      continue;
+    }
 
-  options = normalizeOptions(options);
+    normalizedName = normalizedName.replace(/(?!^)_/g, '-'); // don't replace _ at the start of the key.s
+    envVariablesOptions[normalizedName] = value;
+  }
 
-  return options;
+  return {
+    ...rcOptions,
+    ...normalizeOptions(envVariablesOptions),
+  };
 }
 
 function normalizeOptions(
@@ -302,7 +310,6 @@ export async function fetchPackageManifest(
   } = {},
 ): Promise<PackageManifest> {
   const { usingYarn = false, verbose = false, registry } = options;
-
   ensureNpmrc(logger, usingYarn, verbose);
 
   const response = await pacote.manifest(name, {
@@ -329,18 +336,7 @@ export function getNpmPackageJson(
   }
 
   const { usingYarn = false, verbose = false, registry } = options;
-
-  if (!npmrc) {
-    try {
-      npmrc = readOptions(logger, false, verbose);
-    } catch {}
-
-    if (usingYarn) {
-      try {
-        npmrc = { ...npmrc, ...readOptions(logger, true, verbose) };
-      } catch {}
-    }
-  }
+  ensureNpmrc(logger, usingYarn, verbose);
 
   const resultPromise: Promise<NpmRepositoryPackageJson> = pacote.packument(packageName, {
     fullMetadata: true,

tests/legacy-cli/e2e/tests/commands/add/npm-env-vars.ts

@@ -1,33 +1,26 @@
 import { expectFileNotToExist, expectFileToExist } from '../../../utils/fs';
-import { getActivePackageManager } from '../../../utils/packages';
 import { git, ng } from '../../../utils/process';
 import {
   createNpmConfigForAuthentication,
   setNpmEnvVarsForAuthentication,
 } from '../../../utils/registry';
 
 export default async function () {
-  const packageManager = getActivePackageManager();
+  // Yarn also supports NPM_CONFIG env variables.
+  // https://classic.yarnpkg.com/en/docs/envvars/
 
-  if (packageManager === 'npm') {
-    const originalEnvironment = { ...process.env };
-    try {
-      const command = ['add', '@angular/pwa', '--skip-confirmation'];
+  const command = ['add', '@angular/pwa', '--skip-confirmation'];
 
-      // Environment variables only
-      await expectFileNotToExist('src/manifest.webmanifest');
-      setNpmEnvVarsForAuthentication();
-      await ng(...command);
-      await expectFileToExist('src/manifest.webmanifest');
-      await git('clean', '-dxf');
+  // Environment variables only
+  await expectFileNotToExist('src/manifest.webmanifest');
+  setNpmEnvVarsForAuthentication();
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+  await git('clean', '-dxf');
 
-      // Mix of config file and env vars works
-      await expectFileNotToExist('src/manifest.webmanifest');
-      await createNpmConfigForAuthentication(false, true);
-      await ng(...command);
-      await expectFileToExist('src/manifest.webmanifest');
-    } finally {
-      process.env = originalEnvironment;
-    }
-  }
+  // Mix of config file and env vars works
+  await expectFileNotToExist('src/manifest.webmanifest');
+  await createNpmConfigForAuthentication(false, true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
 }

tests/legacy-cli/e2e/tests/commands/add/registry-option.ts

@@ -10,21 +10,12 @@ export default async function () {
   await writeMultipleFiles({
     '.npmrc': 'registry=http://127.0.0.1:9999',
   });
+
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
+  delete process.env['NPM_CONFIG_REGISTRY'];
 
-  try {
-    await expectToFail(() => ng('add', '@angular/pwa', '--skip-confirmation'));
+  await expectToFail(() => ng('add', '@angular/pwa', '--skip-confirmation'));
 
-    await ng('add', `--registry=${testRegistry}`, '@angular/pwa', '--skip-confirmation');
-    await expectFileToExist('src/manifest.webmanifest');
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await ng('add', `--registry=${testRegistry}`, '@angular/pwa', '--skip-confirmation');
+  await expectFileToExist('src/manifest.webmanifest');
 }

tests/legacy-cli/e2e/tests/commands/add/secure-registry.ts

@@ -5,38 +5,28 @@ import { expectToFail } from '../../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
+  delete process.env['NPM_CONFIG_REGISTRY'];
 
-  try {
-    const command = ['add', '@angular/pwa', '--skip-confirmation'];
-    await expectFileNotToExist('src/manifest.webmanifest');
+  const command = ['add', '@angular/pwa', '--skip-confirmation'];
+  await expectFileNotToExist('src/manifest.webmanifest');
 
-    // Works with unscoped registry authentication details
-    await createNpmConfigForAuthentication(false);
-    await ng(...command);
-    await expectFileToExist('src/manifest.webmanifest');
-    await git('clean', '-dxf');
+  // Works with unscoped registry authentication details
+  await createNpmConfigForAuthentication(false);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+  await git('clean', '-dxf');
 
-    // Works with scoped registry authentication details
-    await expectFileNotToExist('src/manifest.webmanifest');
+  // Works with scoped registry authentication details
+  await expectFileNotToExist('src/manifest.webmanifest');
 
-    await createNpmConfigForAuthentication(true);
-    await ng(...command);
-    await expectFileToExist('src/manifest.webmanifest');
+  await createNpmConfigForAuthentication(true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
 
-    // Invalid authentication token
-    await createNpmConfigForAuthentication(false, true);
-    await expectToFail(() => ng(...command));
+  // Invalid authentication token
+  await createNpmConfigForAuthentication(false, true);
+  await expectToFail(() => ng(...command));
 
-    await createNpmConfigForAuthentication(true, true);
-    await expectToFail(() => ng(...command));
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await createNpmConfigForAuthentication(true, true);
+  await expectToFail(() => ng(...command));
 }

tests/legacy-cli/e2e/tests/commands/add/yarn-env-vars.ts

@@ -0,0 +1,29 @@
+import { expectFileNotToExist, expectFileToExist } from '../../../utils/fs';
+import { getActivePackageManager } from '../../../utils/packages';
+import { git, ng } from '../../../utils/process';
+import {
+  createNpmConfigForAuthentication,
+  setNpmEnvVarsForAuthentication,
+} from '../../../utils/registry';
+
+export default async function () {
+  // Yarn specific test that tests YARN_ env variables.
+  // https://classic.yarnpkg.com/en/docs/envvars/
+  if (getActivePackageManager() !== 'yarn') {
+    return;
+  }
+  const command = ['add', '@angular/pwa', '--skip-confirmation'];
+
+  // Environment variables only
+  await expectFileNotToExist('src/manifest.webmanifest');
+  setNpmEnvVarsForAuthentication(false, true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+  await git('clean', '-dxf');
+
+  // Mix of config file and env vars works
+  await expectFileNotToExist('src/manifest.webmanifest');
+  await createNpmConfigForAuthentication(false, true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+}

tests/legacy-cli/e2e/tests/update/update-secure-registry.ts

@@ -4,37 +4,26 @@ import { expectToFail } from '../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
-
+  delete process.env['NPM_CONFIG_REGISTRY'];
   const worksMessage = 'We analyzed your package.json';
 
-  try {
-    // Valid authentication token
-    await createNpmConfigForAuthentication(false);
-    const { stdout: stdout1 } = await ng('update');
-    if (!stdout1.includes(worksMessage)) {
-      throw new Error(`Expected stdout to contain "${worksMessage}"`);
-    }
+  // Valid authentication token
+  await createNpmConfigForAuthentication(false);
+  const { stdout: stdout1 } = await ng('update');
+  if (!stdout1.includes(worksMessage)) {
+    throw new Error(`Expected stdout to contain "${worksMessage}"`);
+  }
 
-    await createNpmConfigForAuthentication(true);
-    const { stdout: stdout2 } = await ng('update');
-    if (!stdout2.includes(worksMessage)) {
-      throw new Error(`Expected stdout to contain "${worksMessage}"`);
-    }
+  await createNpmConfigForAuthentication(true);
+  const { stdout: stdout2 } = await ng('update');
+  if (!stdout2.includes(worksMessage)) {
+    throw new Error(`Expected stdout to contain "${worksMessage}"`);
+  }
 
-    // Invalid authentication token
-    await createNpmConfigForAuthentication(false, true);
-    await expectToFail(() => ng('update'));
+  // Invalid authentication token
+  await createNpmConfigForAuthentication(false, true);
+  await expectToFail(() => ng('update'));
 
-    await createNpmConfigForAuthentication(true, true);
-    await expectToFail(() => ng('update'));
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await createNpmConfigForAuthentication(true, true);
+  await expectToFail(() => ng('update'));
 }

tests/legacy-cli/e2e/utils/packages.ts

@@ -53,8 +53,7 @@ export async function setRegistry(useTestRegistry: boolean): Promise<void> {
     // Safe to set a user configuration on CI
     await npm('config', 'set', 'registry', url);
   } else {
-    // Yarn does not use the environment variable so an .npmrc file is also required
-    await writeFile('.npmrc', `registry=${url}`);
+    // Yarn supports both `NPM_CONFIG_REGISTRY` and `YARN_REGISTRY`.
     process.env['NPM_CONFIG_REGISTRY'] = url;
   }
 }

tests/legacy-cli/e2e/utils/registry.ts

@@ -61,10 +61,18 @@ export function createNpmConfigForAuthentication(
 export function setNpmEnvVarsForAuthentication(
   /** When true, an incorrect token is used. Use this to validate authentication failures. */
   invalidToken = false,
+  /** When true, `YARN_REGISTRY` is used instead of `NPM_CONFIG_REGISTRY`. */
+  useYarnEnvVariable = false,
 ): void {
-  const token = invalidToken ? `invalid=` : VALID_TOKEN;
-  const registry = SECURE_REGISTRY;
+  delete process.env['YARN_REGISTRY'];
+  delete process.env['NPM_CONFIG_REGISTRY'];
+
+  const registryKey = useYarnEnvVariable ? 'YARN_REGISTRY' : 'NPM_CONFIG_REGISTRY';
+  process.env[registryKey] = `http:${SECURE_REGISTRY}`;
+
+  process.env['NPM_CONFIG__AUTH'] = invalidToken ? `invalid=` : VALID_TOKEN;
 
-  process.env['NPM_CONFIG_REGISTRY'] = `http:${registry}`;
-  process.env['NPM_CONFIG__AUTH'] = token;
+  // Needed for verdaccio when used with yarn
+  // https://verdaccio.org/docs/en/cli-registry#yarn
+  process.env['NPM_CONFIG_ALWAYS_AUTH'] = 'true';
 }

tests/legacy-cli/e2e_runner.ts

@@ -141,6 +141,10 @@ testsToRun
       const start = +new Date();
 
       const module = require(absoluteName);
+      const originalEnvVariables = {
+        ...process.env,
+      };
+
       const fn: (skipClean?: () => void) => Promise<void> | void =
         typeof module == 'function'
           ? module
@@ -169,16 +173,19 @@ testsToRun
         .then(() => {
           // If we're not in a setup, change the directory back to where it was before the test.
           // This allows tests to chdir without worrying about keeping the original directory.
-          if (allSetups.indexOf(relativeName) == -1 && previousDir) {
+          if (!allSetups.includes(relativeName) && previousDir) {
             process.chdir(previousDir);
+
+            // Restore env variables before each test.
+            console.log('  Restoring original environment variables...');
+            process.env = originalEnvVariables;
           }
         })
         .then(() => {
           // Only clean after a real test, not a setup step. Also skip cleaning if the test
           // requested an exception.
-          if (allSetups.indexOf(relativeName) == -1 && clean) {
+          if (!allSetups.includes(relativeName) && clean) {
             logStack.push(new logging.NullLogger());
-
             return gitClean().then(
               () => logStack.pop(),
               (err) => {