upgrade Storybook to 6.0

[haszari]

Fixes #3028

Upgrading Storybook to latest. More info / migration guides:

• https://medium.com/storybookjs/storybook-6-migration-guide-200346241bb5
• https://github.com/storybookjs/storybook/blob/next/MIGRATION.md

Changes:

• Storybook packages all updated (via npx sb upgrade).
• Added a dependency on core-js. This is not strictly a dependency of Storybook but is needed as a workaround (our story build fails without it).

There's more we can do - 6.0 has a bunch of cool new stuff (in particular: essentials + controls + docs). Saving that for other PRs since we don't need to do it to upgrade. Added a follow-up issue: #3041

How to test the changes in this Pull Request:

1. Clone this branch.
2. Install dependencies - npm install or npm ci.
3. Run storybook - npm run storybook.
4. Browse storybook (runs on port 6006 by default) and confirm nothing's broken.

[github-actions]

Size Change: +1.79 kB (0%)

Total Size: 1.11 MB

Filename	Size	Change
build/active-filters.js	8.83 kB	+1 B
build/all-products.js	35.7 kB	-2 B (0%)
build/all-reviews.js	9.76 kB	+2 B (0%)
build/atomic-block-components/add-to-cart.js	7.46 kB	-1 B
build/atomic-block-components/button.js	837 B	-2 B (0%)
build/atomic-block-components/category-list.js	475 B	-1 B
build/atomic-block-components/price.js	2.11 kB	+2 B (0%)
build/atomic-block-components/rating.js	526 B	-1 B
build/atomic-block-components/sale-badge.js	865 B	+1 B
build/atomic-block-components/stock-indicator.js	572 B	+1 B
build/atomic-block-components/summary.js	924 B	-1 B
build/atomic-block-components/tag-list.js	472 B	-1 B
build/atomic-block-components/title.js	1.05 kB	-1 B
build/attribute-filter.js	12.4 kB	-2 B (0%)
build/cart.js	37.4 kB	-9 B (0%)
build/checkout.js	40.6 kB	-14 B (0%)
build/featured-product.js	9.94 kB	-1 B
build/price-filter.js	10.2 kB	-1 B
build/product-category.js	8.35 kB	+1 B
build/product-on-sale.js	7.97 kB	+2 B (0%)
build/product-search.js	3.54 kB	+1 B
build/product-tag.js	6.5 kB	+2 B (0%)
build/product-top-rated.js	7.55 kB	+1 B
build/products-by-attribute.js	8.29 kB	-4 B (0%)
build/reviews-by-category.js	11.8 kB	+1 B
build/reviews-by-product.js	13.3 kB	+4 B (0%)
build/single-product.js	10.1 kB	+2 B (0%)
build/vendors.js	416 kB	+1.81 kB (0%)

ℹ️ View Unchanged

