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

Update getStorybook to return story parameters #5549

Closed
wants to merge 3 commits into from

Conversation

timhaines
Copy link

@timhaines timhaines commented Feb 11, 2019

Issue:
v4 of Storybook introduced story parameters, but getStorybook doesn't return them. This PR updates getStorybook so it does return parameters.

This allows in-browser consumers to access story parameters with a more supportable window['__STORYBOOK_CLIENT_API__'].getStorybook() instead of window['__STORYBOOK_CLIENT_API__']['_storyStore']['_data']

What I did

Updated the getStorybook function to return parameters

How to test

Unit tests

Copy link
Contributor

@Keraito Keraito left a comment

Choose a reason for hiding this comment

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

From a code perspective there's not much wrong with this, tagging @tmeasday to judge the behaviour of this as he was responsible for the tagged PR.

lib/client-api/src/story_store.js Outdated Show resolved Hide resolved
lib/client-api/src/client_api.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #5549 into next will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##            next   #5549   +/-   ##
=====================================
  Coverage   33.1%   33.1%           
=====================================
  Files        638     638           
  Lines       9264    9264           
  Branches    1340    1318   -22     
=====================================
  Hits        3067    3067           
  Misses      5571    5571           
  Partials     626     626
Impacted Files Coverage Δ
lib/client-api/src/story_store.js 75% <100%> (ø) ⬆️
lib/client-api/src/client_api.js 79.59% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9cd83c...04e2b96. Read the comment docs.

@ndelangen
Copy link
Member

Hey @timhaines In v5 there's a new api for getting the stories data complete with parameters:

https://github.com/storybooks/storybook/blob/next/lib/client-api/src/story_store.js#L111-L123

Long term I'd like to remove the getStorybook api.

Would be happy to merge this PR though in the meantime.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

I think returning extra data in getStorybook() is good, but do we need to change the return value of getStories()? That part doesn't seem as necessary?

PS We use code something like

.getStorybook()
  .map(({ kind, stories }) =>
    stories.map(({ name }) => {
      const { parameters } = storyStore.getStoryAndParameters(kind, name);

Although I'm looking at our code and it says "Annoying to have to do this, we should support this in .getStorybook" so it looks like this change is long overdue ;)

lib/client-api/src/story_store.js Show resolved Hide resolved
@timhaines
Copy link
Author

@tmeasday to address your valid concern, I've left getStories behavior intact, and have introduced getStoriesWithParameters.

Note that getStories is only tested indirectly, as getStoriesWithParameters is now.

@timhaines
Copy link
Author

@ndelangen thanks for the heads up on those two functions. I don't think they can be used as is in place of getStories, as they don't preserve the ordering in the same way getStories returns stories.

It's interesting to hear you're planning on deprecating and removing the getStorybook API long-term. Is there anything documented about how you'd like to replace the functionality?

@timhaines
Copy link
Author

Closing this in favor of adopting raw() over improving getStorybook().

@timhaines timhaines closed this Feb 12, 2019
@timhaines timhaines deleted the update-getstorybook-to-include-parameters branch February 12, 2019 00:55
@ndelangen
Copy link
Member

Thanks tim, if were missing any functionality in either .raw or .extract please let me know.

The .extract is for storybook itself, to transfer the stories to the manager.
.raw is for integration purposes such as storyshots.

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

Successfully merging this pull request may close these issues.

4 participants