Upgrade to storybook v8 by avatus · Pull Request #44041 · gravitational/teleport

avatus · 2024-07-10T22:23:11Z

New branch which brings in the changes from #37331 . It was easier to do it this way over a rebase since the OG pr is a bit out of date

enterprise: https://github.com/gravitational/teleport.e/pull/3256

Closes gravitational/teleport-private#1494

avatus · 2024-07-11T19:43:10Z

I still need to resolve this error (which makes no sense to me yet. its talking about some other name?)

 FAIL  web/packages/teleport/src/components/UserMenuNav/UserMenuNav.test.tsx
  ● Test suite failed to run

    ReferenceError: Response is not defined

      21 |
      22 | import { setupServer } from 'msw/node';
    > 23 | import { http, HttpResponse } from 'msw';

github-actions · 2024-07-11T19:49:15Z

🤖 Vercel preview here: https://docs-1udnodud2-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-07-11T19:50:05Z

🤖 Vercel preview here: https://docs-p8zhhqtag-goteleport.vercel.app/docs/ver/preview

ravicious

Geez, that's a ton of work with those msw changes. 🥵 Thanks a ton for doing this.

ravicious · 2024-07-12T12:51:25Z

          plugin.name !== 'teleport-html-plugin' &&
-          plugin.name !== 'teleport-transform-html-plugin'
+          plugin.name !== 'teleport-transform-html-plugin' &&
+          plugin.name !== 'storybook:react-docgen-plugin'


Could you leave a comment explaining why we need to do this?

We don't have to disable this plugin. It didn't work because Storybook still uses babel for the Vite framework and our babel config wasn't fully configured.
After I added typescript preset to it, the errors gone away (I have it in the patch above).

ravicious · 2024-07-12T13:02:18Z

  msw: {
    handlers: [
-      rest.get(cfg.api.nodesPath, (req, res, ctx) => {
-        return res.once(ctx.json({ items: [] }));


PollingSuccess was not the only story that was supposed to use res.once. AFAIK, at the time of the upgrade to v7, all of the uses of res.once in teleport.e were unnecessary (see https://github.com/gravitational/teleport.e/pull/3256/commits/5d0367d6cceedc71a62f7fe425873820ca6df1c0), but there was a couple of places in OSS that actually do needed it.

Could you go through them on master and add { once: true } where necessary? I found that just going to the web dir and grepping for res.once is good enough.

ravicious · 2024-07-12T13:04:50Z

  coverageReporters: ['text-summary', 'lcov'],
  testPathIgnorePatterns: ['e2e'],
+  testEnvironmentOptions: {
+    customExportConditions: [''],


What is this for?

it had to do with the setupServer from msw but, I never got that working anyway. I'll fiddle around with those tests some more and if this is needed, I'll add a comment

Do we still need it?

yes we do still need it for setupServer in our tests. More info here in migration guide
https://mswjs.io/docs/migrations/1.x-to-2.x/#cannot-find-module-mswnode-jsdom

avatus · 2024-07-12T18:47:13Z

I think I'll need another set of eyes on this msw change. something isn't quite right and requests aren't being intercepted as it seems like they should. I'll return to it on monday

So far, everything seems to match the migration guide. I'm sure it's a small config issue I'm not seeing

gzdunek · 2024-07-15T10:58:59Z

I think we don't need this package.

gzdunek · 2024-07-15T10:59:19Z

Missing new line.

gzdunek · 2024-07-15T11:01:40Z

It works without getAbsolutePath too.

gzdunek · 2024-07-15T12:48:09Z

          plugin.name !== 'teleport-html-plugin' &&
-          plugin.name !== 'teleport-transform-html-plugin'
+          plugin.name !== 'teleport-transform-html-plugin' &&
+          plugin.name !== 'storybook:react-docgen-plugin'


We don't have to disable this plugin. It didn't work because Storybook still uses babel for the Vite framework and our babel config wasn't fully configured.
After I added typescript preset to it, the errors gone away (I have it in the patch above).

avatus · 2024-07-15T17:30:05Z

Applied and patch and removed the extra loaders/initialize callsites. the rest of the stories are much smoother/work more reliably but the SetupConnect/PollingSuccess still breaks horrendously and then any story used after is broke. Diving in more

avatus · 2024-07-15T19:17:20Z

I still need to fix the UserContext.test, i've been trying to follow some docs to get Response defined but hit many many many more test failures while trying. I resorted to just removing msw and using jest to mock these response calls when needed. The last one i need to remove is from UserContext.test but it's eaten half the day so I'll return.

SetupConnect polling success still broke. I'll dive back in tomorrow

kimlisa · 2024-07-16T06:18:33Z

this was required to make the theme switcher work again

kimlisa · 2024-07-16T06:19:08Z

we also have really old files that end in jsx, i put js just in case?

kimlisa · 2024-07-16T06:41:28Z

i clicked through every story and fixed almost everything (EXCEPT teleterm... i couldn't handle it anymore, so i'll leave those to rafal or gz): see abb94e4 and https://github.com/gravitational/teleport.e/pull/3256/commits/ba1271c6faff653308e03292dcaacef918c8a1be

I removed main.story because that's been broken for more than a year
I couldn't fix Player.story.tsx because i think we need to somehow mock websocket
I can NOT figure out why this and whereever else it is used doesn't work... i don't get it and i don't understand what the story is doing 😅 (it doesn't work in master either). i tried putting in required fields but it doesn't work O_o fixed

Here are some things i noticed:

an api path with query params (with a question mark), makes stories break (and it starts breaking other stories). i found this issue which i took it to mean it's not supported (how did it work before)? i worked around this by stripping the affected path anything after the ?
msw doesn't seem to work with components that call api through the teleportContext: see Upgrade to storybook v8 #44041 (comment), workaround is to mock the function through the context
- mocking through context doesn't give us greatest flexibility (like how do we test pending states?), so another alternative is to make change to the code so it doesn't call api from contexts, we can do that later if it doesn't matter too much

kimlisa · 2024-07-16T06:42:49Z

this was required for the styled-component css attribute to work

Hmm, the comment says we don't use reactPlugin, yet it's clearly used here. What don't I understand?

(Also: would you mind putting this statement directly into the code comment?)

I'm not sure why this happens. Looks like babel-plugin-styled-components is not transforming the css prop (but I believe it should). I couldn't find anything useful in the storybook docs and issues, but I will keeping looking.

i don't understand it either, i just removed the comment for now

I gave up on that, I don't know why it doesn't work with the babel plugin alone. I only corrected the mode passed to the react plugin.

ravicious · 2024-07-16T12:47:27Z

I get this when I open http://localhost:9002/?path=/story/teleport-discover-connectmycomputer-setupconnect--polling (which is a completely different story).

[MSW] Uncaught exception in the request handler for "GET http://localhost:9002/packages/teleport/src/Discover/ConnectMyComputer/TestConnection/TestConnection.story.tsx": (bunch of indecipherable stacktrace) This exception has been gracefully handled as a 500 response, however, it's strongly recommended to resolve this error, as it indicates a mistake in your code. If you wish to mock an error response, please see this guide: https://mswjs.io/docs/recipes/mocking-error-responses

This seems like Vite failing when Storybook asks for that file. @gzdunek I believe not importing the general vite config might be a good idea, but it looks like we're missing something still.

ravicious · 2024-07-16T13:07:25Z

an api path with query params (with a question mark), makes stories break (and it starts breaking other stories). i found this issue which i took it to mean it's not supported (how did it work before)? i worked around this by stripping the affected path anything after the ?

It might have been something related to msw 1.x, I vaguely remember getting this error in v7 update but I'm not sure.

Currently, I see this when I open http://localhost:9002/?path=/story/teleporte-accesslists-list--list-unlimited.

[MSW] Found a redundant usage of query parameters in the request handler URL for "GET /v1/webapi/roles?startKey=&search=&limit=". Please match against a path instead and access query parameters using "new URL(request.url).searchParams" instead. Learn more: https://mswjs.io/docs/recipes/query-parameters

It comes from e/web/teleport/src/AccessListManagement/CreateAccessList/CreateAccessList.story.tsx.

I believe we should figure out why it was working, why it got broken, and how we're going to address this. Manually splitting the path in each story might not be the best way long term.

Perhaps it'd be possible to override cfg.api within storybook so that it returns some kind of a proxy object with a custom getter that always strips the query params?

Maybe there's a simpler way to replace the whole cfg.api in storybook with a version that doesn't use query params. We probably don't want to touch teleport/src/config.ts itself that much.

I believe we can setup something that will remove query params. i saw it somewhere in the msw docs, I'll look around again today

It would be nice if the query params weren't in the cfg, it's one of the things blocking the upgrade to react-router 6 😔

Completely out of scope for this PR though of course

avatus · 2024-07-18T20:48:00Z

Removing the query param made most of the stories (SetupConnect especially) work right away. I abstracted it to a function and into a new file that we might be able to add any other utils for storybook to (but i didnt call it utils.ts 😏 )

The next step is finding out how to handle missing web assets?

MSW] Warning: intercepted a request without a matching request handler:

  • GET /packages/teleport/src/AccessRequests/LockedAccessRequests/assets/step1.png?import

If you still wish to intercept this unhandled request, please create a request handler for it.
Read more: https://mswjs.io/docs/getting-started/mocks [devUtils.ts:17:10](http://localhost:9002/@fs/Users/michael/code/gravitational/teleport/node_modules/msw/src/core/utils/internal/devUtils.ts)

althought it isn't really breaking stories for me. I go to the locked access requests story and the images are there and works.

It's actually all running for me quite well now. I'll happily update all the stories that use query param urls if we are ok with this solution. i was hoping that we'd be able to somehow update the handler to do it for us automatically but then i realized that some stories might want access to the unfiltered query params so, i think just doing it manual is fine.

avatus · 2024-07-22T14:36:56Z

I'll rebase this onto the pnpm master today

avatus · 2024-07-22T17:45:38Z

In general, stories seem to work now. stories with msw work and if they don't can be fixed with my proposed withoutQuery util function. I'll update them if we agree on it.

I disabled one test because jest gets rid of a bunch of globals that msw was failing with when using. I added a TODO to fix it, but I don't want one test to block this from being merged.

kimlisa

kimlisa · 2024-07-23T01:56:09Z

documenting here

As mentioned on Slack, it might be better to move that to the patched JSDOM env. If that doesn't work, the file with test setup for Jest would be another good candidate.

the patched JSDOM didn't work, but putting into setup test for jest did

ryanclark · 2024-07-23T08:38:14Z

This shouldn't have been changed

gzdunek · 2024-07-23T10:53:51Z

gzdunek · 2024-07-23T10:58:52Z

We don't need this polyfill.

gzdunek · 2024-07-23T11:03:18Z

This doesn't seem used.

it's used in an external script but perhaps it can be removed from here

gzdunek · 2024-07-23T11:05:59Z

I think we can rollback all changes in this file? We no longer have VITE_NO_PROXY.

gzdunek · 2024-07-23T12:03:32Z

I gave up on that, I don't know why it doesn't work with the babel plugin alone. I only corrected the mode passed to the react plugin.

ravicious · 2024-07-23T12:44:14Z

As mentioned on Slack, it might be better to move that to the patched JSDOM env. If that doesn't work, the file with test setup for Jest would be another good candidate.

ravicious

🚀

ravicious · 2024-07-23T17:04:08Z

I'm not sure if this needs to be here. 🤔

the vite config filein storybook wasn't able to find this package without having this here. i could use relative paths but i wanted to keep the import cleaner

avatus changed the title ~~Upgrade to storybook v7~~ Upgrade to storybook v8 Jul 11, 2024

avatus marked this pull request as ready for review July 11, 2024 18:44

avatus added the no-changelog Indicates that a PR does not require a changelog entry label Jul 11, 2024

avatus requested review from kimlisa and ryanclark July 11, 2024 18:44

github-actions Bot requested a review from rudream July 11, 2024 18:44

github-actions Bot added size/sm ui labels Jul 11, 2024

avatus force-pushed the avatus/storybook branch from fd6b9bf to e06ef18 Compare July 11, 2024 19:43

avatus temporarily deployed to vercel July 11, 2024 19:43 — with GitHub Actions Inactive

avatus force-pushed the avatus/storybook branch from e06ef18 to 59e0c1e Compare July 11, 2024 19:44

avatus temporarily deployed to vercel July 11, 2024 19:44 — with GitHub Actions Inactive

ravicious self-requested a review July 12, 2024 08:17

gzdunek self-requested a review July 12, 2024 09:58

ravicious reviewed Jul 12, 2024

View reviewed changes

Comment thread docs/check-redirects/check-redirects.test.js Outdated

gzdunek reviewed Jul 15, 2024

View reviewed changes

kimlisa reviewed Jul 16, 2024

View reviewed changes

Comment thread package.json Outdated

Copy link
Copy Markdown

Contributor

kimlisa Jul 16, 2024

Choose a reason for hiding this comment

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

this was required to make the theme switcher work again

kimlisa reviewed Jul 16, 2024

View reviewed changes

kimlisa force-pushed the avatus/storybook branch from fa6fb1b to abb94e4 Compare July 16, 2024 06:27

kimlisa reviewed Jul 16, 2024

View reviewed changes

ravicious reviewed Jul 16, 2024

View reviewed changes

avatus force-pushed the avatus/storybook branch from 0ef5261 to 3a7dfa6 Compare July 22, 2024 16:27

avatus requested review from gzdunek and ravicious July 22, 2024 17:43

kimlisa force-pushed the avatus/storybook branch 2 times, most recently from b999365 to 619fdf2 Compare July 23, 2024 01:49

kimlisa approved these changes Jul 23, 2024

View reviewed changes

ryanclark approved these changes Jul 23, 2024

View reviewed changes

gzdunek reviewed Jul 23, 2024

View reviewed changes

ravicious reviewed Jul 23, 2024

View reviewed changes

kimlisa mentioned this pull request Jul 23, 2024

Upgrade Storybook to v7, move builder to Vite #37331

Closed

ravicious approved these changes Jul 23, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from rudream July 23, 2024 16:59

ravicious reviewed Jul 23, 2024

View reviewed changes

avatus force-pushed the avatus/storybook branch from 2c99ea0 to 303e3ac Compare July 23, 2024 17:23

avatus enabled auto-merge July 23, 2024 17:25

avatus force-pushed the avatus/storybook branch 2 times, most recently from f0ac6fa to 43589b1 Compare July 23, 2024 17:58

Upgrade to storybook v8

bc74a0c

avatus force-pushed the avatus/storybook branch from 43589b1 to bc74a0c Compare July 23, 2024 18:06

avatus added this pull request to the merge queue Jul 23, 2024

Merged via the queue into master with commit fc7953f Jul 23, 2024

avatus deleted the avatus/storybook branch July 23, 2024 18:23

avatus mentioned this pull request Jul 24, 2024

Unify the setup and usage of msw in stories #34450

Closed

This was referenced Jul 25, 2024

Fix formatting of mockServiceWorker.js #44620

Merged

Storybook on Chrome fails to load due to HSTS forwarded by Vite proxy from Teleport cluster (ERR_SSL_PROTOCOL_ERROR) #32256

Closed

ravicious mentioned this pull request Aug 29, 2024

Bump msw from 0.47.4 to 1.3.3 #43689

Closed

Conversation

avatus commented Jul 10, 2024 • edited by kimlisa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 11, 2024

Uh oh!

github-actions Bot commented Jul 11, 2024

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avatus commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus commented Jul 15, 2024

Uh oh!

avatus commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

avatus commented Jul 10, 2024 •

edited by kimlisa

Loading

avatus commented Jul 11, 2024 •

edited

Loading

avatus commented Jul 12, 2024 •

edited

Loading

kimlisa commented Jul 16, 2024 •

edited

Loading

avatus commented Jul 18, 2024 •

edited

Loading