From 80ff5722d4eb803834d6932c33d6b259541891d1 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Thu, 12 Dec 2024 16:09:29 +0100 Subject: [PATCH 01/15] Remove warnings API --- code/.storybook/preview.tsx | 6 +-- code/addons/a11y/src/params.ts | 3 -- code/addons/a11y/src/preview.test.tsx | 54 ------------------- code/addons/a11y/src/preview.tsx | 9 +--- ...addon-a11y-test-override-warning-levels.md | 32 ----------- docs/writing-tests/accessibility-testing.mdx | 18 ++----- 6 files changed, 6 insertions(+), 116 deletions(-) delete mode 100644 docs/_snippets/storybook-addon-a11y-test-override-warning-levels.md diff --git a/code/.storybook/preview.tsx b/code/.storybook/preview.tsx index 017419318b0a..04210af3f5af 100644 --- a/code/.storybook/preview.tsx +++ b/code/.storybook/preview.tsx @@ -358,9 +358,5 @@ export const parameters = { opacity: 0.4, }, }, - a11y: { - warnings: ['minor', 'moderate', 'serious', 'critical'], - }, + tags: ['test', 'vitest', '!a11ytest'], }; - -export const tags = ['test', 'vitest']; diff --git a/code/addons/a11y/src/params.ts b/code/addons/a11y/src/params.ts index dd4357687340..e66a0813a42c 100644 --- a/code/addons/a11y/src/params.ts +++ b/code/addons/a11y/src/params.ts @@ -6,8 +6,6 @@ export interface Setup { options: RunOptions; } -type Impact = NonNullable; - export interface A11yParameters { element?: ElementContext; config?: Spec; @@ -15,5 +13,4 @@ export interface A11yParameters { /** @deprecated Use globals.a11y.manual instead */ manual?: boolean; disable?: boolean; - warnings?: Impact[]; } diff --git a/code/addons/a11y/src/preview.test.tsx b/code/addons/a11y/src/preview.test.tsx index 334f7f924aeb..d09ba89462ed 100644 --- a/code/addons/a11y/src/preview.test.tsx +++ b/code/addons/a11y/src/preview.test.tsx @@ -156,60 +156,6 @@ describe('afterEach', () => { }); }); - it('should report warning status when there are only warnings', async () => { - const context = createContext({ - parameters: { - a11y: { - warnings: ['minor'], - }, - }, - }); - const result = { - violations: [ - { impact: 'minor', nodes: [] }, - { impact: 'critical', nodes: [] }, - ], - }; - mockedRun.mockResolvedValue(result as any); - - await expect(async () => experimental_afterEach(context)).rejects.toThrow(); - - expect(mockedRun).toHaveBeenCalledWith(context.parameters.a11y); - expect(context.reporting.addReport).toHaveBeenCalledWith({ - type: 'a11y', - version: 1, - result, - status: 'failed', - }); - }); - - it('should report error status when there are warnings and errors', async () => { - const context = createContext({ - parameters: { - a11y: { - warnings: ['minor'], - }, - }, - }); - const result = { - violations: [ - { impact: 'minor', nodes: [] }, - { impact: 'critical', nodes: [] }, - ], - }; - mockedRun.mockResolvedValue(result as any); - - await expect(async () => experimental_afterEach(context)).rejects.toThrow(); - - expect(mockedRun).toHaveBeenCalledWith(context.parameters.a11y); - expect(context.reporting.addReport).toHaveBeenCalledWith({ - type: 'a11y', - version: 1, - result, - status: 'failed', - }); - }); - it('should run accessibility checks if "a11ytest" flag is not available and is not running in Vitest', async () => { const context = createContext({ tags: [], diff --git a/code/addons/a11y/src/preview.tsx b/code/addons/a11y/src/preview.tsx index f7d2f9aa43ff..d4bec863b015 100644 --- a/code/addons/a11y/src/preview.tsx +++ b/code/addons/a11y/src/preview.tsx @@ -21,7 +21,6 @@ export const experimental_afterEach: AfterEach = async ({ }) => { const a11yParameter: A11yParameters | undefined = parameters.a11y; const a11yGlobals = globals.a11y; - const warnings = a11yParameter?.warnings ?? []; const shouldRunEnvironmentIndependent = a11yParameter?.manual !== true && @@ -38,15 +37,11 @@ export const experimental_afterEach: AfterEach = async ({ if (result) { const hasViolations = (result?.violations.length ?? 0) > 0; - const hasErrors = result?.violations.some( - (violation) => !warnings.includes(violation.impact!) - ); - reporting.addReport({ type: 'a11y', version: 1, result: result, - status: hasErrors ? 'failed' : hasViolations ? 'warning' : 'passed', + status: hasViolations ? 'warning' : 'passed', }); /** @@ -58,7 +53,7 @@ export const experimental_afterEach: AfterEach = async ({ * implement proper try catch handling. */ if (getIsVitestStandaloneRun()) { - if (hasErrors) { + if (hasViolations) { // @ts-expect-error - todo - fix type extension of expect from @storybook/test expect(result).toHaveNoViolations(); } diff --git a/docs/_snippets/storybook-addon-a11y-test-override-warning-levels.md b/docs/_snippets/storybook-addon-a11y-test-override-warning-levels.md deleted file mode 100644 index 120a5ad6de8b..000000000000 --- a/docs/_snippets/storybook-addon-a11y-test-override-warning-levels.md +++ /dev/null @@ -1,32 +0,0 @@ -```js filename=".storybook/preview.js" renderer="common" language="js" -export default { - parameters: { - a11y: { - /* - * Configure the warning levels for a11y checks - * The available options are 'minor', 'moderate', 'serious', and 'critical' - */ - warnings: ['minor', 'moderate'], - }, - }, -}; -``` - -```ts filename=".storybook/preview.ts" renderer="common" language="ts" -// Replace your-framework with the framework you are using (e.g., react, vue3) -import { Preview } from '@storybook/your-framework'; - -const preview: Preview = { - parameters: { - a11y: { - /* - * Configure the warning levels for a11y checks - * The available options are 'minor', 'moderate', 'serious', and 'critical' - */ - warnings: ['minor', 'moderate'], - }, - }, -}; - -export default preview; -``` diff --git a/docs/writing-tests/accessibility-testing.mdx b/docs/writing-tests/accessibility-testing.mdx index 7072edb154ad..99139136834d 100644 --- a/docs/writing-tests/accessibility-testing.mdx +++ b/docs/writing-tests/accessibility-testing.mdx @@ -100,7 +100,7 @@ Customize the a11y ruleset at the story level by updating your story to include If you are using Svelte CSF, you can turn off automated accessibility testing for stories or components by adding globals to your story or adjusting the defineMeta function with the required configuration. With a regular CSF story, you can add the following to your story's export or component's default export: - + @@ -130,18 +130,6 @@ If you enabled the addon and you're manually upgrading to Storybook 8.5 or later -### Override accessibility violation levels - -By default, when the accessibility addon runs with the test addon enabled, it interprets all violations as errors. This means that if a story has a minor accessibility violation, the test will fail. However, you can override this behavior by setting the `warnings` parameter in the `a11y` configuration object to define an array of impact levels that should be considered warnings. - -{/* prettier-ignore-start */} - - - -{/* prettier-ignore-end */} - -In the example above, we configured all the `minor` or `moderate` accessibility violations to be considered warnings. All other levels (i.e., `serious` or `critical`) will continue to be considered errors, fail the test, and report the results accordingly in the Storybook UI or Vitest. - ### Configure accessibility tests with the test addon If you want to run accessibility tests only for a subset of your stories, you can use the [tags](../writing-stories/tags.mdx) mechanism to filter the tests you want to run with the test addon. For example, to turn off accessibility tests for a specific story, add the `!a11ytest` tag to the story's meta or directly to the story's `tags` configuration option. For example: @@ -154,8 +142,8 @@ If you want to run accessibility tests only for a subset of your stories, you ca - Tags can be applied at the project, component (meta), or story levels. Read our [documentation](../writing-stories/tags.mdx) for more information on configuring tags. - + Tags can be applied at the project, component (meta), or story levels. Read our [documentation](../writing-stories/tags.mdx) for more information on configuring tags. + From bc8181defde4804b7af60f2bbbab9dc4a7987546 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Thu, 12 Dec 2024 16:15:44 +0100 Subject: [PATCH 02/15] Show errors of axe properly --- code/addons/a11y/src/components/A11YPanel.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/code/addons/a11y/src/components/A11YPanel.tsx b/code/addons/a11y/src/components/A11YPanel.tsx index dc048d97b941..61ea9a663af4 100644 --- a/code/addons/a11y/src/components/A11YPanel.tsx +++ b/code/addons/a11y/src/components/A11YPanel.tsx @@ -133,7 +133,11 @@ export const A11YPanel: React.FC = () => { <> The accessibility scan encountered an error.
- {typeof error === 'string' ? error : JSON.stringify(error)} + {typeof error === 'string' + ? error + : error instanceof Error + ? error.toString() + : JSON.stringify(error)} )} From 25a3a33ba51a77650a0fb66d4223d8ff12fd277f Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Thu, 12 Dec 2024 16:28:11 +0100 Subject: [PATCH 03/15] Fix status to be failed --- code/addons/a11y/src/preview.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/addons/a11y/src/preview.tsx b/code/addons/a11y/src/preview.tsx index d4bec863b015..e496894cb113 100644 --- a/code/addons/a11y/src/preview.tsx +++ b/code/addons/a11y/src/preview.tsx @@ -41,7 +41,7 @@ export const experimental_afterEach: AfterEach = async ({ type: 'a11y', version: 1, result: result, - status: hasViolations ? 'warning' : 'passed', + status: hasViolations ? 'failed' : 'passed', }); /** From f0f4e48ae27b7c35d03860c4957b1f7fedbc3c2c Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 13 Dec 2024 16:18:06 +0100 Subject: [PATCH 04/15] Fix printing null% --- code/addons/test/src/components/TestProviderRender.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/addons/test/src/components/TestProviderRender.tsx b/code/addons/test/src/components/TestProviderRender.tsx index 0c1185ab6f2a..5fdcd8ed2c55 100644 --- a/code/addons/test/src/components/TestProviderRender.tsx +++ b/code/addons/test/src/components/TestProviderRender.tsx @@ -314,7 +314,7 @@ export const TestProviderRender: FC< aria-label={`status: ${coverageSummary.status}`} /> } - right={`${coverageSummary.percentage}%`} + right={coverageSummary.percentage ? `${coverageSummary.percentage}%` : null} /> ) : ( Date: Sun, 15 Dec 2024 22:26:01 +1100 Subject: [PATCH 05/15] Record `metadata.hasRouterPackage` --- .../telemetry/get-has-router-package.test.ts | 29 +++++++++++++++ .../src/telemetry/get-has-router-package.ts | 37 +++++++++++++++++++ code/core/src/telemetry/storybook-metadata.ts | 3 ++ code/core/src/telemetry/types.ts | 1 + 4 files changed, 70 insertions(+) create mode 100644 code/core/src/telemetry/get-has-router-package.test.ts create mode 100644 code/core/src/telemetry/get-has-router-package.ts diff --git a/code/core/src/telemetry/get-has-router-package.test.ts b/code/core/src/telemetry/get-has-router-package.test.ts new file mode 100644 index 000000000000..ae54807ae754 --- /dev/null +++ b/code/core/src/telemetry/get-has-router-package.test.ts @@ -0,0 +1,29 @@ +import { expect, it } from 'vitest'; + +import { getHasRouterPackage } from './get-has-router-package'; + +it('returns true if there is a routing package in package.json', async () => { + expect( + getHasRouterPackage({ + dependencies: { + react: '^18', + 'react-dom': '^18', + 'react-router': '^6', + }, + }) + ).toBe(true); +}); + +it('returns false if there is a routing package in package.json dependenices', async () => { + expect( + getHasRouterPackage({ + dependencies: { + react: '^18', + 'react-dom': '^18', + }, + devDependencies: { + 'react-router': '^6', + }, + }) + ).toBe(false); +}); diff --git a/code/core/src/telemetry/get-has-router-package.ts b/code/core/src/telemetry/get-has-router-package.ts new file mode 100644 index 000000000000..5873c3832d25 --- /dev/null +++ b/code/core/src/telemetry/get-has-router-package.ts @@ -0,0 +1,37 @@ +import type { PackageJson } from '../types'; + +const routerPackages = new Set([ + 'react-router', + 'react-router-dom', + 'remix', + '@tanstack/react-router', + 'expo-router', + '@reach/router', + 'react-easy-router', + '@remix-run/router', + 'wouter', + 'wouter-preact', + 'preact-router', + 'vue-router', + 'unplugin-vue-router', + '@angular/router', + '@solidjs/router', + + // metaframeworks that imply routing + 'next', + 'react-scripts', + 'gatsby', + 'nuxt', + '@sveltejs/kit', +]); + +/** + * @param packageJson The package JSON of the project + * @returns Boolean Does this project use a routing package? + */ +export function getHasRouterPackage(packageJson: PackageJson) { + // NOTE: we just check real dependencies; if it is in dev dependencies, it may just be an example + return Object.keys(packageJson?.dependencies ?? {}).some((depName) => + routerPackages.has(depName) + ); +} diff --git a/code/core/src/telemetry/storybook-metadata.ts b/code/core/src/telemetry/storybook-metadata.ts index 75804b813fc2..95c118aa81e9 100644 --- a/code/core/src/telemetry/storybook-metadata.ts +++ b/code/core/src/telemetry/storybook-metadata.ts @@ -13,6 +13,7 @@ import { findPackage } from 'fd-package-json'; import { getChromaticVersionSpecifier } from './get-chromatic-version'; import { getFrameworkInfo } from './get-framework-info'; +import { getHasRouterPackage } from './get-has-router-package'; import { getMonorepoType } from './get-monorepo-type'; import { getPortableStoriesFileCount } from './get-portable-stories-usage'; import { getActualPackageVersion, getActualPackageVersions } from './package-json'; @@ -100,6 +101,8 @@ export const computeStorybookMetadata = async ({ ) ); + metadata.hasRouterPackage = getHasRouterPackage(packageJson); + const monorepoType = getMonorepoType(); if (monorepoType) { metadata.monorepo = monorepoType; diff --git a/code/core/src/telemetry/types.ts b/code/core/src/telemetry/types.ts index 757f5afc197e..2e0b70a628d0 100644 --- a/code/core/src/telemetry/types.ts +++ b/code/core/src/telemetry/types.ts @@ -59,6 +59,7 @@ export type StorybookMetadata = { version: string; }; testPackages?: Record; + hasRouterPackage?: boolean; hasStorybookEslint?: boolean; hasStaticDirs?: boolean; hasCustomWebpack?: boolean; From 766645f95de9763bef7215b2dfd62a31630ee0e3 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 16 Dec 2024 13:55:01 +1100 Subject: [PATCH 06/15] Add tool to count lines in a command --- .../exec-command-count-lines.test.ts | 71 +++++++++++++++++++ .../src/telemetry/exec-command-count-lines.ts | 33 +++++++++ .../telemetry/get-portable-stories-usage.ts | 11 +-- 3 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 code/core/src/telemetry/exec-command-count-lines.test.ts create mode 100644 code/core/src/telemetry/exec-command-count-lines.ts diff --git a/code/core/src/telemetry/exec-command-count-lines.test.ts b/code/core/src/telemetry/exec-command-count-lines.test.ts new file mode 100644 index 000000000000..eacfe9f72952 --- /dev/null +++ b/code/core/src/telemetry/exec-command-count-lines.test.ts @@ -0,0 +1,71 @@ +import type { Transform } from 'node:stream'; +import { PassThrough } from 'node:stream'; + +import { beforeEach, describe, expect, it, vitest } from 'vitest'; + +// eslint-disable-next-line depend/ban-dependencies +import { execaCommand as rawExecaCommand } from 'execa'; + +import { execCommandCountLines } from './exec-command-count-lines'; + +vitest.mock('execa'); + +const execaCommand = vitest.mocked(rawExecaCommand); +beforeEach(() => { + execaCommand.mockReset(); +}); + +type ExecaStreamer = typeof Promise & { + stdout: Transform; + kill: () => void; +}; + +function createExecaStreamer() { + let resolver: () => void; + const promiseLike: ExecaStreamer = new Promise((aResolver, aRejecter) => { + resolver = aResolver; + }) as any; + + promiseLike.stdout = new PassThrough(); + // @ts-expect-error technically it is invalid to use resolver "before" it is assigned (but not really) + promiseLike.kill = resolver; + return promiseLike; +} + +describe('execCommandCountLines', () => { + it('counts lines, many', async () => { + const streamer = createExecaStreamer(); + execaCommand.mockReturnValue(streamer as any); + + const promise = execCommandCountLines('some command'); + + streamer.stdout.write('First line\n'); + streamer.stdout.write('Second line\n'); + streamer.kill(); + + expect(await promise).toEqual(2); + }); + + it('counts lines, one', async () => { + const streamer = createExecaStreamer(); + execaCommand.mockReturnValue(streamer as any); + + const promise = execCommandCountLines('some command'); + + streamer.stdout.write('First line\n'); + streamer.kill(); + + expect(await promise).toEqual(1); + }); + + it('counts lines, none', async () => { + const streamer = createExecaStreamer(); + execaCommand.mockReturnValue(streamer as any); + + const promise = execCommandCountLines('some command'); + + streamer.kill(); + + expect(await promise).toEqual(0); + }); +}); diff --git a/code/core/src/telemetry/exec-command-count-lines.ts b/code/core/src/telemetry/exec-command-count-lines.ts new file mode 100644 index 000000000000..588583532e5e --- /dev/null +++ b/code/core/src/telemetry/exec-command-count-lines.ts @@ -0,0 +1,33 @@ +import { createInterface } from 'node:readline'; + +// eslint-disable-next-line depend/ban-dependencies +import { execaCommand } from 'execa'; + +/** + * Execute a command in the local terminal and count the lines in the result + * + * @param command The command to execute. + * @param options Execa options + * @returns The number of lines the command returned + */ +export async function execCommandCountLines( + command: string, + options?: Parameters[1] +) { + const process = execaCommand(command, { shell: true, buffer: false, ...options }); + if (!process.stdout) { + // Return null rather than throwing an error + return null; + } + + let lineCount = 0; + const rl = createInterface(process.stdout); + rl.on('line', () => { + lineCount += 1; + }); + + // If the process errors, this will throw + await process; + + return lineCount; +} diff --git a/code/core/src/telemetry/get-portable-stories-usage.ts b/code/core/src/telemetry/get-portable-stories-usage.ts index cd9da7f4f589..e6c0d7ee1696 100644 --- a/code/core/src/telemetry/get-portable-stories-usage.ts +++ b/code/core/src/telemetry/get-portable-stories-usage.ts @@ -1,7 +1,5 @@ -// eslint-disable-next-line depend/ban-dependencies -import { execaCommand } from 'execa'; - import { createFileSystemCache, resolvePathInStorybookCache } from '../common'; +import { execCommandCountLines } from './exec-command-count-lines'; const cache = createFileSystemCache({ basePath: resolvePathInStorybookCache('portable-stories'), @@ -11,12 +9,7 @@ const cache = createFileSystemCache({ export const getPortableStoriesFileCountUncached = async (path?: string) => { const command = `git grep -l composeStor` + (path ? ` -- ${path}` : ''); - const { stdout } = await execaCommand(command, { - cwd: process.cwd(), - shell: true, - }); - - return stdout.split('\n').filter(Boolean).length; + return execCommandCountLines(command); }; const CACHE_KEY = 'portableStories'; From 11e0729d50ff6fe4b070291b536e047f80943d97 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 16 Dec 2024 14:47:02 +1100 Subject: [PATCH 07/15] Added `metadata.applicationFileCount` --- code/core/src/__mocks__/page.ts | 1 + .../src/__mocks__/path/to/Screens/index.jsx | 1 + .../src/telemetry/exec-command-count-lines.ts | 4 +-- .../get-application-file-count.test.ts | 14 ++++++++ .../telemetry/get-application-file-count.ts | 30 ++++++++++++++++ .../telemetry/get-portable-stories-usage.ts | 34 ++++++------------- .../src/telemetry/run-telemetry-operation.ts | 22 ++++++++++++ .../src/telemetry/storybook-metadata.test.ts | 16 +++++++++ code/core/src/telemetry/storybook-metadata.ts | 29 ++++++++++++++-- code/core/src/telemetry/types.ts | 1 + 10 files changed, 124 insertions(+), 28 deletions(-) create mode 100644 code/core/src/__mocks__/page.ts create mode 100644 code/core/src/__mocks__/path/to/Screens/index.jsx create mode 100644 code/core/src/telemetry/get-application-file-count.test.ts create mode 100644 code/core/src/telemetry/get-application-file-count.ts create mode 100644 code/core/src/telemetry/run-telemetry-operation.ts diff --git a/code/core/src/__mocks__/page.ts b/code/core/src/__mocks__/page.ts new file mode 100644 index 000000000000..fb87bbd306b8 --- /dev/null +++ b/code/core/src/__mocks__/page.ts @@ -0,0 +1 @@ +// empty file only matched on path diff --git a/code/core/src/__mocks__/path/to/Screens/index.jsx b/code/core/src/__mocks__/path/to/Screens/index.jsx new file mode 100644 index 000000000000..fb87bbd306b8 --- /dev/null +++ b/code/core/src/__mocks__/path/to/Screens/index.jsx @@ -0,0 +1 @@ +// empty file only matched on path diff --git a/code/core/src/telemetry/exec-command-count-lines.ts b/code/core/src/telemetry/exec-command-count-lines.ts index 588583532e5e..15643fd355de 100644 --- a/code/core/src/telemetry/exec-command-count-lines.ts +++ b/code/core/src/telemetry/exec-command-count-lines.ts @@ -16,8 +16,8 @@ export async function execCommandCountLines( ) { const process = execaCommand(command, { shell: true, buffer: false, ...options }); if (!process.stdout) { - // Return null rather than throwing an error - return null; + // Return undefined rather than throwing an error + return undefined; } let lineCount = 0; diff --git a/code/core/src/telemetry/get-application-file-count.test.ts b/code/core/src/telemetry/get-application-file-count.test.ts new file mode 100644 index 000000000000..7fc570689147 --- /dev/null +++ b/code/core/src/telemetry/get-application-file-count.test.ts @@ -0,0 +1,14 @@ +import { join } from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import { getApplicationFilesCountUncached } from './get-application-file-count'; + +const mocksDir = join(__dirname, '..', '__mocks__'); + +describe('getApplicationFilesCount', () => { + it('should find files with correct names', async () => { + const files = await getApplicationFilesCountUncached(mocksDir); + expect(files).toMatchInlineSnapshot(`2`); + }); +}); diff --git a/code/core/src/telemetry/get-application-file-count.ts b/code/core/src/telemetry/get-application-file-count.ts new file mode 100644 index 000000000000..51d5efc05ba3 --- /dev/null +++ b/code/core/src/telemetry/get-application-file-count.ts @@ -0,0 +1,30 @@ +import { execCommandCountLines } from './exec-command-count-lines'; +import { runTelemetryOperation } from './run-telemetry-operation'; + +// We are looking for files with the word "page" or "screen" somewhere in them with these exts +const nameMatches = ['page', 'screen']; +const extensions = ['js', 'jsx', 'ts', 'tsx']; + +export const getApplicationFilesCountUncached = async (basePath: string) => { + const bothCasesNameMatches = nameMatches.flatMap((match) => [ + match, + [match[0].toUpperCase(), ...match.slice(1)].join(''), + ]); + + const globs = bothCasesNameMatches.flatMap((match) => + extensions.map((extension) => `"${basePath}*${match}*.${extension}"`) + ); + + try { + const command = `git ls-files -- ${globs.join(' ')}`; + return await execCommandCountLines(command); + } catch { + return undefined; + } +}; + +export const getApplicationFileCount = async (path: string) => { + return runTelemetryOperation('applicationFiles', async () => + getApplicationFilesCountUncached(path) + ); +}; diff --git a/code/core/src/telemetry/get-portable-stories-usage.ts b/code/core/src/telemetry/get-portable-stories-usage.ts index e6c0d7ee1696..0831b484ab69 100644 --- a/code/core/src/telemetry/get-portable-stories-usage.ts +++ b/code/core/src/telemetry/get-portable-stories-usage.ts @@ -1,30 +1,18 @@ -import { createFileSystemCache, resolvePathInStorybookCache } from '../common'; import { execCommandCountLines } from './exec-command-count-lines'; - -const cache = createFileSystemCache({ - basePath: resolvePathInStorybookCache('portable-stories'), - ns: 'storybook', - ttl: 24 * 60 * 60 * 1000, // 24h -}); +import { runTelemetryOperation } from './run-telemetry-operation'; export const getPortableStoriesFileCountUncached = async (path?: string) => { - const command = `git grep -l composeStor` + (path ? ` -- ${path}` : ''); - return execCommandCountLines(command); + try { + const command = `git grep -l composeStor` + (path ? ` -- ${path}` : ''); + return await execCommandCountLines(command); + } catch (err: any) { + // exit code 1 if no matches are found + return err.exitCode === 1 ? 0 : undefined; + } }; -const CACHE_KEY = 'portableStories'; export const getPortableStoriesFileCount = async (path?: string) => { - let cached = await cache.get(CACHE_KEY); - if (!cached) { - try { - const count = await getPortableStoriesFileCountUncached(); - cached = { count }; - await cache.set(CACHE_KEY, cached); - } catch (err: any) { - // exit code 1 if no matches are found - const count = err.exitCode === 1 ? 0 : null; - cached = { count }; - } - } - return cached.count; + return runTelemetryOperation('portableStories', async () => + getPortableStoriesFileCountUncached(path) + ); }; diff --git a/code/core/src/telemetry/run-telemetry-operation.ts b/code/core/src/telemetry/run-telemetry-operation.ts new file mode 100644 index 000000000000..9ae6f23fb2a5 --- /dev/null +++ b/code/core/src/telemetry/run-telemetry-operation.ts @@ -0,0 +1,22 @@ +import { createFileSystemCache, resolvePathInStorybookCache } from '../common'; + +const cache = createFileSystemCache({ + basePath: resolvePathInStorybookCache('telemetry'), + ns: 'storybook', + ttl: 24 * 60 * 60 * 1000, // 24h +}); + +/** + * Run an (expensive) operation, caching the result in a FS cache for 24 hours. + * + * NOTE: if the operation returns `undefined` the value will not be cached. Use this to indicate + * that the operation failed. + */ +export const runTelemetryOperation = async (cacheKey: string, operation: () => Promise) => { + let cached = await cache.get(cacheKey); + if (cached === undefined) { + cached = await operation(); + await cache.set(cacheKey, cached); + } + return cached; +}; diff --git a/code/core/src/telemetry/storybook-metadata.test.ts b/code/core/src/telemetry/storybook-metadata.test.ts index 06bf355c0688..8d73ff7b437d 100644 --- a/code/core/src/telemetry/storybook-metadata.test.ts +++ b/code/core/src/telemetry/storybook-metadata.test.ts @@ -12,6 +12,8 @@ const packageJsonMock: PackageJson = { version: 'x.x.x', }; +const packageJsonPath = process.cwd(); + const mainJsMock: StorybookConfig = { stories: [], }; @@ -126,6 +128,7 @@ describe('storybook-metadata', () => { it('should parse pnp paths for known frameworks', async () => { const unixResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -144,6 +147,7 @@ describe('storybook-metadata', () => { const windowsResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -164,6 +168,7 @@ describe('storybook-metadata', () => { it('should parse pnp paths for unknown frameworks', async () => { const unixResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -178,6 +183,7 @@ describe('storybook-metadata', () => { const windowsResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -198,6 +204,7 @@ describe('storybook-metadata', () => { const unixResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -215,6 +222,7 @@ describe('storybook-metadata', () => { cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue('C:\\Users\\foo\\my-project'); const windowsResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -232,6 +240,7 @@ describe('storybook-metadata', () => { it('should return frameworkOptions from mainjs', async () => { const reactResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -250,6 +259,7 @@ describe('storybook-metadata', () => { const angularResult = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: { @@ -279,6 +289,7 @@ describe('storybook-metadata', () => { 'storybook-addon-deprecated': 'x.x.z', }, } as PackageJson, + packageJsonPath, mainConfig: { ...mainJsMock, addons: [ @@ -319,6 +330,7 @@ describe('storybook-metadata', () => { const result = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, features, @@ -332,6 +344,7 @@ describe('storybook-metadata', () => { expect( await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, framework: '@storybook/react-vite', @@ -347,6 +360,7 @@ describe('storybook-metadata', () => { it('should return the number of refs', async () => { const res = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, refs: { @@ -361,6 +375,7 @@ describe('storybook-metadata', () => { it('only reports addon options for addon-essentials', async () => { const res = await computeStorybookMetadata({ packageJson: packageJsonMock, + packageJsonPath, mainConfig: { ...mainJsMock, addons: [ @@ -395,6 +410,7 @@ describe('storybook-metadata', () => { [metaFramework]: 'x.x.x', }, } as PackageJson, + packageJsonPath, mainConfig: mainJsMock, }); expect(res.metaFramework).toEqual({ diff --git a/code/core/src/telemetry/storybook-metadata.ts b/code/core/src/telemetry/storybook-metadata.ts index 95c118aa81e9..5258eef0ffb7 100644 --- a/code/core/src/telemetry/storybook-metadata.ts +++ b/code/core/src/telemetry/storybook-metadata.ts @@ -1,3 +1,5 @@ +import { dirname } from 'node:path'; + import { getProjectRoot, getStorybookConfiguration, @@ -9,8 +11,9 @@ import type { PackageJson, StorybookConfig } from '@storybook/core/types'; import { readConfig } from '@storybook/core/csf-tools'; import { detect, getNpmVersion } from 'detect-package-manager'; -import { findPackage } from 'fd-package-json'; +import { findPackage, findPackagePath } from 'fd-package-json'; +import { getApplicationFileCount } from './get-application-file-count'; import { getChromaticVersionSpecifier } from './get-chromatic-version'; import { getFrameworkInfo } from './get-framework-info'; import { getHasRouterPackage } from './get-has-router-package'; @@ -42,9 +45,11 @@ export const sanitizeAddonName = (name: string) => { // Analyze a combination of information from main.js and package.json // to provide telemetry over a Storybook project export const computeStorybookMetadata = async ({ + packageJsonPath, packageJson, mainConfig, }: { + packageJsonPath: string; packageJson: PackageJson; mainConfig: StorybookConfig & Record; }): Promise => { @@ -212,11 +217,13 @@ export const computeStorybookMetadata = async ({ const storybookVersion = storybookPackages[storybookInfo.frameworkPackage]?.version; const portableStoriesFileCount = await getPortableStoriesFileCount(); + const applicationFileCount = await getApplicationFileCount(dirname(packageJsonPath)); return { ...metadata, ...frameworkInfo, portableStoriesFileCount, + applicationFileCount, storybookVersion, storybookVersionSpecifier: storybookInfo.version, language, @@ -226,13 +233,29 @@ export const computeStorybookMetadata = async ({ }; }; +async function getPackageJsonDetails() { + const packageJsonPath = await findPackagePath(process.cwd()); + if (packageJsonPath) { + return { + packageJsonPath, + packageJson: (await findPackage(packageJsonPath)) || {}, + }; + } + + // If we don't find a `package.json`, we assume it "would have" been in the current working directory + return { + packageJsonPath: process.cwd(), + packageJson: {}, + }; +} + let cachedMetadata: StorybookMetadata; export const getStorybookMetadata = async (_configDir?: string) => { if (cachedMetadata) { return cachedMetadata; } - const packageJson = (await findPackage(process.cwd())) || {}; + const { packageJson, packageJsonPath } = await getPackageJsonDetails(); // TODO: improve the way configDir is extracted, as a "storybook" script might not be present // Scenarios: // 1. user changed it to something else e.g. "storybook:dev" @@ -246,6 +269,6 @@ export const getStorybookMetadata = async (_configDir?: string) => { ) as string)) ?? '.storybook'; const mainConfig = await loadMainConfig({ configDir }); - cachedMetadata = await computeStorybookMetadata({ mainConfig, packageJson }); + cachedMetadata = await computeStorybookMetadata({ mainConfig, packageJson, packageJsonPath }); return cachedMetadata; }; diff --git a/code/core/src/telemetry/types.ts b/code/core/src/telemetry/types.ts index 2e0b70a628d0..e43373e5e61b 100644 --- a/code/core/src/telemetry/types.ts +++ b/code/core/src/telemetry/types.ts @@ -70,6 +70,7 @@ export type StorybookMetadata = { usesGlobals?: boolean; }; portableStoriesFileCount?: number; + applicationFileCount?: number; }; export interface Payload { From 0b45cb37bdd850cd5555eb3ea51162bfe9dea918 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 16 Dec 2024 16:52:12 +1100 Subject: [PATCH 08/15] We need to put a path seperator or git complains --- code/core/src/telemetry/get-application-file-count.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/code/core/src/telemetry/get-application-file-count.ts b/code/core/src/telemetry/get-application-file-count.ts index 51d5efc05ba3..4f4807ddff00 100644 --- a/code/core/src/telemetry/get-application-file-count.ts +++ b/code/core/src/telemetry/get-application-file-count.ts @@ -1,3 +1,5 @@ +import { sep } from 'node:path'; + import { execCommandCountLines } from './exec-command-count-lines'; import { runTelemetryOperation } from './run-telemetry-operation'; @@ -12,7 +14,7 @@ export const getApplicationFilesCountUncached = async (basePath: string) => { ]); const globs = bothCasesNameMatches.flatMap((match) => - extensions.map((extension) => `"${basePath}*${match}*.${extension}"`) + extensions.map((extension) => `"${basePath}${sep}*${match}*.${extension}"`) ); try { From 7a3e991a2d1a6fac0939434f3ae66403f54fac6d Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Mon, 16 Dec 2024 14:13:24 +0800 Subject: [PATCH 09/15] Experiment: Highlight VTA on first load if it's installed --- code/core/src/manager-api/modules/layout.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/core/src/manager-api/modules/layout.ts b/code/core/src/manager-api/modules/layout.ts index 468d51af1b8a..6687c36b1137 100644 --- a/code/core/src/manager-api/modules/layout.ts +++ b/code/core/src/manager-api/modules/layout.ts @@ -100,7 +100,7 @@ export const defaultLayoutState: SubState = { panelPosition: 'bottom', showTabs: true, }, - selectedPanel: undefined, + selectedPanel: 'chromaui/addon-visual-tests/panel', theme: create(), }; From f94f7d9fc808a7b6c5f5d62d1fe45bb7767793b9 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 16 Dec 2024 22:32:07 +1100 Subject: [PATCH 10/15] Fix some greptile things --- code/core/src/telemetry/exec-command-count-lines.ts | 6 ++++-- code/core/src/telemetry/get-has-router-package.test.ts | 4 ++-- code/core/src/telemetry/run-telemetry-operation.ts | 5 ++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/code/core/src/telemetry/exec-command-count-lines.ts b/code/core/src/telemetry/exec-command-count-lines.ts index 15643fd355de..fdc4547ce464 100644 --- a/code/core/src/telemetry/exec-command-count-lines.ts +++ b/code/core/src/telemetry/exec-command-count-lines.ts @@ -16,8 +16,8 @@ export async function execCommandCountLines( ) { const process = execaCommand(command, { shell: true, buffer: false, ...options }); if (!process.stdout) { - // Return undefined rather than throwing an error - return undefined; + // eslint-disable-next-line local-rules/no-uncategorized-errors + throw new Error('Unexpected missing stdout'); } let lineCount = 0; @@ -29,5 +29,7 @@ export async function execCommandCountLines( // If the process errors, this will throw await process; + rl.close(); + return lineCount; } diff --git a/code/core/src/telemetry/get-has-router-package.test.ts b/code/core/src/telemetry/get-has-router-package.test.ts index ae54807ae754..8504a5bc4d84 100644 --- a/code/core/src/telemetry/get-has-router-package.test.ts +++ b/code/core/src/telemetry/get-has-router-package.test.ts @@ -2,7 +2,7 @@ import { expect, it } from 'vitest'; import { getHasRouterPackage } from './get-has-router-package'; -it('returns true if there is a routing package in package.json', async () => { +it('returns true if there is a routing package in package.json', () => { expect( getHasRouterPackage({ dependencies: { @@ -14,7 +14,7 @@ it('returns true if there is a routing package in package.json', async () => { ).toBe(true); }); -it('returns false if there is a routing package in package.json dependenices', async () => { +it('returns false if there is a routing package in package.json dependencies', () => { expect( getHasRouterPackage({ dependencies: { diff --git a/code/core/src/telemetry/run-telemetry-operation.ts b/code/core/src/telemetry/run-telemetry-operation.ts index 9ae6f23fb2a5..29d2aee6c721 100644 --- a/code/core/src/telemetry/run-telemetry-operation.ts +++ b/code/core/src/telemetry/run-telemetry-operation.ts @@ -16,7 +16,10 @@ export const runTelemetryOperation = async (cacheKey: string, operation: () = let cached = await cache.get(cacheKey); if (cached === undefined) { cached = await operation(); - await cache.set(cacheKey, cached); + // Undefined indicates an error, setting isn't really valuable. + if (cached !== undefined) { + await cache.set(cacheKey, cached); + } } return cached; }; From d09ea61c100d32adbda9aecf10f908c499a93f27 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Mon, 16 Dec 2024 12:52:32 +0100 Subject: [PATCH 11/15] Add conditional rendering for a11yNotPassedAmount based on story entry --- code/addons/test/src/components/TestProviderRender.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/code/addons/test/src/components/TestProviderRender.tsx b/code/addons/test/src/components/TestProviderRender.tsx index 0c1185ab6f2a..1ea623ce7378 100644 --- a/code/addons/test/src/components/TestProviderRender.tsx +++ b/code/addons/test/src/components/TestProviderRender.tsx @@ -114,6 +114,8 @@ export const TestProviderRender: FC< state.config || { a11y: false, coverage: false } ); + const isStoryEntry = entryId?.includes('--') ?? false; + const a11yResults = useMemo(() => { if (!isA11yAddon) { return []; @@ -156,7 +158,7 @@ export const TestProviderRender: FC< (result) => result?.status === 'failed' || result?.status === 'warning' ).length; - const storyId = entryId?.includes('--') ? entryId : undefined; + const storyId = isStoryEntry ? entryId : undefined; const results = (state.details?.testResults || []) .flatMap((test) => { if (!entryId) { @@ -340,7 +342,7 @@ export const TestProviderRender: FC< : null } icon={} - right={a11yNotPassedAmount || null} + right={isStoryEntry ? null : a11yNotPassedAmount || null} /> )} From 2307f9f704ee0e847c1dc79072368e907018b945 Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Mon, 16 Dec 2024 20:23:14 +0800 Subject: [PATCH 12/15] E2E Tests: Fix failing test when Controls is not the default tab --- code/e2e-tests/preview-api.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/code/e2e-tests/preview-api.spec.ts b/code/e2e-tests/preview-api.spec.ts index cfabaf89674c..31b566027fd9 100644 --- a/code/e2e-tests/preview-api.spec.ts +++ b/code/e2e-tests/preview-api.spec.ts @@ -65,6 +65,7 @@ test.describe('preview-api', () => { const root = sbPage.previewRoot(); + await sbPage.viewAddonPanel('Controls'); const labelControl = sbPage.page.locator('#control-label'); await expect(root.getByText('Loaded. Click me')).toBeVisible(); From 2a5a8557d1294849593fa7d5bfccc8db478f3d5f Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Mon, 16 Dec 2024 21:13:25 +0800 Subject: [PATCH 13/15] UI: Fix bug where unknown addon panel selection defaults to first tab --- code/core/src/manager/components/panel/Panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/core/src/manager/components/panel/Panel.tsx b/code/core/src/manager/components/panel/Panel.tsx index 701cd94925e3..dc80c20949d2 100644 --- a/code/core/src/manager/components/panel/Panel.tsx +++ b/code/core/src/manager/components/panel/Panel.tsx @@ -60,7 +60,7 @@ export const AddonPanel = React.memo<{ return ( Date: Mon, 16 Dec 2024 14:59:38 +0100 Subject: [PATCH 14/15] Adjust sandbox parts --- .../project.json | 21 +++++++++++++++++++ scripts/tasks/sandbox-parts.ts | 7 +------ 2 files changed, 22 insertions(+), 6 deletions(-) create mode 100644 code/sandbox/experimental-nextjs-vite-default-ts/project.json diff --git a/code/sandbox/experimental-nextjs-vite-default-ts/project.json b/code/sandbox/experimental-nextjs-vite-default-ts/project.json new file mode 100644 index 000000000000..ae9d595865dd --- /dev/null +++ b/code/sandbox/experimental-nextjs-vite-default-ts/project.json @@ -0,0 +1,21 @@ +{ + "name": "experimental-nextjs-vite/default-ts", + "$schema": "../../node_modules/nx/schemas/project-schema.json", + "projectType": "application", + "implicitDependencies": [ + "storybook", + "core", + "addon-essentials", + "addon-interactions", + "addon-links", + "addon-onboarding", + "blocks", + "experimental-nextjs-vite" + ], + "targets": { + "sandbox": {}, + "sb:dev": {}, + "sb:build": {} + }, + "tags": ["ci:normal", "ci:merged", "ci:daily"] +} diff --git a/scripts/tasks/sandbox-parts.ts b/scripts/tasks/sandbox-parts.ts index ea99a566dbef..98f05c95f9d3 100644 --- a/scripts/tasks/sandbox-parts.ts +++ b/scripts/tasks/sandbox-parts.ts @@ -809,12 +809,7 @@ export const extendPreview: Task['run'] = async ({ template, sandboxDir }) => { const previewConfig = await readConfig({ cwd: sandboxDir, fileName: 'preview' }); if (template.expected.builder.includes('vite')) { - previewConfig.setFieldValue(['tags'], ['vitest']); - // TODO: Remove this once the starter components + test stories have proper accessibility - previewConfig.setFieldValue( - ['parameters', 'a11y', 'warnings'], - ['minor', 'moderate', 'serious', 'critical'] - ); + previewConfig.setFieldValue(['tags'], ['vitest', '!a11ytest']); } await writeConfig(previewConfig); From ba9d40224a321eee84b3f9851f7cffce7d2a45ab Mon Sep 17 00:00:00 2001 From: storybook-bot <32066757+storybook-bot@users.noreply.github.com> Date: Mon, 16 Dec 2024 15:02:57 +0000 Subject: [PATCH 15/15] Write changelog for 8.5.0-beta.1 [skip ci] --- CHANGELOG.prerelease.md | 8 ++++++++ code/package.json | 3 ++- docs/versions/next.json | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.prerelease.md b/CHANGELOG.prerelease.md index f8f8d923cd14..e5212d214e18 100644 --- a/CHANGELOG.prerelease.md +++ b/CHANGELOG.prerelease.md @@ -1,3 +1,11 @@ +## 8.5.0-beta.1 + +- Addon A11y: Add conditional rendering for a11y violation number in Testing Module - [#30073](https://github.com/storybookjs/storybook/pull/30073), thanks @valentinpalkovic! +- Addon A11y: Remove warnings API - [#30049](https://github.com/storybookjs/storybook/pull/30049), thanks @kasperpeulen! +- Addon A11y: Show errors of axe properly - [#30050](https://github.com/storybookjs/storybook/pull/30050), thanks @kasperpeulen! +- Addon Test: Fix printing null% for coverage - [#30061](https://github.com/storybookjs/storybook/pull/30061), thanks @ghengeveld! +- Telemetry: Add metadata distinguishing "apps" from "design systems" - [#30070](https://github.com/storybookjs/storybook/pull/30070), thanks @tmeasday! + ## 8.5.0-beta.0 - Automigration: Improve setup file transformation and version range handling for a11y migration - [#30060](https://github.com/storybookjs/storybook/pull/30060), thanks @valentinpalkovic! diff --git a/code/package.json b/code/package.json index c3e23d70bb46..3c204e8e0572 100644 --- a/code/package.json +++ b/code/package.json @@ -294,5 +294,6 @@ "Dependency Upgrades" ] ] - } + }, + "deferredNextVersion": "8.5.0-beta.1" } diff --git a/docs/versions/next.json b/docs/versions/next.json index 24786dc7c726..ac38904a3bb2 100644 --- a/docs/versions/next.json +++ b/docs/versions/next.json @@ -1 +1 @@ -{"version":"8.5.0-beta.0","info":{"plain":"- Automigration: Improve setup file transformation and version range handling for a11y migration - [#30060](https://github.com/storybookjs/storybook/pull/30060), thanks @valentinpalkovic!\n- Next.js: Support v15.1.1 - [#30068](https://github.com/storybookjs/storybook/pull/30068), thanks @valentinpalkovic!"}} +{"version":"8.5.0-beta.1","info":{"plain":"- Addon A11y: Add conditional rendering for a11y violation number in Testing Module - [#30073](https://github.com/storybookjs/storybook/pull/30073), thanks @valentinpalkovic!\n- Addon A11y: Remove warnings API - [#30049](https://github.com/storybookjs/storybook/pull/30049), thanks @kasperpeulen!\n- Addon A11y: Show errors of axe properly - [#30050](https://github.com/storybookjs/storybook/pull/30050), thanks @kasperpeulen!\n- Addon Test: Fix printing null% for coverage - [#30061](https://github.com/storybookjs/storybook/pull/30061), thanks @ghengeveld!\n- Telemetry: Add metadata distinguishing \\\"apps\\\" from \\\"design systems\\\" - [#30070](https://github.com/storybookjs/storybook/pull/30070), thanks @tmeasday!"}}