Merge pull request #28699 from storybookjs/valentin/fix-installing-st…

…orybook-in-workspaces CLI: Fix the initialization of Storybook in workspaces
storybookjs · Jul 25, 2024 · e432880 · e432880
2 parents 141d8a2 + dd2f9b4
commit e432880
Show file tree

Hide file tree

Showing 10 changed files with 74 additions and 88 deletions.
diff --git a/code/core/src/common/js-package-manager/JsPackageManager.ts b/code/core/src/common/js-package-manager/JsPackageManager.ts
@@ -64,22 +64,6 @@ export abstract class JsPackageManager {
     return packageJSON ? packageJSON.version ?? null : null;
   }
 
-  // NOTE: for some reason yarn prefers the npm registry in
-  // local development, so always use npm
-  async setRegistryURL(url: string) {
-    if (url) {
-      await this.executeCommand({ command: 'npm', args: ['config', 'set', 'registry', url] });
-    } else {
-      await this.executeCommand({ command: 'npm', args: ['config', 'delete', 'registry'] });
-    }
-  }
-
-  async getRegistryURL() {
-    const res = await this.executeCommand({ command: 'npm', args: ['config', 'get', 'registry'] });
-    const url = res.trim();
-    return url === 'undefined' ? undefined : url;
-  }
-
   constructor(options?: JsPackageManagerOptions) {
     this.cwd = options?.cwd || process.cwd();
   }
@@ -487,6 +471,8 @@ export abstract class JsPackageManager {
   ): // Use generic and conditional type to force `string[]` if fetchAllVersions is true and `string` if false
   Promise<T extends true ? string[] : string>;
 
+  public abstract getRegistryURL(): Promise<string | undefined>;
+
   public abstract runPackageCommand(
     command: string,
     args: string[],

diff --git a/code/core/src/common/js-package-manager/NPMProxy.test.ts b/code/core/src/common/js-package-manager/NPMProxy.test.ts
@@ -34,21 +34,6 @@ describe('NPM Proxy', () => {
     });
   });
 
-  describe('setRegistryUrl', () => {
-    it('should run `npm config set registry https://foo.bar`', async () => {
-      const executeCommandSpy = vi.spyOn(npmProxy, 'executeCommand').mockResolvedValueOnce('');
-
-      await npmProxy.setRegistryURL('https://foo.bar');
-
-      expect(executeCommandSpy).toHaveBeenCalledWith(
-        expect.objectContaining({
-          command: 'npm',
-          args: ['config', 'set', 'registry', 'https://foo.bar'],
-        })
-      );
-    });
-  });
-
   describe('installDependencies', () => {
     describe('npm6', () => {
       it('should run `npm install`', async () => {

diff --git a/code/core/src/common/js-package-manager/NPMProxy.ts b/code/core/src/common/js-package-manager/NPMProxy.ts
@@ -181,6 +181,17 @@ export class NPMProxy extends JsPackageManager {
     });
   }
 
+  public async getRegistryURL() {
+    const res = await this.executeCommand({
+      command: 'npm',
+      // "npm config" commands are not allowed in workspaces per default
+      // https://github.com/npm/cli/issues/6099#issuecomment-1847584792
+      args: ['config', 'get', 'registry', '-ws=false', '-iwr'],
+    });
+    const url = res.trim();
+    return url === 'undefined' ? undefined : url;
+  }
+
   protected async runAddDeps(dependencies: string[], installAsDevDependencies: boolean) {
     const { logStream, readLogFile, moveLogFile, removeLogFile } = await createLogStream();
     let args = [...dependencies];

diff --git a/code/core/src/common/js-package-manager/PNPMProxy.test.ts b/code/core/src/common/js-package-manager/PNPMProxy.test.ts
@@ -24,21 +24,6 @@ describe('PNPM Proxy', () => {
     });
   });
 
-  describe('setRegistryUrl', () => {
-    it('should run `npm config set registry https://foo.bar`', async () => {
-      const executeCommandSpy = vi.spyOn(pnpmProxy, 'executeCommand').mockResolvedValueOnce('');
-
-      await pnpmProxy.setRegistryURL('https://foo.bar');
-
-      expect(executeCommandSpy).toHaveBeenCalledWith(
-        expect.objectContaining({
-          command: 'npm',
-          args: ['config', 'set', 'registry', 'https://foo.bar'],
-        })
-      );
-    });
-  });
-
   describe('installDependencies', () => {
     it('should run `pnpm install`', async () => {
       const executeCommandSpy = vi

diff --git a/code/core/src/common/js-package-manager/PNPMProxy.ts b/code/core/src/common/js-package-manager/PNPMProxy.ts
@@ -90,6 +90,15 @@ export class PNPMProxy extends JsPackageManager {
     });
   }
 
+  public async getRegistryURL() {
+    const res = await this.executeCommand({
+      command: 'pnpm',
+      args: ['config', 'get', 'registry'],
+    });
+    const url = res.trim();
+    return url === 'undefined' ? undefined : url;
+  }
+
   async runPackageCommand(command: string, args: string[], cwd?: string): Promise<string> {
     return this.executeCommand({
       command: 'pnpm',

diff --git a/code/core/src/common/js-package-manager/Yarn1Proxy.test.ts b/code/core/src/common/js-package-manager/Yarn1Proxy.test.ts
@@ -25,21 +25,6 @@ describe('Yarn 1 Proxy', () => {
     });
   });
 
-  describe('setRegistryUrl', () => {
-    it('should run `yarn config set npmRegistryServer https://foo.bar`', async () => {
-      const executeCommandSpy = vi.spyOn(yarn1Proxy, 'executeCommand').mockResolvedValueOnce('');
-
-      await yarn1Proxy.setRegistryURL('https://foo.bar');
-
-      expect(executeCommandSpy).toHaveBeenCalledWith(
-        expect.objectContaining({
-          command: 'npm',
-          args: ['config', 'set', 'registry', 'https://foo.bar'],
-        })
-      );
-    });
-  });
-
   describe('installDependencies', () => {
     it('should run `yarn`', async () => {
       const executeCommandSpy = vi.spyOn(yarn1Proxy, 'executeCommand').mockResolvedValueOnce('');

diff --git a/code/core/src/common/js-package-manager/Yarn1Proxy.ts b/code/core/src/common/js-package-manager/Yarn1Proxy.ts
@@ -83,6 +83,15 @@ export class Yarn1Proxy extends JsPackageManager {
     return JSON.parse(readFileSync(packageJsonPath, 'utf-8')) as Record<string, any>;
   }
 
+  public async getRegistryURL() {
+    const res = await this.executeCommand({
+      command: 'yarn',
+      args: ['config', 'get', 'registry'],
+    });
+    const url = res.trim();
+    return url === 'undefined' ? undefined : url;
+  }
+
   public async findInstallations(pattern: string[], { depth = 99 }: { depth?: number } = {}) {
     const yarnArgs = ['list', '--pattern', pattern.map((p) => `"${p}"`).join(' '), '--json'];
 

diff --git a/code/core/src/common/js-package-manager/Yarn2Proxy.test.ts b/code/core/src/common/js-package-manager/Yarn2Proxy.test.ts
@@ -53,21 +53,6 @@ describe('Yarn 2 Proxy', () => {
     });
   });
 
-  describe('setRegistryUrl', () => {
-    it('should run `yarn config set npmRegistryServer https://foo.bar`', async () => {
-      const executeCommandSpy = vi.spyOn(yarn2Proxy, 'executeCommand').mockResolvedValueOnce('');
-
-      await yarn2Proxy.setRegistryURL('https://foo.bar');
-
-      expect(executeCommandSpy).toHaveBeenCalledWith(
-        expect.objectContaining({
-          command: 'npm',
-          args: ['config', 'set', 'registry', 'https://foo.bar'],
-        })
-      );
-    });
-  });
-
   describe('addDependencies', () => {
     it('with devDep it should run `yarn install -D @storybook/core`', async () => {
       const executeCommandSpy = vi.spyOn(yarn2Proxy, 'executeCommand').mockResolvedValueOnce('');

diff --git a/code/core/src/common/js-package-manager/Yarn2Proxy.ts b/code/core/src/common/js-package-manager/Yarn2Proxy.ts
@@ -239,6 +239,15 @@ export class Yarn2Proxy extends JsPackageManager {
     await removeLogFile();
   }
 
+  public async getRegistryURL() {
+    const res = await this.executeCommand({
+      command: 'yarn',
+      args: ['config', 'get', 'npmRegistryServer'],
+    });
+    const url = res.trim();
+    return url === 'undefined' ? undefined : url;
+  }
+
   protected async runRemoveDeps(dependencies: string[]) {
     const args = [...dependencies];
 

diff --git a/scripts/sandbox/generate.ts b/scripts/sandbox/generate.ts
@@ -45,18 +45,34 @@ const sbInit = async (
   await runCommand(`${sbCliBinaryPath} init ${fullFlags.join(' ')}`, { cwd, env }, debug);
 };
 
-const withLocalRegistry = async (packageManager: JsPackageManager, action: () => Promise<void>) => {
+type LocalRegistryProps = {
+  packageManager: JsPackageManager;
+  action: () => Promise<void>;
+  cwd: string;
+  env: Record<string, any>;
+  debug: boolean;
+};
+
+const withLocalRegistry = async ({
+  packageManager,
+  action,
+  cwd,
+  env,
+  debug,
+}: LocalRegistryProps) => {
   const prevUrl = await packageManager.getRegistryURL();
   let error;
   try {
     console.log(`📦 Configuring local registry: ${LOCAL_REGISTRY_URL}`);
-    packageManager.setRegistryURL(LOCAL_REGISTRY_URL);
+    // NOTE: for some reason yarn prefers the npm registry in
+    // local development, so always use npm
+    await runCommand(`npm config set registry ${LOCAL_REGISTRY_URL}`, { cwd, env }, debug);
     await action();
   } catch (e) {
     error = e;
   } finally {
     console.log(`📦 Restoring registry: ${prevUrl}`);
-    await packageManager.setRegistryURL(prevUrl);
+    await runCommand(`npm config set registry ${prevUrl}`, { cwd, env }, debug);
 
     if (error) {
       throw error;
@@ -88,14 +104,20 @@ const addStorybook = async ({
 
     const packageManager = JsPackageManagerFactory.getPackageManager({ force: 'yarn1' }, tmpDir);
     if (localRegistry) {
-      await withLocalRegistry(packageManager, async () => {
-        await packageManager.addPackageResolutions({
-          ...storybookVersions,
-          // Yarn1 Issue: https://github.com/storybookjs/storybook/issues/22431
-          jackspeak: '2.1.1',
-        });
-
-        await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
+      await withLocalRegistry({
+        packageManager,
+        action: async () => {
+          await packageManager.addPackageResolutions({
+            ...storybookVersions,
+            // Yarn1 Issue: https://github.com/storybookjs/storybook/issues/22431
+            jackspeak: '2.1.1',
+          });
+
+          await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
+        },
+        cwd: tmpDir,
+        env,
+        debug,
       });
     } else {
       await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
@@ -159,7 +181,7 @@ const runGenerators = async (
         const baseDir = join(REPROS_DIRECTORY, dirName);
         const beforeDir = join(baseDir, BEFORE_DIR_NAME);
         try {
-          let flags: string[] = [];
+          let flags: string[] = ['--no-dev'];
           if (expected.renderer === '@storybook/html') flags = ['--type html'];
           else if (expected.renderer === '@storybook/server') flags = ['--type server'];