Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Storybook to 8.x (next) #2373

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Upgrade Storybook to 8.x (next) #2373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
83cc551
Upgrade Storybook to latest version (next)
jandrade Nov 27, 2024
d4991cf
Rever package.json change
jandrade Nov 27, 2024
f63d521
Remove unsused dep, fix lint issues
jandrade Nov 27, 2024
6d82915
More lint fixes
jandrade Nov 27, 2024
91ed3aa
Revert some TS changes
jandrade Nov 27, 2024
508d892
Upgrade SB eslint plugin
jandrade Nov 27, 2024
f111d08
Add storybook test support in CI
jandrade Nov 27, 2024
e0a5132
Reverting back ESLint SB version, fix SB tests tooling for CI
jandrade Nov 27, 2024
c4fa248
Pre-install Playwright in CI
jandrade Nov 27, 2024
6ad522e
Fix some Storybook interaction tests
jandrade Nov 27, 2024
93279fa
Export decorators properly in preview.tsx
jandrade Nov 28, 2024
3f3d310
Final fixes
jandrade Nov 28, 2024
8875ff2
Update TypeScript to 5.7.2 and modify tsconfig-common.json ot use 'bu…
kevinb-khan Nov 28, 2024
7060439
Change chromatic GH step to be explicit about the testing part
jandrade Nov 28, 2024
a136065
Fix merge conflicts
jandrade Dec 11, 2024
9d18c59
Merge remote-tracking branch 'origin' into sb-85
jandrade Dec 12, 2024
0393fdb
Remove react v16 alias in vite config
jandrade Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/chromatic-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ jobs:
# Generate snapshots for only changed stories
# See: https://www.chromatic.com/docs/turbosnap
onlyChanged: true
# Install Playwright browsers so Vitest browser mode can run story tests
- name: Install playwright dependencies
if: ${{ inputs.target == 'review' }}
run: yarn exec playwright install chromium --with-deps
- name: Run Storybook tests
if: ${{ inputs.target == 'review' }}
run: yarn test:storybook
env:
SB_URL: '${{ steps.chromatic_publish.outputs.storybookUrl }}'

# (if) Run this step on dependabot or changesets PRs.
- name: Skip Chromatic builds (dependabot or changesets PRs)
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/chromatic-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
if: |
github.actor != 'dependabot[bot]' && github.actor != 'dependabot-preview[bot]' &&
!startsWith(github.head_ref, 'changeset-release/')
name: Chromatic - Build on regular PRs
name: Chromatic - Build and test on regular PRs
uses: ./.github/workflows/chromatic-build.yml
with:
target: 'review'
Expand Down
50 changes: 6 additions & 44 deletions .storybook/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
import {resolve, dirname, join} from "path";
import {mergeConfig} from "vite";
import turbosnap from "vite-plugin-turbosnap";
import type {StorybookConfig} from "@storybook/react-vite";

const config: StorybookConfig = {
Expand All @@ -9,52 +6,17 @@ const config: StorybookConfig = {
"../__docs__/**/*.mdx",
],
addons: [
getAbsolutePath("@storybook/addon-essentials"),
getAbsolutePath("@storybook/addon-a11y"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storybook upgrades always want to drop this back in. I finally searched when it's needed. Apparently in Yarn PnP situations module resolution can fail without this. Good to know we can remove this in Perseus too then.

https://storybook.js.org/docs/faq#how-do-i-fix-module-resolution-in-special-environments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, thanks for the context.

getAbsolutePath("@storybook/addon-designs"),
getAbsolutePath("@storybook/addon-interactions"),
getAbsolutePath("@storybook/addon-mdx-gfm"),
getAbsolutePath("storybook-addon-pseudo-states"),
"@storybook/addon-essentials",
"@storybook/addon-a11y",
"@storybook/addon-designs",
"storybook-addon-pseudo-states",
"@storybook/experimental-addon-test",
],
staticDirs: ["../static"],
core: {
disableTelemetry: true,
},
framework: getAbsolutePath("@storybook/react-vite"),
async viteFinal(config, {configType}) {
// Merge custom configuration into the default config
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I'm moving this to a separate vite.config.ts file for better integration with vitest.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y'all are switching to vitest?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it's just for running storybook tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct! I'm thinking that we should probably write an ADR if we want to adopt this more widely.

Just curious.... I know this is a big change, but I wonder if you have any opinions about switching from Jest to vitest for unit tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest is definitely a more well-trodden path as it's been around for longer. But Vitest is a solid testing framework! I was able to do everything I needed with Vitest and Testing Library. It's supposedly faster and easier to configure than Jest, too.

const mergedConfig = mergeConfig(config, {
resolve: {
// Allow us to detect changes from local wonder-blocks packages.
alias: [
{
find: /^@khanacademy\/wonder-blocks(-.*)$/,
replacement: resolve(
__dirname,
"../packages/wonder-blocks$1/src",
),
},
],
},
});

// Add turbosnap to production builds so we can let Chromatic take
// snapshots only to stories associated with the current PR.
if (configType === "PRODUCTION") {
config.plugins?.push(
turbosnap({rootDir: config.root || process.cwd()}),
);
}

Comment on lines -43 to -47
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Dropping this as it is no longer needed as a separate package. SB 8 now includes it internally.

return mergedConfig;
},
docs: {
autodocs: true,
},
framework: "@storybook/react-vite",
};

export default config;

function getAbsolutePath(value: string): any {
return dirname(require.resolve(join(value, "package.json")));
}
43 changes: 21 additions & 22 deletions .storybook/preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from "react";
import wonderBlocksTheme from "./wonder-blocks-theme";

import {Decorator} from "@storybook/react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally out of the blue and unrelated to this PR, but it looks like you don't have import/order enabled in Wonder Blocks. I (not sure about others Perseus) really love how it automatically orders imports so I don't think about it... although, maybe you don't think about it either. :D

https://github.com/Khan/perseus/blob/main/.eslintrc.js#L220..L236

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I'll add this rule in a separate PR. I'll probably wait until WB is in a more "quiet" state to apply the changes :)

import {color} from "@khanacademy/wonder-blocks-tokens";
import Link from "@khanacademy/wonder-blocks-link";
import {ThemeSwitcherContext} from "@khanacademy/wonder-blocks-theming";
Expand Down Expand Up @@ -95,32 +95,29 @@ const parameters = {
},
};

const decorators = [
(Story, context) => {
const theme = context.globals.theme;
const enableRenderStateRootDecorator =
context.parameters.enableRenderStateRootDecorator;

if (enableRenderStateRootDecorator) {
return (
<RenderStateRoot>
<ThemeSwitcherContext.Provider value={theme}>
<Story />
</ThemeSwitcherContext.Provider>
</RenderStateRoot>
);
}
const withThemeSwitcher: Decorator = (
Story,
{globals: {theme}, parameters: {enableRenderStateRootDecorator}},
) => {
if (enableRenderStateRootDecorator) {
return (
<ThemeSwitcherContext.Provider value={theme}>
<Story />
</ThemeSwitcherContext.Provider>
<RenderStateRoot>
<ThemeSwitcherContext.Provider value={theme}>
<Story />
</ThemeSwitcherContext.Provider>
</RenderStateRoot>
);
},
];
}
return (
<ThemeSwitcherContext.Provider value={theme}>
<Story />
</ThemeSwitcherContext.Provider>
);
};

const preview: Preview = {
parameters,
decorators,
decorators: [withThemeSwitcher],
globalTypes: {
// Allow the user to select a theme from the toolbar.
theme: {
Expand All @@ -147,6 +144,8 @@ const preview: Preview = {
},
},
},

tags: ["autodocs"],
};

export default preview;
9 changes: 9 additions & 0 deletions .storybook/vitest.setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { beforeAll } from 'vitest';
import { setProjectAnnotations } from '@storybook/react';
import * as projectAnnotations from './preview';

// This is an important step to apply the right configuration when testing your stories.
// More info at: https://storybook.js.org/docs/api/portable-stories/portable-stories-vitest#setprojectannotations
const project = setProjectAnnotations([projectAnnotations]);

beforeAll(project.beforeAll);
2 changes: 1 addition & 1 deletion __docs__/wonder-blocks-dropdown/combobox.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ export const ControlledMultilpleCombobox: Story = {
],

