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

Core: Fix api.selectStory for component permalinks #9054

Merged
merged 3 commits into from
Dec 5, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Dec 4, 2019

Issue: #8507 (follow-up)

What I did

Added some cases to take care of some addon-links bugs, which may also be broken in other uses of the API.

How to test

See updated unit tests & official-storybook

@shilman shilman requested a review from usulpro as a code owner December 4, 2019 10:30
@vercel
Copy link

vercel bot commented Dec 4, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/ozskfue97
🌍 Preview: https://monorepo-git-8507-addon-links-permalinks.storybook.now.sh

@shilman shilman changed the title Core: Fix api.selectStory in the case of component permalinks Core: Fix api.selectStory for component permalinks Dec 4, 2019
@shilman shilman requested a review from tmeasday December 4, 2019 10:31
@shilman shilman added this to the 5.3.0 milestone Dec 4, 2019
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.

Suggestion to make it simpler

// Support legacy API with component permalinks, where kind is `x/y` but permalink is 'z'
const k = storiesHash[sanitize(kindOrId)];
if (k && k.children) {
const foundId = k.children.find(childId => storiesHash[childId].name === story);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to do:

// This kind has a custom id, look up the story using it
if (k && k.id !== sanitize(kindOrId))
  return selectStory(k.id, story)

PS prefer kind to k if the code is more than one line.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Unfortunately k.id is sanitize(kindOrId)
  2. kind was already in scope

Copy link
Member

Choose a reason for hiding this comment

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

Is there nothing stored on the kind in the store to indicate its custom id? Should we change that?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this line of code is pretty dodgy and we'll regret it in the long run

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that the entire setup is lacking integrity, but I don't have a good idea for a better setup. Any suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where would this get passed in:

storiesOf({ kind, componentId }, module).add(...);

storiesOf(kind, module).setComponentId(componentId);

???

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so currently we don't store componentId at all, we just change the individual stories ids based on it?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest setComponentId()

Copy link
Member Author

@shilman shilman Dec 5, 2019

Choose a reason for hiding this comment

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

@tmeasday there's no intermediate data structure in story_store to hold this AFAIK. i feel like this is all getting out of scope.

Copy link
Member

Choose a reason for hiding this comment

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

OK, fair enough.

// Support legacy API with component permalinks, where kind is `x/y` but permalink is 'z'
const k = storiesHash[sanitize(kindOrId)];
if (k && k.children) {
const foundId = k.children.find(childId => storiesHash[childId].name === story);
Copy link
Member

Choose a reason for hiding this comment

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

Is there nothing stored on the kind in the store to indicate its custom id? Should we change that?

lib/api/src/tests/stories.test.js Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging December 4, 2019 13:58 Inactive
@shilman shilman merged commit 3b62882 into next Dec 5, 2019
@shilman shilman deleted the 8507-addon-links-permalinks branch December 5, 2019 02:55
@shilman shilman mentioned this pull request Dec 5, 2019
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.

2 participants