Filename	Size	Change
build/active-filters-frontend.js	8.76 kB	0 B
build/all-products-frontend.js	31.2 kB	0 B
build/atomic-block-components/add-to-cart-frontend.js	8.89 kB	0 B
build/atomic-block-components/add-to-cart~atomic-block-components/button.js	3.16 kB	0 B
build/atomic-block-components/add-to-cart~atomic-block-components/image~atomic-block-components/title.js	335 B	0 B
build/atomic-block-components/button-frontend.js	2.02 kB	0 B
build/atomic-block-components/category-list-frontend.js	468 B	0 B
build/atomic-block-components/image-frontend.js	1.71 kB	0 B
build/atomic-block-components/image.js	1.15 kB	0 B
build/atomic-block-components/price-frontend.js	2.08 kB	0 B
build/atomic-block-components/rating-frontend.js	524 B	0 B
build/atomic-block-components/sale-badge-frontend.js	863 B	0 B
build/atomic-block-components/sku-frontend.js	389 B	0 B
build/atomic-block-components/sku.js	394 B	0 B
build/atomic-block-components/stock-indicator-frontend.js	567 B	0 B
build/atomic-block-components/summary-frontend.js	918 B	0 B
build/atomic-block-components/tag-list-frontend.js	466 B	0 B
build/atomic-block-components/title-frontend.js	1.22 kB	0 B
build/attribute-filter-frontend.js	18.1 kB	0 B
build/blocks.js	3.54 kB	0 B
build/cart-frontend.js	69 kB	0 B
build/checkout-frontend.js	84 kB	0 B
build/editor-rtl.css	13.9 kB	0 B
build/editor.css	13.9 kB	0 B
build/featured-category.js	7.7 kB	0 B
build/handpicked-products.js	7.34 kB	0 B
build/price-filter-frontend.js	14.4 kB	0 B
build/product-best-sellers.js	7.42 kB	0 B
build/product-categories.js	3.23 kB	0 B
build/product-new.js	7.58 kB	0 B
build/reviews-frontend.js	9.34 kB	0 B
build/single-product-frontend.js	34 kB	0 B
build/style-rtl.css	18 kB	0 B
build/style.css	18 kB	0 B
build/vendors-style-rtl.css	1.03 kB	0 B
build/vendors-style.css	1.03 kB	0 B
build/vendors~atomic-block-components/price-frontend.js	5.65 kB	0 B
build/wc-blocks-data.js	7.09 kB	0 B
build/wc-blocks-middleware.js	931 B	0 B
build/wc-blocks-registry.js	2.28 kB	0 B
build/wc-payment-method-bacs.js	790 B	0 B
build/wc-payment-method-cheque.js	787 B	0 B
build/wc-payment-method-cod.js	875 B	0 B
build/wc-payment-method-paypal.js	831 B	0 B
build/wc-payment-method-stripe.js	11.9 kB	0 B
build/wc-settings.js	2.33 kB	0 B
build/wc-shared-context.js	1.53 kB	0 B
build/wc-shared-hocs.js	1.66 kB	0 B

_{compressed-size-action}

[haszari]

Rebased to address conflicts - ready for review! Keen to get this in so we can start writing new stories. (Will work on a branch based off this for now.)

[nerrad]

In general, changes look okay but there are a variety of console warnings/errors depending on what component is being previewed. So it'd be good if you reviewed all the components to make sure any console warnings/errors that don't happen in the current storybook and do happen in this branch are addressed.

Also, I don't think it's impacted, but after you merge to master you'll need to make sure that the storybook deploy on merge to master still works and generates the storybook instance for public viewing.

I'm going to pre-approve but please don't merge until console errors/warnings differing from what's currently in master are handled.

[haszari]

In general, changes look okay but there are a variety of console warnings/errors depending on what component is being previewed. So it'd be good if you reviewed all the components to make sure any console warnings/errors that don't happen in the current storybook and do happen in this branch are addressed.

Good point – I meant to come back to this, thanks for the reminder.

Also, I don't think it's impacted, but after you merge to master you'll need to make sure that the storybook deploy on merge to master still works and generates the storybook instance for public viewing.

Yes will do!

[haszari]

console warnings/errors depending on what component is being previewed

Checked them all on main then on this branch (npm ci between each check of course). There are lots of errors on main, and most of the same errors on this branch - one gets fixed! Note: I don't see the errors on GitHub pages storybook; I'm assuming that's a release build, and they are hidden.

So here's what I'm thinking (assuming they are not introduced). Let's merge this PR as is - 🎉 we're upgraded! And then…

1. Log issues for each of errors/warnings ... OR
2. Work through all existing stories as a team to fix any issues and consider using new Storybook features (MDX docs, controls) as we work through them.

I'm leaning towards (2). Here's why:

• There are a few issues to log (relative to number of stories total), and these aren't impacting anyone, so I'm thinking it's more efficient to just commit as a team to tidying up our stories (< 10).
• Also, the console issues aren't the only area we could improve. For example, the CountryInput dropdown renders in dark mode. Also the coupon input could include submit/success/error behaviour in the story. IMO it's better to try and improve things and get familiar with Storybook 6 tech at the same time.

What do you think @nerrad - sound like a plan? We have <10 stories total, so if we only need to do 2-3 each 👨‍👨‍👦‍👦.

Going forward, we'll need to keep an eye on this to avoid stuff like this creeping in over time. Keen for ideas on that, but maybe we just need to get in the habit of using Storybook locally (I'm not)!

Scroll down for ugly details:

Storybook warnings/errors on main

# MIA __experimentalCreateInterpolateElement with old 'wordpress-components'
# can fix by updating "wordpress-components": "npm:@wordpress/components@10.0.5", 
# but why do we have this dep?
WARNING in ./node_modules/wordpress-components/build-module/text-highlight/index.js 24:9-47
"export '__experimentalCreateInterpolateElement' was not found in '@wordpress/element'
 @ ./node_modules/wordpress-components/build-module/index.js
 @ ./assets/js/base/components/button/index.js
 @ ./assets/js/base/components/button/stories/index.js
 @ ./assets/js sync ^\.\/(?:(?:|\/|(?:(?:(?!(?:|\/)\.).)*?)\/)stories\/(?!\.)(?=.)[^/]*?\.js)$
 @ ./storybook/generated-entry.js
 @ multi ./node_modules/@storybook/core/dist/server/common/polyfills.js ./node_modules/@storybook/core/dist/server/preview/globals.js ./node_modules/@storybook/addon-docs/dist/frameworks/common/config.js ./node_modules/@storybook/addon-docs/dist/frameworks/react/config.js ./storybook/preview.js ./node_modules/@storybook/addon-knobs/dist/preset/addDecorator.js ./storybook/generated-entry.js (webpack)-hot-middleware/client.js?reload=true&quiet=true

# Horrible error on storybook load
Warning: Cannot update during an existing state transition (such as within `render`). Render methods should be a pure function of props and state.
    in Unknown (created by Context.Consumer)
    in WithTheme(Component)
    in WithTheme(Component) (created by Preview)
    in div (created by Context.Consumer)
    in Side (created by FlexBar)
    in div (created by Context.Consumer)
    in Styled(div) (created by FlexBar)
    in div (created by SimpleBar)

    ...

    in div (created by Context.Consumer)
    in Styled(div)
    in Unknown
    in Unknown (created by SizeMeRenderer(Component))
    in SizeMeReferenceWrapper (created by SizeMeRenderer(Component))
    in SizeMeRenderer(Component) (created by SizeMe(Component))
    in SizeMe(Component) (created by Manager)
    in ThemeProvider (created by Manager)
    in Manager (created by Context.Consumer)
    in Location (created by QueryLocation)
    in QueryLocation (created by Root)
    in LocationProvider (created by Root)
    in HelmetProvider (created by Root)
    in Root

# Error on selecting a country with CountryInput
react-dom.development.js:88 Warning: Cannot update a component (`StoryComponent`) while rendering a different component (`CustomSelectControl`). To locate the bad setState() call inside `CustomSelectControl`, follow the stack trace as described in https://fb.me/setstate-in-render
    in CustomSelectControl (created by Select)
    in div (created by Select)
    in Select (created by ValidatedSelect)
    in ValidatedSelect (created by WithInstanceId(ValidatedSelect))
    in WithInstanceId(ValidatedSelect) (created by CountryInput)
    in div (created by CountryInput)
    in CountryInput (created by StoryComponent)
    in StoryComponent (created by storyFn)
    in ValidationContextProvider (created by storyFn)
    in storyFn
    in ErrorBoundary

Also - coupon story is not interactive, submit does nothing.

# 3x markup issues in icons story
react-dom.development.js:88 Warning: Unsupported style property padding-left. Did you mean paddingLeft?
    in svg (created by SVG)
    in SVG
    in Icon (created by LibraryExample)
react-dom.development.js:88 Warning: Unsupported style property flex-direction. Did you mean flexDirection?
    in div (created by LibraryExample)
    in div (created by LibraryExample)
    in div (created by LibraryExample)
    in LibraryExample (created by storyFn)
    in storyFn
    in ErrorBoundary