play: async ({canvasElement}) => {
const canvas = within(canvasElement);
const canvas = within(canvasElement.ownerDocument.body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, this differs from the examples for these play() functions. :( They always show how you originally had it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yeah, this is useful for when we are using portals. I should add a comment here stating that.


// Move to second option item
await userEvent.keyboard("{ArrowDown}");
Expand Down
72 changes: 38 additions & 34 deletions __docs__/wonder-blocks-switch/switch.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,46 +82,50 @@ export const Default: StoryComponentType = {
* The `onChange` prop is optional in case the toggle will
* be wrapped in a larger clickable component.
*/
export const Controlled: StoryComponentType = () => {
const [checkedOne, setCheckedOne] = React.useState(false);
const [checkedTwo, setCheckedTwo] = React.useState(false);
export const Controlled: StoryComponentType = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This component doesn't have any big changes, just moved the code around to support CSF3.

render: function Render() {
const [checkedOne, setCheckedOne] = React.useState(false);
const [checkedTwo, setCheckedTwo] = React.useState(false);

return (
<View style={styles.column}>
<Switch checked={checkedOne} onChange={setCheckedOne} />

<Switch
testId="test-switch"
aria-label="test switch"
checked={checkedTwo}
onChange={setCheckedTwo}
icon={<PhosphorIcon icon={magnifyingGlassIcon} />}
/>
</View>
);
};

Controlled.play = async ({canvasElement}) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In previous versions of Storybook you had to manually navigate to each story and press "play" to run the tests, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! and that made it hard to determine when we introduced a regression with these interaction tests. I also added the CI step to ensure that we don't introduce issues in the future.

NOTE: Storybook uses the test-runner for this, but it will be replaced by this new test addon.

The previous runner used Jest + Playwright, and they wanted to switch to Vitest specially because they can run these tests in browsers as well as in headless mode. This is a new vitest feature that is also being refined in collaboration between the Storybook and vitest teams: https://vitest.dev/guide/browser/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context here: https://storybook.js.org/blog/storybook-test-sneak-peek/

const canvas = within(canvasElement);
return (
<View style={styles.column}>
<Switch checked={checkedOne} onChange={setCheckedOne} />
<Switch
testId="test-switch"
aria-label="test switch"
checked={checkedTwo}
onChange={setCheckedTwo}
icon={<PhosphorIcon icon={magnifyingGlassIcon} />}
/>
</View>
);
},
play: async ({canvasElement}) => {
const canvas = within(canvasElement);

const switchWithIcon = canvas.getByTestId("test-switch");
const switchInput = canvas.getByRole("switch", {name: "test switch"});
const switchWithIcon = canvas.getByTestId("test-switch");
const switchInput = canvas.getByRole("switch", {name: "test switch"});

await userEvent.tab();
await userEvent.tab();
await userEvent.tab();
await userEvent.tab();

expect(switchWithIcon).toHaveStyle(
"background-color: rgba(33, 36, 44, 0.5)",
);
expect(switchWithIcon).toHaveStyle("outline: 2px solid rgb(24, 101, 242)");
expect(switchInput).toHaveProperty("checked", false);
expect(switchWithIcon).toHaveStyle(
"background-color: rgba(33, 36, 44, 0.5)",
);
expect(switchWithIcon).toHaveStyle(
"outline: 2px solid rgb(24, 101, 242)",
);
expect(switchInput).toHaveProperty("checked", false);

await userEvent.click(switchWithIcon);
// Wait for animations to finish
await new Promise((resolve) => setTimeout(resolve, 150));
await userEvent.click(switchWithIcon);
// Wait for animations to finish
await new Promise((resolve) => setTimeout(resolve, 150));

expect(switchInput).toHaveProperty("checked", true);
expect(switchWithIcon).toHaveStyle("background-color: rgb(24, 101, 242)");
expect(switchInput).toHaveProperty("checked", true);
expect(switchWithIcon).toHaveStyle(
"background-color: rgb(24, 101, 242)",
);
},
};

/**
Expand Down
34 changes: 17 additions & 17 deletions __docs__/wonder-blocks-tooltip/tooltip.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,23 +375,6 @@ export const AutoUpdate: StoryComponentType = {
} | null>(null);
return (
<View style={[styles.centered, styles.row, {position: "relative"}]}>
<Tooltip
content="This is a tooltip that auto-updates its position when the trigger element changes."
opened={true}
autoUpdate={true}
>
<View
style={[
position && {
position: "absolute",
top: position.y,
left: position.x,
},
]}
>
Trigger element
</View>
</Tooltip>
Comment on lines -378 to -394
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was moved to be included after the buttons. See my previous comment here: https://github.com/Khan/wonder-blocks/pull/2373/files#r1862635270

<Button
onClick={() => {
setPosition({
Expand All @@ -413,6 +396,23 @@ export const AutoUpdate: StoryComponentType = {
>
Click to update trigger position (fixed)
</Button>
<Tooltip
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Moved this instance to the right so it can work more consistently in different environments (browser and JSDOM).

The issue with the previous story was that the assertion in the play function was not working properly in headless browsers (e.g. running npx vitest) because the viewport/document size was different from the browser one.

This was causing that the Tooltip was not positioned horizontally, causing the test to fail in CI. With this change, we can now ensure that there will be a horizontal reposition.

content="This is a tooltip that auto-updates its position when the trigger element changes."
opened={true}
autoUpdate={true}
>
<View
style={[
position && {
position: "absolute",
top: position.y,
left: position.x,
},
]}
>
Trigger element
</View>
</Tooltip>
</View>
);
},
Expand Down
31 changes: 17 additions & 14 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"start:storybook": "storybook dev -p 6061",
"test:common": "yarn run build && yarn run lint && yarn run typecheck && yarn run alex",
"test:coverage": "yarn run test:common && yarn run jest --coverage",
"test:storybook": "vitest --project=storybook",
"test": "yarn run jest && yarn run test:common",
"typecheck": "tsc",
"typewatch": "tsc --noEmit --watch --incremental",
Expand All @@ -52,17 +53,16 @@
"@khanacademy/eslint-plugin": "^2.0.0",
"@khanacademy/wonder-stuff-testing": "^3.0.1",
"@rollup/plugin-node-resolve": "^15.0.2",
"@storybook/addon-a11y": "^8.2.1",
"@storybook/addon-actions": "^8.2.1",
"@storybook/addon-designs": "^8.0.3",
"@storybook/addon-docs": "^8.2.1",
"@storybook/addon-essentials": "^8.2.1",
"@storybook/addon-interactions": "^8.2.1",
"@storybook/addon-mdx-gfm": "^8.2.1",
"@storybook/preview-api": "^8.2.1",
"@storybook/react": "^8.2.1",
"@storybook/react-vite": "^8.2.1",
"@storybook/test": "^8.2.1",
"@storybook/addon-a11y": "^8.5.0-alpha.10",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-alpha.10 😱

Are you planning to wait till this is released before landing? 😁

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe living on the edge! j/k

I can totally wait until it is officially released :). I mainly wanted to start this PR to be able to unlock their upcoming Accessibility addon (that uses this new testing tooling), so @marcysutton can try it out and give us some feedback, and also give feedback to the Storybook team as well, which she's planning to do :).

"@storybook/addon-actions": "^8.5.0-alpha.10",
"@storybook/addon-designs": "^8.0.4",
"@storybook/addon-docs": "^8.5.0-alpha.10",
"@storybook/addon-essentials": "^8.5.0-alpha.10",
"@storybook/experimental-addon-test": "^8.5.0-alpha.10",
"@storybook/preview-api": "^8.5.0-alpha.10",
"@storybook/react": "^8.5.0-alpha.10",
"@storybook/react-vite": "^8.5.0-alpha.10",
"@storybook/test": "^8.5.0-alpha.10",
"@swc-node/register": "^1.6.5",
"@swc/core": "^1.3.36",
"@testing-library/jest-dom": "^6.5.0",
Expand All @@ -80,6 +80,8 @@
"@types/react-window": "^1.8.5",
"@typescript-eslint/eslint-plugin": "^5.59.5",
"@typescript-eslint/parser": "^5.59.5",
"@vitejs/plugin-react": "^4.3.3",
"@vitest/browser": "^2.1.5",
"alex": "^11.0.0",
"babel-jest": "^29.7.0",
"babel-loader": "^8.2.3",
Expand Down Expand Up @@ -109,17 +111,18 @@
"jest-extended": "^4.0.2",
"jscodeshift": "^0.15.1",
"npm-package-json-lint": "^6.4.0",
"playwright": "^1.49.0",
"prettier": "^2.8.1",
"react-refresh": "^0.14.0",
"rollup": "^2.79.1",
"rollup-plugin-babel": "^4.4.0",
"rollup-plugin-node-externals": "^7.1.2",
"storybook": "^8.2.1",
"storybook": "^8.5.0-alpha.10",
"storybook-addon-pseudo-states": "^3.1.1",
"typescript": "^4.9.5",
"typescript": "5.7.2",
"typescript-coverage-report": "^0.7.0",
"vite": "^4.4.8",
"vite-plugin-turbosnap": "^1.0.2"
"vitest": "^2.1.5"
},
"dependencies": {
"@babel/runtime": "^7.18.6",
Expand Down
2 changes: 1 addition & 1 deletion tsconfig-common.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// Required for dynamic imports even though we aren't using
// tsc to output any modules.
"module": "ESNext",
"moduleResolution": "node",
"moduleResolution": "bundler",
"resolveJsonModule": true,
/* Interop Constraints */
"esModuleInterop": true,
Expand Down
Loading
Loading