From 620cb807a1cd8253c20963a632f0d6b2b62cf8e3 Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Wed, 31 Jan 2024 13:26:07 +0100 Subject: [PATCH] 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 7c21c34edc780905a24f83ec9a557c59120c2301) --- code/lib/cli/src/upgrade.test.ts | 43 ++++++++++++++++++++++++-- code/lib/cli/src/upgrade.ts | 40 ++++++++++++++++-------- code/lib/telemetry/src/index.ts | 2 -- code/lib/telemetry/src/package-json.ts | 6 ---- 4 files changed, 67 insertions(+), 24 deletions(-) diff --git a/code/lib/cli/src/upgrade.test.ts b/code/lib/cli/src/upgrade.test.ts index 56827546b548..7252e3ad6ba4 100644 --- a/code/lib/cli/src/upgrade.test.ts +++ b/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']); }); }); diff --git a/code/lib/cli/src/upgrade.ts b/code/lib/cli/src/upgrade.ts index 5537d3fe6b2a..e222e2603c44 100644 --- a/code/lib/cli/src/upgrade.ts +++ b/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; 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, diff --git a/code/lib/telemetry/src/index.ts b/code/lib/telemetry/src/index.ts index cd5e81dd83c2..5a318cd91b64 100644 --- a/code/lib/telemetry/src/index.ts +++ b/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'; diff --git a/code/lib/telemetry/src/package-json.ts b/code/lib/telemetry/src/package-json.ts index 56e6823b5920..fe860382dd7f 100644 --- a/code/lib/telemetry/src/package-json.ts +++ b/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; -};