fix!: call executePackageManagerRequest directly (#430)

* fix!: call `executePackageManagerRequest` directly * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> * test: update assertions --------- Co-authored-by: Antoine du Hamel <[email protected]>
nodejs · Apr 20, 2024 · 0f9b748 · 0f9b748
1 parent df1b773
commit 0f9b748
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 88 deletions.
diff --git a/sources/main.ts b/sources/main.ts
@@ -1,23 +1,23 @@
-import {BaseContext, Builtins, Cli, Command, Option} from 'clipanion';
-
-import {version as corepackVersion}                  from '../package.json';
-
-import {Engine, PackageManagerRequest}               from './Engine';
-import {CacheCommand}                                from './commands/Cache';
-import {DisableCommand}                              from './commands/Disable';
-import {EnableCommand}                               from './commands/Enable';
-import {InstallGlobalCommand}                        from './commands/InstallGlobal';
-import {InstallLocalCommand}                         from './commands/InstallLocal';
-import {PackCommand}                                 from './commands/Pack';
-import {UpCommand}                                   from './commands/Up';
-import {UseCommand}                                  from './commands/Use';
-import {HydrateCommand}                              from './commands/deprecated/Hydrate';
-import {PrepareCommand}                              from './commands/deprecated/Prepare';
+import {BaseContext, Builtins, Cli}    from 'clipanion';
+
+import {version as corepackVersion}    from '../package.json';
+
+import {Engine, PackageManagerRequest} from './Engine';
+import {CacheCommand}                  from './commands/Cache';
+import {DisableCommand}                from './commands/Disable';
+import {EnableCommand}                 from './commands/Enable';
+import {InstallGlobalCommand}          from './commands/InstallGlobal';
+import {InstallLocalCommand}           from './commands/InstallLocal';
+import {PackCommand}                   from './commands/Pack';
+import {UpCommand}                     from './commands/Up';
+import {UseCommand}                    from './commands/Use';
+import {HydrateCommand}                from './commands/deprecated/Hydrate';
+import {PrepareCommand}                from './commands/deprecated/Prepare';
 
 export type CustomContext = {cwd: string, engine: Engine};
 export type Context = BaseContext & CustomContext;
 
-function getPackageManagerRequestFromCli(parameter: string | undefined, context: CustomContext & Partial<Context>): PackageManagerRequest | null {
+function getPackageManagerRequestFromCli(parameter: string | undefined, engine: Engine): PackageManagerRequest | null {
   if (!parameter)
     return null;
 
@@ -26,7 +26,7 @@ function getPackageManagerRequestFromCli(parameter: string | undefined, context:
     return null;
 
   const [, binaryName, binaryVersion] = match;
-  const packageManager = context.engine.getPackageManagerFor(binaryName)!;
+  const packageManager = engine.getPackageManagerFor(binaryName)!;
 
   if (packageManager == null && binaryVersion == null) return null;
 
@@ -38,17 +38,10 @@ function getPackageManagerRequestFromCli(parameter: string | undefined, context:
 }
 
 export async function runMain(argv: Array<string>) {
-  // Because we load the binaries in the same process, we don't support custom contexts.
-  const context = {
-    ...Cli.defaultContext,
-    cwd: process.cwd(),
-    engine: new Engine(),
-  };
+  const engine = new Engine();
 
   const [firstArg, ...restArgs] = argv;
-  const request = getPackageManagerRequestFromCli(firstArg, context);
-
-  let code: number;
+  const request = getPackageManagerRequestFromCli(firstArg, engine);
 
   if (!request) {
     // If the first argument doesn't match any supported package manager, we fallback to the standard Corepack CLI
@@ -74,29 +67,21 @@ export async function runMain(argv: Array<string>) {
     cli.register(HydrateCommand);
     cli.register(PrepareCommand);
 
-    code = await cli.run(argv, context);
-  } else {
-    // Otherwise, we create a single-command CLI to run the specified package manager (we still use Clipanion in order to pretty-print usage errors).
-    const cli = new Cli({
-      binaryLabel: `'${request.binaryName}', via Corepack`,
-      binaryName: request.binaryName,
-      binaryVersion: `corepack/${corepackVersion}`,
-    });
-
-    cli.register(class BinaryCommand extends Command<Context> {
-      proxy = Option.Proxy();
-      async execute() {
-        return this.context.engine.executePackageManagerRequest(request, {
-          cwd: this.context.cwd,
-          args: this.proxy,
-        });
-      }
-    });
+    const context = {
+      ...Cli.defaultContext,
+      cwd: process.cwd(),
+      engine,
+    };
 
-    code = await cli.run(restArgs, context);
-  }
+    const code = await cli.run(argv, context);
 
-  if (code !== 0) {
-    process.exitCode ??= code;
+    if (code !== 0) {
+      process.exitCode ??= code;
+    }
+  } else {
+    await engine.executePackageManagerRequest(request, {
+      cwd: process.cwd(),
+      args: restArgs,
+    });
   }
 }
diff --git a/tests/main.test.ts b/tests/main.test.ts
@@ -24,8 +24,8 @@ it(`should refuse to download a package manager if the hash doesn't match`, asyn
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /Mismatch hashes/,
+      stderr: /Mismatch hashes/,
+      stdout: ``,
     });
   });
 });
@@ -35,8 +35,8 @@ it(`should refuse to download a known package manager from a URL`, async () => {
     // Package managers known by Corepack cannot be loaded from a URL.
     await expect(runCli(cwd, [`yarn@https://registry.npmjs.com/yarn/-/yarn-1.22.21.tgz`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /Illegal use of URL for known package manager/,
+      stderr: /Illegal use of URL for known package manager/,
+      stdout: ``,
     });
 
     // Unknown package managers can be loaded from a URL.
@@ -57,8 +57,8 @@ it.failing(`should refuse to download a known package manager from a URL in pack
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /Illegal use of URL for known package manager/,
+      stderr: /Illegal use of URL for known package manager/,
+      stdout: ``,
     });
 
     // Unknown package managers can be loaded from a URL.
@@ -82,8 +82,8 @@ it(`should require a version to be specified`, async () => {
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /expected a semver version/,
+      stderr: /expected a semver version/,
+      stdout: ``,
     });
 
     await xfs.writeJsonPromise(ppath.join(cwd, `package.json` as Filename), {
@@ -92,8 +92,8 @@ it(`should require a version to be specified`, async () => {
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /expected a semver version/,
+      stderr: /expected a semver version/,
+      stdout: ``,
     });
 
     await xfs.writeJsonPromise(ppath.join(cwd, `package.json` as Filename), {
@@ -102,8 +102,8 @@ it(`should require a version to be specified`, async () => {
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
-      stdout: /expected a semver version/,
+      stderr: /expected a semver version/,
+      stdout: ``,
     });
   });
 });
@@ -272,7 +272,7 @@ it(`shouldn't allow using regular Yarn commands on npm-configured projects`, asy
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
       exitCode: 1,
-      stderr: ``,
+      stderr: /This project is configured to use npm/,
     });
   });
 });
@@ -419,9 +419,10 @@ it(`should refuse to run a different package manager within a configured project
     process.env.FORCE_COLOR = `0`;
 
     await expect(runCli(cwd, [`pnpm`, `--version`])).resolves.toMatchObject({
-      stdout: `Usage Error: This project is configured to use yarn because ${
+      stdout: ``,
+      stderr: expect.stringContaining(`This project is configured to use yarn because ${
         npath.fromPortablePath(ppath.join(cwd, `package.json` as Filename))
-      } has a "packageManager" field\n\n$ pnpm ...\n`,
+      } has a "packageManager" field`),
       exitCode: 1,
     });
 
@@ -471,8 +472,8 @@ it(`should support disabling the network accesses from the environment`, async (
     });
 
     await expect(runCli(cwd, [`yarn`, `--version`])).resolves.toMatchObject({
-      stdout: expect.stringContaining(`Network access disabled by the environment`),
-      stderr: ``,
+      stdout: ``,
+      stderr: /Network access disabled by the environment/,
       exitCode: 1,
     });
   });
@@ -998,13 +999,13 @@ describe(`handle integrity checks`, () => {
     await xfs.mktempPromise(async cwd => {
       await expect(runCli(cwd, [`[email protected]`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`yarn@stable`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
     });
   });
@@ -1014,19 +1015,19 @@ describe(`handle integrity checks`, () => {
     await xfs.mktempPromise(async cwd => {
       await expect(runCli(cwd, [`pnpm`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
-        stderr: ``,
+        stderr: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
+        stdout: ``,
       });
       // A second time to validate the invalid version was not cached.
       await expect(runCli(cwd, [`pnpm`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
-        stderr: ``,
+        stderr: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`yarn`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
-        stderr: ``,
+        stderr: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`use`, `pnpm`], true)).resolves.toMatchObject({
         exitCode: 1,
@@ -1041,19 +1042,19 @@ describe(`handle integrity checks`, () => {
     await xfs.mktempPromise(async cwd => {
       await expect(runCli(cwd, [`pnpm`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
       // A second time to validate the invalid version was not cached.
       await expect(runCli(cwd, [`pnpm`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`yarn`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`use`, `pnpm`], true)).resolves.toMatchObject({
         exitCode: 1,
@@ -1068,8 +1069,8 @@ describe(`handle integrity checks`, () => {
     await xfs.mktempPromise(async cwd => {
       await expect(runCli(cwd, [`[email protected]`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Signature does not match/,
-        stderr: ``,
+        stderr: /Signature does not match/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`use`, `[email protected]`], true)).resolves.toMatchObject({
         exitCode: 1,
@@ -1084,8 +1085,8 @@ describe(`handle integrity checks`, () => {
     await xfs.mktempPromise(async cwd => {
       await expect(runCli(cwd, [`[email protected]`, `--version`], true)).resolves.toMatchObject({
         exitCode: 1,
-        stdout: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
-        stderr: ``,
+        stderr: /Mismatch hashes. Expected [a-f0-9]{128}, got [a-f0-9]{128}/,
+        stdout: ``,
       });
       await expect(runCli(cwd, [`use`, `[email protected]`], true)).resolves.toMatchObject({
         exitCode: 1,
@@ -1101,10 +1102,10 @@ describe(`handle integrity checks`, () => {
       const result = await runCli(cwd, [`[email protected]+sha1.deadbeef`, `--version`], true);
       expect(result).toMatchObject({
         exitCode: 1,
-        stderr: ``,
+        stdout: ``,
       });
-      const match = /Mismatch hashes. Expected deadbeef, got ([a-f0-9]{40})/.exec(result.stdout);
-      if (match == null) throw new Error(`Invalid output`, {cause: result.stdout});
+      const match = /Mismatch hashes. Expected deadbeef, got ([a-f0-9]{40})/.exec(result.stderr);
+      if (match == null) throw new Error(`Invalid output`, {cause: result.stderr});
       await expect(runCli(cwd, [`[email protected]+sha1.${match[1]}`, `--version`], true)).resolves.toMatchObject({
         exitCode: 0,
         stdout: `yarn: Hello from custom registry\n`,