Merge pull request #25752 from storybookjs/jeppe/fix-upgrade-version-…

…detection

CLI: Fix `upgrade` detecting the wrong version of existing Storybooks
(cherry picked from commit 7c21c34)

code/lib/cli/src/upgrade.test.ts

@@ -1,4 +1,3 @@
-import { getStorybookCoreVersion } from '@storybook/telemetry';
 import {
   UpgradeStorybookToLowerVersionError,
   UpgradeStorybookToSameVersionError,
@@ -16,6 +15,20 @@ jest.mock('./versions', () => {
   };
 });
 
+const findInstallationsMock = jest.fn();
+
+jest.mock('./js-package-manager', () => {
+  const originalModule = jest.requireActual('./js-package-manager');
+  return {
+    ...originalModule,
+    JsPackageManagerFactory: {
+      getPackageManager: () => ({
+        findInstallations: findInstallationsMock,
+      }),
+    },
+  };
+});
+
 describe.each([
   ['│ │ │ ├── @babel/code-frame@7.10.3 deduped', null],
   [
@@ -39,13 +52,37 @@ describe.each([
 
 describe('Upgrade errors', () => {
   it('should throw an error when upgrading to a lower version number', async () => {
-    jest.mocked(getStorybookCoreVersion).mockResolvedValue('8.1.0');
+    findInstallationsMock.mockResolvedValue({
+      dependencies: {
+        '@storybook/cli': [
+          {
+            version: '8.1.0',
+          },
+        ],
+      },
+      duplicatedDependencies: {},
+      infoCommand: '',
+      dedupeCommand: '',
+    });
 
     await expect(doUpgrade({} as any)).rejects.toThrowError(UpgradeStorybookToLowerVersionError);
+    expect(findInstallationsMock).toHaveBeenCalledWith(['storybook', '@storybook/cli']);
   });
   it('should throw an error when upgrading to the same version number', async () => {
-    jest.mocked(getStorybookCoreVersion).mockResolvedValue('8.0.0');
+    findInstallationsMock.mockResolvedValue({
+      dependencies: {
+        '@storybook/cli': [
+          {
+            version: '8.0.0',
+          },
+        ],
+      },
+      duplicatedDependencies: {},
+      infoCommand: '',
+      dedupeCommand: '',
+    });
 
     await expect(doUpgrade({} as any)).rejects.toThrowError(UpgradeStorybookToSameVersionError);
+    expect(findInstallationsMock).toHaveBeenCalledWith(['storybook', '@storybook/cli']);
   });
 });

code/lib/cli/src/upgrade.ts

@@ -1,5 +1,5 @@
 import { sync as spawnSync } from 'cross-spawn';
-import { telemetry, getStorybookCoreVersion } from '@storybook/telemetry';
+import { telemetry } from '@storybook/telemetry';
 import semver, { coerce, eq, lt } from 'semver';
 import { deprecate, logger } from '@storybook/node-logger';
 import { withTelemetry } from '@storybook/core-server';
@@ -11,7 +11,11 @@ import {
 import chalk from 'chalk';
 import dedent from 'ts-dedent';
 import boxen from 'boxen';
-import type { PackageJsonWithMaybeDeps, PackageManagerName } from './js-package-manager';
+import type {
+  JsPackageManager,
+  PackageJsonWithMaybeDeps,
+  PackageManagerName,
+} from './js-package-manager';
 import { JsPackageManagerFactory, getPackageDetails, useNpmWarning } from './js-package-manager';
 import { commandLog } from './helpers';
 import { automigrate } from './automigrate';
@@ -34,6 +38,18 @@ export const getStorybookVersion = (line: string) => {
   };
 };
 
+const getInstalledStorybookVersion = async (packageManager: JsPackageManager) => {
+  const installations = await packageManager.findInstallations(['storybook', '@storybook/cli']);
+  if (!installations) {
+    return undefined;
+  }
+  const cliVersion = installations.dependencies['@storybook/cli']?.[0].version;
+  if (cliVersion) {
+    return cliVersion;
+  }
+  return installations.dependencies['storybook']?.[0].version;
+};
+
 const deprecatedPackages = [
   {
     minVersion: '6.0.0-alpha.0',
@@ -91,9 +107,7 @@ export const checkVersionConsistency = () => {
   });
 };
 
-/**
- * DEPRECATED BEHAVIOR SECTION
- */
+// #region DEPRECATED BEHAVIOR SECTION
 
 type ExtraFlags = Record<string, string[]>;
 const EXTRA_FLAGS: ExtraFlags = {
@@ -170,7 +184,8 @@ export const deprecatedUpgrade = async ({
   }
   const packageManager = JsPackageManagerFactory.getPackageManager({ force: pkgMgr });
 
-  const beforeVersion = await getStorybookCoreVersion();
+  // If we can't determine the existing version fallback to v0.0.0 to not block the upgrade
+  const beforeVersion = (await getInstalledStorybookVersion(packageManager)) ?? '0.0.0';
 
   commandLog(`Checking for latest versions of '@storybook/*' packages`);
 
@@ -221,7 +236,7 @@ export const deprecatedUpgrade = async ({
     automigrationResults = await automigrate({ dryRun, yes, packageManager: pkgMgr, configDir });
   }
   if (!options.disableTelemetry) {
-    const afterVersion = await getStorybookCoreVersion();
+    const afterVersion = await getInstalledStorybookVersion(packageManager);
     const { preCheckFailure, fixResults } = automigrationResults || {};
     const automigrationTelemetry = {
       automigrationResults: preCheckFailure ? null : fixResults,
@@ -237,9 +252,7 @@ export const deprecatedUpgrade = async ({
   }
 };
 
-/**
- * DEPRECATED BEHAVIOR SECTION END
- */
+// #endregion DEPRECATED BEHAVIOR SECTION
 
 export interface UpgradeOptions {
   /**
@@ -292,8 +305,9 @@ export const doUpgrade = async ({
 
   const packageManager = JsPackageManagerFactory.getPackageManager({ force: pkgMgr });
 
-  // If we can't determine the existing version (Yarn PnP), fallback to v0.0.0 to not block the upgrade
-  const beforeVersion = (await getStorybookCoreVersion()) ?? '0.0.0';
+  // If we can't determine the existing version fallback to v0.0.0 to not block the upgrade
+  const beforeVersion = (await getInstalledStorybookVersion(packageManager)) ?? '0.0.0';
+
   const currentVersion = versions['@storybook/cli'];
   const isCanary = currentVersion.startsWith('0.0.0');
 
@@ -385,7 +399,7 @@ export const doUpgrade = async ({
     automigrationResults = await automigrate({ dryRun, yes, packageManager: pkgMgr, configDir });
   }
   if (!options.disableTelemetry) {
-    const afterVersion = await getStorybookCoreVersion();
+    const afterVersion = await getInstalledStorybookVersion(packageManager);
     const { preCheckFailure, fixResults } = automigrationResults || {};
     const automigrationTelemetry = {
       automigrationResults: preCheckFailure ? null : fixResults,

code/lib/telemetry/src/index.ts

@@ -11,8 +11,6 @@ export * from './storybook-metadata';
 
 export * from './types';
 
-export { getStorybookCoreVersion } from './package-json';
-
 export { getPrecedingUpgrade } from './event-cache';
 
 export { addToGlobalContext } from './telemetry';

code/lib/telemetry/src/package-json.ts

@@ -27,9 +27,3 @@ export const getActualPackageJson = async (packageName: string) => {
   const packageJson = await fs.readJson(resolvedPackageJson);
   return packageJson;
 };
-
-// Note that this probably doesn't work in Yarn PNP mode because @storybook/telemetry doesn't depend on @storybook/cli
-export const getStorybookCoreVersion = async () => {
-  const { version } = await getActualPackageVersion('@storybook/cli');
-  return version;
-};