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

Upgrade and migrate Storybook to v6 #2128

Closed
aaemnnosttv opened this issue Oct 5, 2020 · 7 comments
Closed

Upgrade and migrate Storybook to v6 #2128

aaemnnosttv opened this issue Oct 5, 2020 · 7 comments
Labels
P2 Low priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Oct 5, 2020

Feature Description

Shortly after we recently upgraded the bulk of our dependencies in #1356, Storybook v6 was released.

The release is said to be "almost fully backwards compatible with 5.X", so it should require little to no necessary changes, but some changes may be good to include to better prepare for #1935 and other best practices as well as move away from any potentially deprecated uses.

See the official Migration Guide


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Storybook packages should be upgraded to their latest ^6.0 or compatible version
  • Storybook configuration should use to the new main.js and preview.js modules
  • Stories should no longer be restricted to definition in the stories directory and new stories should be able to be written in CSF format

Implementation Brief

  • Upgrade all Storybook packages using the following command:
     npx sb upgrade
  • Using /.storybook
    • Create main.js which contains the following:
       module.exports = {
       	stories: [
       		'../stories/**/*.stories.js',
       		'../assets/js/**/*.stories.js',
       	],
       	addons: [
       		'@storybook/addon-viewport',
       	],
       };
    • Create preview.js which contains the code from config.js except the part where stories are loaded:
       const req = require.context( '../stories', true, /\.stories\.js$/ );
      
       function loadStories() {
       	req.keys().forEach( ( filename ) => req( filename ) );
       }
      
       configure( loadStories, module );
    • Delete the following unused files:
      • config.js
      • addons.js
  • Run storybook and check if all stories are working fine.

Test Coverage

  • All stories should be loaded and working fine.

Visual Regression Changes

  • There should be no visual changes.

QA Brief

  • Run storybook using the following command: npm run storybook
  • All stories should load without errors.

Changelog entry

  • N/A
@aaemnnosttv aaemnnosttv added P2 Low priority Type: Enhancement Improvement of an existing feature labels Oct 5, 2020
@aaemnnosttv aaemnnosttv self-assigned this Oct 5, 2020
@aaemnnosttv aaemnnosttv removed their assignment Dec 10, 2020
@aaemnnosttv
Copy link
Collaborator Author

@felixarntz I don't recall what else I was waiting for here but I think it's already ready for an IB 👍

@aaemnnosttv aaemnnosttv added the QA: Eng Requires specialized QA by an engineer label Dec 10, 2020
@asvinb asvinb assigned asvinb and unassigned asvinb Jan 5, 2021
@aaemnnosttv aaemnnosttv self-assigned this Jan 11, 2021
@aaemnnosttv
Copy link
Collaborator Author

@asvinb

module.exports = {
	stories: [
		'../stories/**/*.stories.js',
	],

This should also allow for assets/js/**/*.stories.js so that stories can be co-located with their components like we do with tests.

addons: [
	'@storybook/addon-viewport/register',
],

I think this can be simplified as Gutenberg uses to exclude the /register as the register.js script use is deprecated (I think the docs for the viewport addon are out of date here). See here https://storybook.js.org/docs/react/addons/install-addons#using-addons

@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Jan 11, 2021
@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Jan 12, 2021
@aaemnnosttv
Copy link
Collaborator Author

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Jan 12, 2021
@fhollis fhollis added this to the Sprint 41 milestone Jan 15, 2021
@asvinb asvinb self-assigned this Jan 18, 2021
@asvinb
Copy link
Collaborator

asvinb commented Jan 20, 2021

This is taking longer than anticipated because of some changes in Storybook 6. In Storybook 5 (the version we are using), the decorators within stories are self contained and the code for those decorators are run only when viewing the story, even they are under the same kind. For e.g the ReportZero story is within the Global kind . The Global kind hosts other components as well, ReportError, Header, ResetButton, etc, in separate files.
As per the migration guide, in Storybook 6, they have deprecated the ability to split a kind's (component's) stories into multiple files and will likely be removed in v7. So there is some refactoring to be done.
Since we have many different stories under the same kind (for e.g Global) and each stories have their own decorators, we'll need to refactor the stories to use local decorators per story. For e.g

export const ReportZeroStory = () => (
	<ReportZero moduleSlug="test-module" />
);
ReportZeroStory.storyName = 'Report Zero';
ReportZeroStory.decorators = [
	( Story ) => {
		const registry = createTestRegistry();
		const testModuleDefinition = createModuleStore( 'test-module', {
			storeName: 'modules/test-module',
		} );
		registry.registerStore( testModuleDefinition.STORE_NAME, testModuleDefinition );
		provideModules( registry, [ { slug: 'test-module', name: 'Test Module' } ] );

		return (
			<WithTestRegistry registry={ registry }>
				<Story />
			</WithTestRegistry>
		);
	},
];

export default {
	title: 'Global',
};

This will prevent decorators which is not related to the story to be run. In case a story exports different stories, for e.g header.stories.js, we'll have the add the decorators for each story. Since we'll have to update almost all the stories to update the decorators, I suggest we take to time to convert all the stories to not use storiesOf and switch over to the Component Story Format.

Let me know what you think.

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Jan 20, 2021
@aaemnnosttv
Copy link
Collaborator Author

Thanks for the report @asvinb – that the migration doesn't seem to be as straightforward as originally anticipated.

Let's do some testing to see if there is a way to continue using the storiesOf API (since it isn't deprecated yet) with our current stories for now to avoid a major refactor here if possible.

@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Jan 25, 2021
@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Jan 29, 2021
@fhollis fhollis added the Rollover Issues which role over to the next sprint label Feb 1, 2021
@fhollis fhollis modified the milestones: Sprint 41, Sprint 42 Feb 1, 2021
@ivankruchkoff
Copy link
Contributor

@asvinb and @aaemnnosttv as part of the work for #2941 feature/2941-button-storybook6-stories I spun up a branch and pulled some of the content in your PR, writing a story in the new format (v6) for Buttons, and it works with the old format of stories (v5). Not entirely sure why it's not currently starting the checks https://github.com/google/site-kit-wp/pull/2959/checks but it is working locally.

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Mar 18, 2021
@aaemnnosttv aaemnnosttv removed their assignment Mar 18, 2021
@tofumatt tofumatt self-assigned this Mar 19, 2021
@tofumatt
Copy link
Collaborator

QA ✅

@tofumatt tofumatt removed their assignment Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants