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

Upgrades Storybook to major version 4 #508

Merged
merged 4 commits into from
Nov 19, 2018

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Nov 15, 2018

This PR is a:

[ ] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[x] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

When this pull request is merged, it will fix the current error that is causing Storybook to be unable to load any component stories:

screen shot 2018-11-15 at 1 58 27 pm

Additional information

The reason Storybook errors out like this is that the storybook-readme addon that we are attempting to use wasn't listed as a dependency in package.json and therefore was not installed. couldn't be found.

I bumped Storybook (@storybook/react) and storybook-readme to major version 4 in pwa-studio and updated Storybook in packages/peregrine and packages/venia-concept to match.

Storybook now finds the storybook-readme addon at the higher level and therefore it does not need to be listed as a dev dependency in peregrine.

After this change, Storybook works as intended:

screen shot 2018-11-15 at 2 06 59 pm

Potential Risk

Upgrading Storybook outside of the peregrine project may introduce some breakages.

I couldn't find anywhere else in pwa-studio or packages/venia-concept where Storybook was actually being used though, so I think the risk is none.

But with that being the case, should we remove the Storybook dependency from packages/venia-conept?

Note!

This PR is opened against the release/2.0 branch. If this is incorrect, kindly let me know and I'll point it to a different branch 🥂 .

@vercel
Copy link

vercel bot commented Nov 15, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@magento-cicd2
Copy link

magento-cicd2 commented Nov 15, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Nov 15, 2018

Coverage Status

Coverage remained the same at 17.555% when pulling 01e7fc4 on supernova-at:storybook-error into 7a4cbf3 on magento-research:release/2.0.

@jimbo
Copy link
Contributor

jimbo commented Nov 15, 2018

Thanks for your contribution! It would appear that storybook-readme is listed under devDependencies in the main repo's package file, though it's an earlier version. I'm surprised it doesn't work.

@zetlen Any idea why it wouldn't pick up the module from the main repo's node_modules directory?

@zetlen
Copy link
Contributor

zetlen commented Nov 16, 2018

Super weird. It may be that the versions are incompatible--can you try upgrading storybook-readme?

@supernova-at
Copy link
Contributor Author

supernova-at commented Nov 16, 2018

It may be that the versions are incompatible

That was it!

I upgraded @storybook/react and storybook-readme to major version 4 and it works without having to install storybook-readme in peregrine.

I also bumped @storybook/react in peregrine and venia-concept to version 4 to match.

@supernova-at
Copy link
Contributor Author

I updated the PR description with the new solution and outlined a potential risk.

@supernova-at supernova-at changed the title Installs missing dependency to resolve Storybook error Upgrades Storybook to major version 4 Nov 16, 2018
@supernova-at
Copy link
Contributor Author

For completeness

can you try upgrading storybook-readme?

I did try this and it did not work without also bumping @storybook/react.

@jimbo jimbo merged commit b81dbb2 into magento:release/2.0 Nov 19, 2018
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.

5 participants