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 Storybook to v6 #214

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Conversation

jeremylenz
Copy link
Contributor

@jeremylenz jeremylenz commented Sep 2, 2020

Testing instructions are on the associated Foreman PR: theforeman/foreman#7990

Along with the upgrade to v6, this PR incorporates several Storybook improvements and addresses issues I had along the way:

  • Switched to the new main.js API. This simplifies our configuration quite a bit.
  • With the new main.js configuration, it appeared that some Webpack loaders were being run twice on the same files, particularly the @mdx-js/loader. This manifested in an error message Unexpected default export without title for every story. The loaders were being run once from main.js and another time from our manual Webpack configuration in webpack.config.rules.js.
  • After addressing the above, Storybook still wouldn't render any stories, instead spewing out copious console errors. I noticed that in index.js we were doing export * from several @storybook/addon packages. Once I removed them, the Storybook finally rendered.
  • Since the Storybook addon exports are no longer passed through @theforeman/stories, the stories files in Foreman must import them directly from @storybook/addon-knobs etc. I added deprecation errors to inform the users. Also, I suppressed the import/no-extraneous-dependencies eslint rule for packages that are dependencies of @theforeman/stories.
  • The story sorting implementation we were using in v5.3 involved passing in a storyWeight parameter, which stopped working in v6. Thanks to the Storybook folks for the help on Component-level parameters not available in storySort storybookjs/storybook#11010, I was able to update our storySort function and no changes will be required in our story files.

Comment on lines -3 to -4
export * from '@storybook/addon-actions';
export * from '@storybook/addon-storysource';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storybook didn't render when both of these were exported. Probably not safe to export * from multiple npm packages when some of the exports could have the same names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: Added deprecations to inform users of the new export locations. also updated all existing stories in theforeman/foreman#7990

@jeremylenz jeremylenz marked this pull request as ready for review September 29, 2020 19:02
@jeremylenz
Copy link
Contributor Author

@sharvit @johnpmitsch rebased and ready for review :)

Will squash once acked

const storyWeightA = kindParamsA.storyWeight || globalParamsA.storyWeight;
const storyWeightB = kindParamsB.storyWeight || globalParamsB.storyWeight;
const combined = storyWeightA - storyWeightB;
if (combined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will evaluate to false if 0

// htmlWebpackPlugin is the first plugin in the array
const htmlWebpackPlugin = config.plugins[0];
const htmlWebpackPlugin = config.plugins.find(
p => p.constructor.name === 'HtmlWebpackPlugin'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the upgrade it appears the order of config.plugins changed, so I changed this to look for the correct name instead

@johnpmitsch
Copy link
Contributor

The functionality appears to be working great!

The code seems fine but I need to take a closer look at the webpack aspect of it as I'm not as familiar with the existing code there. Maybe @sharvit can review that part better.

@sharvit sharvit self-assigned this Oct 12, 2020
@jeremylenz
Copy link
Contributor Author

@sharvit can this be merged soon?

@johnpmitsch
Copy link
Contributor

@jeremylenz commits look a little funky here, did something happen when rebasing?

@jeremylenz
Copy link
Contributor Author

@johnpmitsch Re-rebased, hopefully it's better now

@johnpmitsch
Copy link
Contributor

@jeremylenz still working well 👍

johnpmitsch
johnpmitsch previously approved these changes Nov 13, 2020
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thanks @jeremylenz 👍

I deployed those changes to npm as 6.1.0-next-storybook-6-upgrade.0 and it works well together with theforeman/foreman#7990, see:

To install it, replace the version in the package.json with ^6.1.0-next-storybook-6-upgrade.0.

  1. Please update the readme file according to the changes and the deprecations.

  2. Please squash and finalize your commit message.
    In your commit, describe the breaking changes and write a migration guide so it will be landed in the changelog (https://github.com/theforeman/foreman-js/blob/master/CHANGELOG.md) and in the release notes (https://github.com/theforeman/foreman-js/releases).

@jeremylenz
Copy link
Contributor Author

Thanks @sharvit, squashed and updated!

jeremylenz added a commit to jeremylenz/foreman that referenced this pull request Nov 16, 2020
- Do not merge until theforeman/foreman-js#214
  is merged and @theforeman/stories@7.0.0 is published to npm
@sharvit
Copy link
Contributor

sharvit commented Nov 17, 2020

Thanks @jeremylenz

  1. Please add another depreciation error and a breaking change line about the storiesOf is not supported anymore.

  2. I was playing with the changelog and the release notes generator and it seems to be working best when I formated the commit as well:

    feat(stories): update Storybook to v6

    BREAKING CHANGE: @storybook/addon-knobs, @storybook/addon-actions,
    and @storybook/addon-storysource are no longer re-exported
    from @theforeman/stories.  Going forward, import these items
    directly: e.g. `import { withKnobs } from @storybook/addon-knobs`
    instead of `import { withKnobs } from @theforeman/stories`.
    Deprecation error messages have been added where appropriate.

    BREAKING CHANGE: The Preview component has been renamed to Canvas.

    BREAKING CHANGE: The Props component has been renamed to ArgsTable.

    BREAKING CHANGE: Custom separators are no longer supported in story titles.
    Instead of `title: 'Components|DiffView'` you must write `title: 'Components/DiffView'`.

This format generated the following release notes:

Please format the commit accordingly.

BREAKING CHANGE: @storybook/addon-knobs, @storybook/addon-actions,
and @storybook/addon-storysource are no longer re-exported
from @theforeman/stories.  Going forward, import these items
directly: e.g. `import { withKnobs } from @storybook/addon-knobs`
instead of `import { withKnobs } from @theforeman/stories`.
Deprecation error messages have been added where appropriate.

BREAKING CHANGE: The Preview component has been renamed to Canvas.

BREAKING CHANGE: The Props component has been renamed to ArgsTable.

BREAKING CHANGE: Custom separators are no longer supported in story titles.
Instead of `title: 'Components|DiffView'` you must write `title: 'Components/DiffView'`.

BREAKING CHANGE: The storiesOf API is no longer supported.
Please use Component Story Format or MDX instead.
@jeremylenz
Copy link
Contributor Author

@sharvit updated and formatted the commit message!

Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thanks @jeremylenz

@sharvit sharvit merged commit f9abe6e into theforeman:master Nov 18, 2020
@sharvit
Copy link
Contributor

sharvit commented Nov 18, 2020

Published as v7.0.0, see https://github.com/theforeman/foreman-js/releases/tag/v7.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants