terrible fix

Author: ecraig12345

packages/beachball/src/__e2e__/syncE2E.test.ts

@@ -31,7 +31,7 @@ describe('sync command (e2e)', () => {
   const publishOptions: Parameters<typeof packagePublish>[1] = {
     registry: 'fake',
     retries: 3,
-    path: undefined,
+    path: '',
     npmReadConcurrency: 2,
   };
 

packages/beachball/src/__fixtures__/mockNpm.test.ts

@@ -134,50 +134,48 @@ describe('_mockNpmShow', () => {
 
   it("errors if package doesn't exist", async () => {
     const emptyData = _makeRegistryData({});
-    const result = await _mockNpmShow(emptyData, ['foo'], { cwd: undefined });
+    const result = await _mockNpmShow(emptyData, ['foo'], { cwd: '' });
     expect(result).toEqual(getErrorResult('[fake] code E404 - foo - not found'));
   });
 
   it('returns requested version plus dist-tags and version list', async () => {
-    const result = await _mockNpmShow(data, ['foo@1.0.0'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['foo@1.0.0'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0' }));
   });
 
   it('returns requested version of scoped package', async () => {
-    const result = await _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0' }));
   });
 
   it('returns requested tag', async () => {
-    const result = await _mockNpmShow(data, ['foo@beta'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['foo@beta'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0-beta' }));
   });
 
   it('returns requested tag of scoped package', async () => {
-    const result = await _mockNpmShow(data, ['@foo/bar@beta'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['@foo/bar@beta'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0-beta' }));
   });
 
   it('returns latest version if no version requested', async () => {
-    const result = await _mockNpmShow(data, ['foo'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['foo'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.1' }));
   });
 
   it('returns latest version of scoped package if no version requested', async () => {
-    const result = await _mockNpmShow(data, ['@foo/bar'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['@foo/bar'], { cwd: '' });
     expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.1' }));
   });
 
   it("errors if requested version doesn't exist", async () => {
-    const result = await _mockNpmShow(data, ['foo@2.0.0'], { cwd: undefined });
+    const result = await _mockNpmShow(data, ['foo@2.0.0'], { cwd: '' });
     expect(result).toEqual(getErrorResult('[fake] code E404 - foo@2.0.0 - not found'));
   });
 
   // support for this could be added later
   it('currently throws if requested version is a range', async () => {
-    await expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: undefined })).rejects.toThrow(
-      /not currently supported/
-    );
+    await expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: '' })).rejects.toThrow(/not currently supported/);
   });
 });
 
@@ -209,9 +207,7 @@ describe('_mockNpmPublish', () => {
   });
 
   it('throws if cwd is not specified', async () => {
-    await expect(() => _mockNpmPublish({}, [], { cwd: undefined })).rejects.toThrow(
-      'cwd is required for mock npm publish'
-    );
+    await expect(() => _mockNpmPublish({}, [], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
   });
 
   it('errors if reading package.json fails', async () => {
@@ -308,7 +304,7 @@ describe('_mockNpmPack', () => {
   });
 
   it('throws if cwd is not specified', async () => {
-    await expect(() => _mockNpmPack({}, [], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack');
+    await expect(() => _mockNpmPack({}, [], { cwd: '' })).rejects.toThrow('cwd is required for mock npm pack');
   });
 
   it('errors if reading package.json fails', async () => {
@@ -433,7 +429,7 @@ describe('mockNpm', () => {
         versions: ['1.0.0'],
         'dist-tags': { latest: '1.0.0' },
       });
-      expect(await npm(['show', 'foo'], { cwd: undefined })).toMatchObject({
+      expect(await npm(['show', 'foo'], { cwd: '' })).toMatchObject({
         success: true,
         stdout: JSON.stringify({
           name: 'foo',
@@ -485,19 +481,19 @@ describe('mockNpm', () => {
     });
 
     it('throws if required cwd is missing', async () => {
-      await expect(() => npm(['publish'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm publish');
-      await expect(() => npm(['pack'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack');
+      await expect(() => npm(['publish'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
+      await expect(() => npm(['pack'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm pack');
     });
 
     it('throws on unsupported command', async () => {
-      await expect(() => npm(['foo'], { cwd: undefined })).rejects.toThrow('Command not supported by mock npm: foo');
+      await expect(() => npm(['foo'], { cwd: '' })).rejects.toThrow('Command not supported by mock npm: foo');
       expect(npmMock.mock).toHaveBeenCalledTimes(1);
-      expect(npmMock.mock).toHaveBeenCalledWith(['foo'], expect.objectContaining({ cwd: undefined }));
+      expect(npmMock.mock).toHaveBeenCalledWith(['foo'], expect.objectContaining({ cwd: '' }));
     });
 
     it('TEMP mocks npm show command', async () => {
       npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
-      const result = await npm(['show', 'foo'], { cwd: undefined });
+      const result = await npm(['show', 'foo'], { cwd: '' });
       expect(result).toMatchObject({
         success: true,
         stdout: expect.stringContaining('"name":"foo"'),
@@ -511,24 +507,24 @@ describe('mockNpm', () => {
     it('respects mocked command override', async () => {
       const mockPublish = jest.fn(() => Promise.resolve(fakePublishResult as unknown as MockNpmResult));
       npmMock.setCommandOverride('publish', mockPublish);
-      const result = await npm(['publish', 'foo'], { cwd: undefined });
+      const result = await npm(['publish', 'foo'], { cwd: '' });
       expect(result).toEqual(fakePublishResult);
-      expect(mockPublish).toHaveBeenCalledWith(expect.any(Object), ['foo'], { cwd: undefined });
+      expect(mockPublish).toHaveBeenCalledWith(expect.any(Object), ['foo'], { cwd: '' });
     });
 
     it("respects extra mocked command that's not normally supported", async () => {
       const mockFoo = jest.fn(() => Promise.resolve('hi' as unknown as MockNpmResult));
       npmMock.setCommandOverride('foo', mockFoo);
-      const result = await npm(['foo'], { cwd: undefined });
+      const result = await npm(['foo'], { cwd: '' });
       expect(result).toEqual('hi');
-      expect(mockFoo).toHaveBeenCalledWith(expect.any(Object), [], { cwd: undefined });
+      expect(mockFoo).toHaveBeenCalledWith(expect.any(Object), [], { cwd: '' });
     });
 
     it('resets commands after each test', async () => {
       // extra command is gone
-      await expect(() => npm(['foo'], { cwd: undefined })).rejects.toThrow('Command not supported by mock npm: foo');
+      await expect(() => npm(['foo'], { cwd: '' })).rejects.toThrow('Command not supported by mock npm: foo');
       // publish mock is gone
-      await expect(() => npm(['publish'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm publish');
+      await expect(() => npm(['publish'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
     });
   });
 });

packages/beachball/src/__functional__/packageManager/getNpmPackageInfo.test.ts

@@ -29,13 +29,13 @@ describe('getNpmPackageInfo', () => {
     { desc: 'scoped', name: '@lage-run/cli', knownVersion: '0.33.0' },
   ])('gets info for $desc package from public npm registry', async ({ name, knownVersion }) => {
     const timeout = 10000;
-    const result = await getNpmPackageInfo(name, { registry, timeout, path: undefined });
+    const result = await getNpmPackageInfo(name, { registry, timeout, path: '' });
 
     expect(npmSpy).toHaveBeenCalledTimes(1);
     // Verify args format
     expect(npmSpy).toHaveBeenCalledWith(
       ['show', '--registry', registry, '--json', name, ..._npmShowProperties],
-      expect.objectContaining({ timeout, cwd: undefined })
+      expect.objectContaining({ timeout, cwd: '' })
     );
     // Verify output format (there's no toHaveResolvedWith matcher, so await the return value)
     expect(await npmSpy.mock.results[0].value).toMatchObject({
@@ -91,7 +91,7 @@ describe('getNpmPackageInfo', () => {
 
   it('passes auth args', async () => {
     // Don't care about the result in this case
-    await getNpmPackageInfo(shouldNotExist, { registry, token: 'fake', path: undefined });
+    await getNpmPackageInfo(shouldNotExist, { registry, token: 'fake', path: '' });
 
     expect(npmSpy).toHaveBeenCalledTimes(1);
     expect(npmSpy).toHaveBeenCalledWith(

packages/beachball/src/__functional__/packageManager/packagePublish.test.ts

@@ -38,9 +38,10 @@ describe('packagePublish', () => {
 
   const logs = initMockLogs();
 
-  const defaultOptions: Omit<PackagePublishOptions, 'path' | 'registry'> = {
+  const defaultOptions: Omit<PackagePublishOptions, 'path'> = {
     npmReadConcurrency: 2,
     retries: 3,
+    registry: 'http://fake-registry', // overwritten for real tests
   };
 
   function getTestPackageInfo(): PackageInfo {
@@ -184,10 +185,11 @@ describe('packagePublish', () => {
       expect(logs2ndTry).toMatch(`${testSpec} already exists in the registry`);
     });
 
-    // TODO: remove condition once node version is upgraded (this test doesn't work with npm 6 because
-    // that version seems to allow truly anonymous publishing with verdaccio)
     it('handles auth error and does not retry', async () => {
+      // TODO: remove condition once node version is upgraded (this test doesn't work with npm 6 because
+      // that version seems to allow truly anonymous publishing with verdaccio)
       if (npmVersion.startsWith('6.')) {
+        console.warn('Skipping auth error test on npm 6');
         return;
       }
       await registry.logout();
@@ -212,7 +214,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(testPackageInfo, {
         ...defaultOptions,
-        registry: 'http://fake-registry',
         path: tempRoot,
         token: 'fake-token',
       });
@@ -244,7 +245,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(testPackageInfo, {
         ...defaultOptions,
-        registry: 'fake',
         path: tempRoot,
       });
       expect(publishResult).toEqual(successResult);
@@ -264,7 +264,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(getTestPackageInfo(), {
         ...defaultOptions,
-        registry: 'fake',
         path: tempRoot,
       });
       expect(publishResult).toEqual(failedResult);
@@ -284,7 +283,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(testPackageInfo, {
         ...defaultOptions,
-        registry: 'fake',
         path: tempRoot,
       });
       expect(publishResult).toEqual(failedResult);
@@ -303,7 +301,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(testPackageInfo, {
         ...defaultOptions,
-        registry: 'fake',
         path: tempRoot,
       });
       expect(publishResult).toEqual(failedResult);
@@ -318,7 +315,6 @@ describe('packagePublish', () => {
 
       const publishResult = await packagePublish(testPackageInfo, {
         ...defaultOptions,
-        registry: 'fake',
         path: tempRoot,
       });
       expect(publishResult).toEqual(failedResult);

packages/beachball/src/__tests__/packageManager/listPackageVersions.test.ts

@@ -26,7 +26,7 @@ describe('list npm versions', () => {
     const npmOptions: NpmOptions = {
       registry,
       timeout,
-      path: undefined,
+      path: '',
       npmReadConcurrency: 2,
     };
 

packages/beachball/src/__tests__/publish/getNewPackages.test.ts

@@ -12,7 +12,7 @@ describe('getNewPackages', () => {
   const logs = initMockLogs();
   /** Mock the `npm show` command for `npmAsync` calls. This also handles cleanup after each test. */
   const npmMock = initNpmMock();
-  const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: undefined, registry: 'https://fake' };
+  const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: '', registry: 'https://fake' };
 
   it('returns empty if no packages exist', async () => {
     const newPackages = await getNewPackages({ modifiedPackages: new Set(), packageInfos: {} }, npmOptions);

packages/beachball/src/__tests__/publish/validatePackageVersions.test.ts

@@ -12,7 +12,7 @@ describe('validatePackageVersions', () => {
   const logs = initMockLogs();
   /** Mock the `npm show` command. This also handles cleanup after each test. */
   const npmMock = initNpmMock();
-  const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: undefined, registry: 'https://fake' };
+  const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: '', registry: 'https://fake' };
 
   it('succeeds with nothing to validate', async () => {
     expect(await validatePackageVersions([], {}, npmOptions)).toBe(true);

packages/beachball/src/packageManager/packageManager.ts

@@ -1,7 +1,8 @@
 import execa from 'execa';
+import path from 'path';
 
 export type PackageManagerResult = execa.ExecaReturnValue & { success: boolean };
-export type PackageManagerOptions = execa.Options & { cwd: string | undefined };
+export type PackageManagerOptions = execa.Options & { cwd: string };
 
 /**
  * Run a package manager command. Returns the error result instead of throwing on failure.
@@ -15,12 +16,33 @@ export async function packageManager(
   args: string[],
   options: PackageManagerOptions
 ): Promise<PackageManagerResult> {
+  let pathEnv = options.env?.PATH || process.env.PATH;
+  if (manager === 'npm' && pathEnv) {
+    // Workaround for an issue on certain platforms/shells(?) if the parent command was run VIA yarn:
+    // The auth environment variable (e.g. `npm_config_//someRegistry/:_authToken`) was not being
+    // passed through to the child process. This might be because:
+    // - Special characters such as / and : aren't valid in env var names for certain shells/platforms
+    // - On every `yarn run ...` command, yarn makes temp directories like /<temp>/yarn--1776822418161-0.7992675923334178
+    //   with aliases for `node` and `yarn`. On Linux (and Mac), the `node` alias looks something like:
+    //     #!/bin/sh
+    //     exec "/path/to/node" "$@"
+    // - Best guess: invalid environment variable names are dropped by this extra `exec` step??
+    //   (This consistently reproed on Ubuntu+bash, but not Mac+zsh or bash. The clue was that the
+    //   tests passed even on Linux when run via debugTests.js, but failed when run via yarn test.)
+    //
+    // Removing the yarn-- segment from the PATH seems to consistently fix this issue.
+    pathEnv = pathEnv
+      .split(path.delimiter)
+      .filter(p => !path.basename(p).startsWith('yarn--'))
+      .join(path.delimiter);
+  }
+
   try {
     const result = await execa(manager, args, {
       ...options,
+      env: { ...options.env, PATH: pathEnv },
       // This is required for Windows due to https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
-      // but only provide it on Windows because it breaks oddly-named environment variables
-      // such as npm_config_//someRegistry/:_authToken on Linux...
+      // but only provide it on Windows because it breaks the auth env var on Linux...
       ...(process.platform === 'win32' && { shell: true }),
     });
     return {

packages/beachball/src/packageManager/packagePublish.ts

@@ -38,7 +38,9 @@ export async function packagePublish(
       cwd: packageRoot,
       timeout: options.timeout,
       all: true,
-      env: { ...process.env, ...authEnv },
+      preferLocal: false,
+      extendEnv: true,
+      env: authEnv,
     });
 
     if (result.success) {

packages/beachball/src/types/NpmOptions.ts

@@ -1,5 +1,4 @@
 import type { BeachballOptions } from './BeachballOptions';
 
-export type NpmOptions = Required<Pick<BeachballOptions, 'registry' | 'npmReadConcurrency'>> & {
-  path: string | undefined;
-} & Partial<Pick<BeachballOptions, 'token' | 'authType' | 'access' | 'timeout' | 'verbose' | 'tag' | 'defaultNpmTag'>>;
+export type NpmOptions = Required<Pick<BeachballOptions, 'registry' | 'npmReadConcurrency' | 'path'>> &
+  Partial<Pick<BeachballOptions, 'token' | 'authType' | 'access' | 'timeout' | 'verbose' | 'tag' | 'defaultNpmTag'>>;