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.

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

chore(story📗): extract MSW logic into a loader #4919

Merged
merged 1 commit into from
May 2, 2022

Conversation

virtuoushub
Copy link
Collaborator

@virtuoushub virtuoushub commented Mar 25, 2022

the loaders are async and execute before the story is rendered.


Why this matters:

interaction testing

Because using a loader happens before a story is rendered, we do not have to worry about the mock data being unavailable to a component. With the previous approach, we possibly had to wait for a couple render cycles in order to guarantee the mock data was available to the component.

@netlify
Copy link

netlify bot commented Mar 25, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit bc89956
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/626fe461150ff3000876d655

@thedavidprice thedavidprice added the release:chore This PR is a chore (means nothing for users) label Mar 25, 2022
@virtuoushub virtuoushub force-pushed the chore/use-storybook-loaders branch from 50c2ea3 to ff4f832 Compare March 25, 2022 03:33
@jtoar
Copy link
Contributor

jtoar commented Apr 5, 2022

@virtuoushub awesome!

@virtuoushub virtuoushub marked this pull request as ready for review April 5, 2022 15:46
@thedavidprice
Copy link
Contributor

@virtuoushub are you waiting on us for this one? Sorry, wasn't clear if this was ready to go or not.

If so, should we loop in anyone else for review?

Thanks!

@virtuoushub
Copy link
Collaborator Author

@virtuoushub are you waiting on us for this one? Sorry, wasn't clear if this was ready to go or not.

If so, should we loop in anyone else for review?

Thanks!

@thedavidprice in theory it is ready to go.

That being said, I do want to start getting some of this stuff unit tested; so any recommendations on that front are greatly appreciated.

@virtuoushub virtuoushub force-pushed the chore/use-storybook-loaders branch from ff4f832 to 88c5517 Compare April 9, 2022 14:08
@virtuoushub virtuoushub mentioned this pull request Apr 9, 2022
1 task
@virtuoushub virtuoushub force-pushed the chore/use-storybook-loaders branch from 88c5517 to f14209d Compare April 16, 2022 15:48
@thedavidprice
Copy link
Contributor

@dac09 You had some ideas about better testing for Storybook — not sure how to implement other than via functional tests using Playwright?

I can definitely run through QA locally via project:sync

@virtuoushub Given that I'm out of my depth here, any thoughts on whether we need to loop in Yann or Michael for a once-over?

@thedavidprice thedavidprice self-requested a review April 18, 2022 17:41
@virtuoushub virtuoushub force-pushed the chore/use-storybook-loaders branch from f14209d to effc2f7 Compare April 20, 2022 00:14
@virtuoushub
Copy link
Collaborator Author

@dac09 You had some ideas about better testing for Storybook — not sure how to implement other than via functional tests using Playwright?

gentle bump to @dac09 to get your thoughts on how we might test this change.

any thoughts on whether we need to loop in Yann or Michael for a once-over?

At this time, I do not think so. A lot of this code came from a direct discussion with Yann.

@jtoar jtoar self-requested a review April 20, 2022 04:08
@dac09
Copy link
Contributor

dac09 commented Apr 20, 2022

gentle bump to @dac09 to get your thoughts on how we might test this change.

So only real way to test something like this is the playwright e2e test as you suggested!

@virtuoushub - I've read through the links and it makes sense to use loaders, but the missing pieces for me are:
a) Why loaders are still experimental? I'm not sure its a good idea for us to switch to this, if it has no additional benefits.
b) What is the benefit of using a loader over the old provider method?

Would apperciate a screen recording, or if you prefer we can connect real time!

@virtuoushub
Copy link
Collaborator Author

a) Why loaders are still experimental?

briefly spoke with the Storybook team on this and there is little risk in adopting loaders even though they are still experimental.

b) What is the benefit of using a loader over the old provider method?

Updated the description, but tl;dr is because loaders happen before render this helps with integration testing.

Would apperciate a screen recording, or if you prefer we can connect real time!

👍🏻

@dac09
Copy link
Contributor

dac09 commented May 2, 2022

Yo sorry @virtuoushub - I pulled in your changes into my branch, and I think codespaes accidentally pushed some of mine into your branch (auto sync maybe?)

experiment with loaders

the loaders are async and execute before the story is rendered

https://storybook.js.org/docs/react/writing-stories/loaders
https://mswjs.io/

Co-authored-by: Yann Braga <[email protected]>
@virtuoushub virtuoushub force-pushed the chore/use-storybook-loaders branch from a7d6877 to bc89956 Compare May 2, 2022 14:02
@virtuoushub
Copy link
Collaborator Author

Yo sorry @virtuoushub - I pulled in your changes into my branch, and I think codespaes accidentally pushed some of mine into your branch (auto sync maybe?)

No problem, it looks like from the testing in #5329 this PR is ready to be merged ( cc @jtoar / @thedavidprice )

@dac09
Copy link
Contributor

dac09 commented May 2, 2022

Yup I pulled in your changes into the PR, so lets wait for it to pass and we should be good to go. Github will probably automatically close!

@dac09 dac09 merged commit f9f934a into redwoodjs:main May 2, 2022
@jtoar jtoar added this to the next-release milestone May 2, 2022
@virtuoushub virtuoushub deleted the chore/use-storybook-loaders branch May 2, 2022 15:10
dac09 added a commit to dac09/redwood that referenced this pull request May 3, 2022
…ok-smoke-test

* 'main' of github.com:redwoodjs/redwood:
  Remove extra checkout in RC workflow (redwoodjs#5414)
  chore(deps): update dependency @azure/msal-browser to v2.24.0 (redwoodjs#5412)
  fix(deps): update react monorepo (redwoodjs#5406)
  cli upgrade: Always search from the start for semvers (redwoodjs#5368)
  codegen graphql schema (redwoodjs#5213)
  fix(deps): update dependency core-js to v3.22.4 (redwoodjs#5409)
  fix(deps): update typescript-eslint monorepo to v5.22.0 (redwoodjs#5410)
  Add graphql-scalars to graphql-server (redwoodjs#5408)
  Update yarn.lock
  v1.2.1
  fix(deps): update dependency cross-undici-fetch to v0.3.6 (redwoodjs#5402)
  fix(deps): update dependency cross-undici-fetch to v0.3.5 (redwoodjs#5398)
  fix(deps): update dependency cross-undici-fetch to v0.3.3 (redwoodjs#5378)
  chore(story📗): extract MSW logic into a loader (redwoodjs#4919)
@jtoar jtoar modified the milestones: next-release, v1.4.0 May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users) topic/storybook
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants