Skip to content

Upgrade Storybook to v7, move builder to Vite#37331

Closed
ryanclark wants to merge 4 commits intomasterfrom
ryan/storybook
Closed

Upgrade Storybook to v7, move builder to Vite#37331
ryanclark wants to merge 4 commits intomasterfrom
ryan/storybook

Conversation

@ryanclark
Copy link
Copy Markdown
Member

This upgrades Storybook to v7 and moves it over to use Vite as a builder instead of Webpack.

I had to remove some usage of window.msw and move it over to using the storybook msw addon, but other than that it was quite painless.

Timings

Builds
Webpack - 21s
Vite - 2.7

To the browser opened & something rendered on screen
Webpack - 23.4s
Vite - 9.1s

It actually fixed some stories too

Before:
image

Now:
image

Note: I had to remove the (unnecessary) XML header from some SVGs as Vite wasn't liking them

@ryanclark ryanclark added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Jan 26, 2024
@github-actions github-actions Bot requested review from avatus and rudream January 26, 2024 12:42
@ryanclark ryanclark mentioned this pull request Jan 26, 2024
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Jan 26, 2024

The first access list story fails for me:

TypeError: Cannot destructure property 'worker' of 'window.msw' as it is undefined.
    at http://localhost:9002/@fs/Users/zmb/src/teleport/e/web/teleport/src/AccessListManagement/AccessLists/AccessLists.story.tsx:18:3

Additionally, not sure if this matters, but I do see this warning:

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

I see some other failures not related to window.msw:

Failed to fetch dynamically imported module: http://localhost:9002/packages/teleport/src/TrustedClusters/TrustedClusters.story.tsx

And another on authndialog-loaded:

[plugin:storybook:react-docgen-plugin] /Users/zmb/src/teleport/web/packages/teleport/src/TrustedClusters/empty.png: Unexpected character '�'. (1:0)

  4 | IHDR�"����bS� IDATx��it#�u�ْlI��n-���$��@�$��
/Users/zmb/src/teleport/web/packages/teleport/src/TrustedClusters/assets.js:1:0
1  |  import emptyPng from './empty.png';
   |  ^
2  |  export { emptyPng };

Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

(My mistake for not checking out the corresponding e branch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like instead of renaming .js stories to .jsx, only .jsx were added.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I mentioned in #34450, msw-storybook-addon has some problems with resetting the handlers between stories when using res.once.

For example, if you open the Access List story and then open "Empty with Igs", which uses res.once, other Access List stories will return 404 from cfg.getAccessManagementListUrl(). The workaround is to refresh the page between each story.

Now, the Access List story doesn't need to use res.once at all, it can just use res instead and the issue is gone. I already fixed it in the teleport.e branch. Other than that, we only have four stories which use res.once, so perhaps for now we can live with this problem. The issue in msw-storybook-addon repo doesn't mention any solutions to this. mswjs/msw-storybook-addon#82

Comment thread package.json
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The stories for DocumentCluster in teleterm fail with "ambiguous import: FileTransferDirection" and I didn't have time to investigate this. Maybe it's a matter of updating this branch with master?

@ravicious ravicious self-requested a review January 29, 2024 14:36
@kimlisa
Copy link
Copy Markdown
Contributor

kimlisa commented Jan 30, 2024

@ryanclark it looks like you're using chrome right? how did you get localhost to work for you?

Comment thread package.json
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@avatus Regarding your question as to what work is left to in this PR, here's what I see:

I just quickly ran git merge master on this branch as it is right now and it doesn't look that scary. There are conflicts in build/package.json, but resolving them should be easy – pick the version from master and then manually bump the versions just as they are currently bumped in this PR. Possibly install new minor/major versions of v7 that were released since this PR was created.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For a sec I thought these changes to svg are not necessary, but Ryan mentioned in the PR description that they're needed.

@avatus avatus mentioned this pull request Jul 10, 2024
@kimlisa
Copy link
Copy Markdown
Contributor

kimlisa commented Jul 23, 2024

closing in favor of #44041

@kimlisa kimlisa closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/lg ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants