From b5efbfe0c00a4fad8f161282d08362b642e4a4cb Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Wed, 24 Jan 2024 14:44:39 +0100 Subject: [PATCH 1/6] use package manager CLIs to get currently installed versions --- code/lib/cli/src/upgrade.ts | 11 ++++++++--- code/lib/telemetry/src/index.ts | 2 -- code/lib/telemetry/src/package-json.ts | 6 ------ 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/code/lib/cli/src/upgrade.ts b/code/lib/cli/src/upgrade.ts index 14135e44b2e7..2c42b1bc0cdb 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, { eq, lt, prerelease } from 'semver'; import { logger } from '@storybook/node-logger'; import { withTelemetry } from '@storybook/core-server'; @@ -114,7 +114,10 @@ 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'; + const beforeVersion = + (await packageManager.findInstallations(['@storybook/cli']))?.dependencies['@storybook/cli'][0] + .version ?? '0.0.0'; + const currentVersion = versions['@storybook/cli']; const isCanary = currentVersion.startsWith('0.0.0'); @@ -206,7 +209,9 @@ export const doUpgrade = async ({ automigrationResults = await automigrate({ dryRun, yes, packageManager: pkgMgr, configDir }); } if (!options.disableTelemetry) { - const afterVersion = await getStorybookCoreVersion(); + const afterVersion = (await packageManager.findInstallations(['@storybook/cli']))?.dependencies[ + '@storybook/cli' + ][0].version; 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; -}; From 0df3e942f7af7c2bd78c4d0fea3e811a5da0bb13 Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Thu, 25 Jan 2024 10:36:38 +0100 Subject: [PATCH 2/6] refactor upgrade tests --- code/lib/cli/src/upgrade.test.ts | 36 +++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/code/lib/cli/src/upgrade.test.ts b/code/lib/cli/src/upgrade.test.ts index 3987b9b35789..5f9cf57787db 100644 --- a/code/lib/cli/src/upgrade.test.ts +++ b/code/lib/cli/src/upgrade.test.ts @@ -1,5 +1,4 @@ import { describe, it, expect, vi } from 'vitest'; -import { getStorybookCoreVersion } from '@storybook/telemetry'; import { UpgradeStorybookToLowerVersionError, UpgradeStorybookToSameVersionError, @@ -8,11 +7,18 @@ import { doUpgrade, getStorybookVersion } from './upgrade'; import type * as sbcc from '@storybook/core-common'; +const findInstallationsMock = vi.fn>(); + vi.mock('@storybook/telemetry'); vi.mock('@storybook/core-common', async (importOriginal) => { const originalModule = (await importOriginal()) as typeof sbcc; return { ...originalModule, + JsPackageManagerFactory: { + getPackageManager: () => ({ + findInstallations: findInstallationsMock, + }), + }, versions: Object.keys(originalModule.versions).reduce( (acc, key) => { acc[key] = '8.0.0'; @@ -46,13 +52,37 @@ describe.each([ describe('Upgrade errors', () => { it('should throw an error when upgrading to a lower version number', async () => { - vi.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/cli']); }); it('should throw an error when upgrading to the same version number', async () => { - vi.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/cli']); }); }); From d33215886774915714f916382fada8e4299cacab Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Thu, 25 Jan 2024 11:03:01 +0100 Subject: [PATCH 3/6] don't fail if @storybook/cli can't be found --- code/lib/cli/src/upgrade.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/code/lib/cli/src/upgrade.ts b/code/lib/cli/src/upgrade.ts index 2c42b1bc0cdb..2021bf4b5df5 100644 --- a/code/lib/cli/src/upgrade.ts +++ b/code/lib/cli/src/upgrade.ts @@ -115,8 +115,9 @@ export const doUpgrade = async ({ // If we can't determine the existing version (Yarn PnP), fallback to v0.0.0 to not block the upgrade const beforeVersion = - (await packageManager.findInstallations(['@storybook/cli']))?.dependencies['@storybook/cli'][0] - .version ?? '0.0.0'; + (await packageManager.findInstallations(['@storybook/cli']))?.dependencies[ + '@storybook/cli' + ]?.[0].version ?? '0.0.0'; const currentVersion = versions['@storybook/cli']; const isCanary = currentVersion.startsWith('0.0.0'); From 154d9795c1d2f95ab2ab53020e7168ce802029f7 Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Fri, 26 Jan 2024 10:02:01 +0100 Subject: [PATCH 4/6] look for storybook instead of @storybook/cli --- code/lib/cli/src/upgrade.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/code/lib/cli/src/upgrade.ts b/code/lib/cli/src/upgrade.ts index 2021bf4b5df5..5247977c3ba8 100644 --- a/code/lib/cli/src/upgrade.ts +++ b/code/lib/cli/src/upgrade.ts @@ -115,9 +115,8 @@ export const doUpgrade = async ({ // If we can't determine the existing version (Yarn PnP), fallback to v0.0.0 to not block the upgrade const beforeVersion = - (await packageManager.findInstallations(['@storybook/cli']))?.dependencies[ - '@storybook/cli' - ]?.[0].version ?? '0.0.0'; + (await packageManager.findInstallations(['@storybook/cli']))?.dependencies['storybook']?.[0] + .version ?? '0.0.0'; const currentVersion = versions['@storybook/cli']; const isCanary = currentVersion.startsWith('0.0.0'); From 424630b7c6ad65e2ff293c852f7004b2e9a07cc7 Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Fri, 26 Jan 2024 10:07:32 +0100 Subject: [PATCH 5/6] use both @storybook/cli and storybook to detect version --- code/lib/cli/src/upgrade.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/code/lib/cli/src/upgrade.ts b/code/lib/cli/src/upgrade.ts index 5247977c3ba8..1d13c5a7f0c2 100644 --- a/code/lib/cli/src/upgrade.ts +++ b/code/lib/cli/src/upgrade.ts @@ -11,7 +11,7 @@ import { import chalk from 'chalk'; import dedent from 'ts-dedent'; import boxen from 'boxen'; -import type { PackageManagerName } from '@storybook/core-common'; +import type { JsPackageManager, PackageManagerName } from '@storybook/core-common'; import { JsPackageManagerFactory, isCorePackage, @@ -37,6 +37,18 @@ export const getStorybookVersion = (line: string) => { }; }; +const getInstalledStorybookVersion = async (packageManager: JsPackageManager) => { + const installations = await packageManager.findInstallations(['storybook', '@storybook/cli']); + if (!installations) { + return; + } + 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', @@ -113,10 +125,8 @@ export const doUpgrade = async ({ }: UpgradeOptions) => { 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 packageManager.findInstallations(['@storybook/cli']))?.dependencies['storybook']?.[0] - .version ?? '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'); @@ -209,9 +219,7 @@ export const doUpgrade = async ({ automigrationResults = await automigrate({ dryRun, yes, packageManager: pkgMgr, configDir }); } if (!options.disableTelemetry) { - const afterVersion = (await packageManager.findInstallations(['@storybook/cli']))?.dependencies[ - '@storybook/cli' - ][0].version; + const afterVersion = await getInstalledStorybookVersion(packageManager); const { preCheckFailure, fixResults } = automigrationResults || {}; const automigrationTelemetry = { automigrationResults: preCheckFailure ? null : fixResults, From d209ba420aff852fea4a8c4978adc110fed19113 Mon Sep 17 00:00:00 2001 From: Jeppe Reinhold Date: Fri, 26 Jan 2024 11:53:43 +0100 Subject: [PATCH 6/6] add storybook to upgrade test to match behavior --- code/lib/cli/src/upgrade.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/lib/cli/src/upgrade.test.ts b/code/lib/cli/src/upgrade.test.ts index 5f9cf57787db..149951460724 100644 --- a/code/lib/cli/src/upgrade.test.ts +++ b/code/lib/cli/src/upgrade.test.ts @@ -66,7 +66,7 @@ describe('Upgrade errors', () => { }); await expect(doUpgrade({} as any)).rejects.toThrowError(UpgradeStorybookToLowerVersionError); - expect(findInstallationsMock).toHaveBeenCalledWith(['@storybook/cli']); + expect(findInstallationsMock).toHaveBeenCalledWith(['storybook', '@storybook/cli']); }); it('should throw an error when upgrading to the same version number', async () => { findInstallationsMock.mockResolvedValue({ @@ -83,6 +83,6 @@ describe('Upgrade errors', () => { }); await expect(doUpgrade({} as any)).rejects.toThrowError(UpgradeStorybookToSameVersionError); - expect(findInstallationsMock).toHaveBeenCalledWith(['@storybook/cli']); + expect(findInstallationsMock).toHaveBeenCalledWith(['storybook', '@storybook/cli']); }); });