react-dom.development.js:88 Warning: Unsupported style property flex-wrap. Did you mean flexWrap?
    in div (created by LibraryExample)
    in div (created by LibraryExample)
    in LibraryExample (created by storyFn)
    in storyFn
    in ErrorBoundary

And here's what we get on upgrade/storybook-6

• MIA __experimentalCreateInterpolateElement with old 'wordpress-components' dep.
• Error on selecting a country with CountryInput.
• Icons story markup issues.

[haszari]

A note about one issue specifically @nerrad - MIA __experimentalCreateInterpolateElement with old 'wordpress-components' dep.

I think this is because we have an explicit dependency on a specific old version – "wordpress-components": "npm:@wordpress/components@8.5.0",. Alongside another regular devDependency on @wordpress/components@8.5.0.

I upgraded both to 10.0.5 and it fixed the Storybook issue. I'm confused though, why do we have the devDependency, aliasing to wordpress-components? And is it ok to upgrade both of these? I can log an issue about this, lots of potential code impact (@import 'wordpress-components').

Hopefully we can just upgrade to latest in the normal way 😄

[nerrad]

Thanks for the analysis @haszari, I'm fine with merging as is given your report.

I'm leaning towards (2). Here's why:

I'm okay with this approach but before you start it'd be good to at least identify what issues are story agnostic and log just issues for those things. Splitting up stories in parallel is good but you don't want folks fixing the same issue in each of their pulls.

I'm confused though, why do we have the devDependency, aliasing to wordpress-components?

I'm looking at the devDependency map in the main branch and it's not aliasing to wordpress-components? devDependency has @wordpress/components (correctly) listed as a dependency.

The reason why @wordpress/components is a devDependency is for the purpose of automated tests so we don't have to mock all the imports from WordPress packages. Our builds reference WordPress packages via externals in which code comes in via the wp global. Tests (other than e2e) don't have access to the WordPress global, so the source file imports in the test environment reference the imports from node_modules instead of the global.

dependencies has the wordpress-components alias in package dependency that points to the direct package because there are a few places in built code where we don't want to use what is in the WordPress global for that package as an external, because a WordPress version we support does not expose the api we're using in the global (WP includes an older version of the package).

And is it ok to upgrade both of these? I can log an issue about this, lots of potential code impact (@import 'wordpress-components').

This has huge potential impact - you can see this issue for reference so please don't upgrade in this branch. It's quite possible that the issues Nadir was experiencing with upgrading in the issue I linked to are no longer a problem with the latest versions of the components package but the impacts still need accounted for.

To speak to the error around createInterpolateElement though, it's a bit puzzling because afaik, the whole purpose of explicitly using wordpress-components in the dependency (which in turn implements @wordpress/components@8.5) was to get access to that specific function. I'd expect errors in our running tests if there was a universal problem here. So I suspect this is something specific to how Storybook is configured (but just handily resolves when updating the packages).

[haszari]

Woah, thanks @nerrad for explaining – I had a feeling it's complicated!

I'll add issues for the specific errors – I think there's one or two that affect all storybook, and then a couple that affect individual stories.

[haszari]

I'm okay with this approach but before you start it'd be good to at least identify what issues are story agnostic and log just issues for those things. Splitting up stories in parallel is good but you don't want folks fixing the same issue in each of their pulls.

👍 - added one issue for the main, baffling build warning - #3060

The other issues are all specific to component stories (CountryInput, Icons) at runtime. We can add individual issues later if useful, for now I want to stay focused on how we write our stories, and get access to the new good stuff in 6.0.

Merging!

[haszari]

make sure that the storybook deploy on merge to master still works and generates the storybook instance for public viewing

Confirmed this is all still working - here's the travis job: https://travis-ci.com/github/woocommerce/woocommerce-gutenberg-products-block/jobs/378177518

I couldn't find a way to see the Storybook version on the generated site, but the SMALL CAPS heading is new in 6.0. So that confirms it's running the latest. I checked all the stories and everything is 👌

https://woocommerce.github.io/woocommerce-gutenberg-products-block