Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Storybook V6 upgrade #3728

Merged
merged 52 commits into from
Oct 2, 2020
Merged

Storybook V6 upgrade #3728

merged 52 commits into from
Oct 2, 2020

Conversation

andrewscfc
Copy link
Contributor

@andrewscfc andrewscfc commented Sep 2, 2020 •
edited by RichardPK
Loading

Migrating Storybook from v 5.3.19 to 6.X.X

Resolved issues:

Some minor stuff around:

  • Updated integration of a11y addon
  • Implementation of new manager.js main.js preview.js structure
  • Removed use of addParameters from preview.js, migrated to the new method
  • Importing a storybook.css file to overwrite new padding being applied to the story <body> tag

1. Components aren't re-rendering when navigating around Storybook, or modifying knobs.

[FIXED ✅ ]
6.0.2 & 6.0.21 (with or without forcing @storybook/core-events resolutions).

tl;dr: Fixed by migrating from notes to docs and removing the storybook-readme package. Note that docs is far more 'sophisticated' than notes and is trying to do a lot of dynamic stuff to our readmes, that doesn't work very well. One for the backlog

Knobs and path changes cause an error, and the component does not re-render.
Console error:

TypeError: Cannot read property 'debug' of undefined
    at PostmsgTransport.handleEvent

It turns out this is being caused by our use of addon-notes, which has been deprecated in v6 and replaced by addon-docs

A bit of digging showed that we still had an old instance of @storybook/logger, which was erroring. This was being used by an old instance of @storybook/components, which was being used by... storybook-readme: ^5.0.8

Completely nuking addon-notes & storybook-readme has fixed the problem, now to migrate to addon-docs! Migration docs.

2. Infinite spinner on-load.

[FIXED ✅ ] 6.0.21

tl;dr: This is no longer triggered. 6.0.21 can safely be loaded without an infinite spinner, and without forcing the "@storybook/core-events": "6.0.2", resolution.

3. Components` required props are undefined

[FIXED ✅ ]. 6.0.2 & 6.0.21 (with or without forcing @storybook/core-events resolutions).

tl;dr: 'Args' are now the first argument passed into a story, which means our decorators (i.e. withServiceKnob) were not being interpreted correctly as component props. https://github.com/storybookjs/storybook/blob/master/MIGRATION.md#args-passed-as-first-argument-to-story.

There are two potential fixes for this:

Migrate from

({ service }) => (
      <Component
         service={service}/>

to

( _args, { service }) => (
      <Component
         service={service}/>

Or add passArgsFirst: false, to config.js (which is what I've done)

before:
Screenshot 2020-09-17 at 16 54 34

after:
Screenshot 2020-09-22 at 10 19 11

4. | is no longer a valid story hierarchy separator

[FIXED ✅ ]

tl;dr: replaced | with /

We use this throughout. It needs to be replaced by /.

Impact:

  • Component tree visualisation is gubbed.

Screenshot 2020-09-17 at 15 35 21

5. Arbitrary decorators, and those added after the first story now cause an error

[FIXED ✅ ] 6.0.21

tl;dr: removed any arbitrary addDecorator funcs in the middle of components

i.e. https://github.com/bbc/psammead/blob/2de4871bc90c5be179594b6962ee36037d0f03ff/packages/components/psammead-media-player/src/index.stories.jsx#L130 Although our decoration of stories has tended to be pretty arbitrary, it seems like this component is the only one triggering these errors, so should be a straightforward fix.

Impact:

  • Triggers error You cannot add a decorator after the first story for a kind. Storybook won't render

Future work:

  • Currently best practice, but soon to be enforced. As outlined here
  • Do you use storiesOf to write your stories? Upgrade to Component Story Format (CSF). Or choose MDX if you want more control over your documentation.
  • Do you use config.js to configure Storybook? Main.js declarative configuration, introduced in 5.3, is a huge improvement and the strong recommendation. Upgrade to main.js.
  • Do you use addon-knobs? Storybook Controls is the no-code successor to Knobs that auto-generates the controls based on your component props. Here’s how to migrate your stories to controls.
  • Do you use global parameters or decorators? In 6.0 you should add them declaratively; we’ve deprecatedaddParameters/addDecorator.

Testing notes:

  • Checkout the dependabot-storybook branch locally within Psammead
  • Run storybook npm run storybook
  • Check that there are no issues as you navigate around the components (specifically those above, but keep an eye for any weirdness). Comparing it with https://bbc.github.io/psammead/ will be useful for this
  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@andrewscfc andrewscfc changed the title Dependabot storybook Update storybook dependencies, major version Sep 2, 2020
FK78
FK78 reviewed Sep 2, 2020
View reviewed changes
Copy link
Contributor

@FK78 FK78 left a comment

Choose a reason for hiding this comment

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

I believe these major bumps have been done in this PR: #3729

@karinathomasbbc karinathomasbbc added the dependencies Pull requests that update a dependency file label Sep 2, 2020
@karinathomasbbc karinathomasbbc added this to the Dependency Updates milestone Sep 2, 2020
@andrewscfc
Copy link
Contributor Author

Work completed in #3729

@andrewscfc andrewscfc closed this Sep 2, 2020
@FK78 FK78 deleted the dependabot-storybook branch September 2, 2020 16:01
@andrewscfc andrewscfc restored the dependabot-storybook branch September 3, 2020 13:24
@andrewscfc andrewscfc reopened this Sep 3, 2020
@andrewscfc andrewscfc marked this pull request as draft September 3, 2020 13:49
@andrewscfc
Copy link
Contributor Author

I've re-opened this PR following @rhenshaw56 's revert #3736 ; I am trying to get the upgrade to Storybook 6.0 to work

On latest we experienced an infinite loading wheel with a too much recurision message, I have stopped this by forcing @storybook/core-events to 6.0.21 b5c4bdf

This was necessary because @storybook/addon-notes does not have a storybook 6 compatible verision released; I tried the alpha and that didn't seem to work.

Next Steps

@RichardPK RichardPK self-assigned this Sep 14, 2020
@RichardPK RichardPK added the ws-media The World Service media stream label Sep 16, 2020
@LilyL0u
Copy link
Contributor

LilyL0u commented Sep 25, 2020

There are some padding differences introduced in this upgrade. Link to issue describing the bug bbc/simorgh#7917

@LilyL0u LilyL0u mentioned this pull request Sep 28, 2020
Closed
This was referenced Oct 1, 2020
Closed
Closed
@RichardPK RichardPK merged commit b1cf3e8 into latest Oct 2, 2020
@bbc-news-frameworks bbc-news-frameworks mentioned this pull request Oct 2, 2020
Closed
This was referenced Nov 20, 2020
Merged
Closed
Closed
This was referenced Nov 27, 2020
Closed
Closed
@RichardPK RichardPK deleted the dependabot-storybook branch January 20, 2021 10:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RichardPK RichardPK RichardPK left review comments

@FK78 FK78 FK78 left review comments

@karinathomasbbc karinathomasbbc karinathomasbbc approved these changes

@rebeccamcginn rebeccamcginn rebeccamcginn approved these changes

Assignees

@RichardPK RichardPK

Labels
dependencies Pull requests that update a dependency file ws-media The World Service media stream
Projects
None yet
Milestone
Dependency Updates
Development

Successfully merging this pull request may close these issues.
7 participants
@andrewscfc @RichardPK @LilyL0u @FK78 @karinathomasbbc @rebeccamcginn @rhenshaw56