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

Refactor Client API: pull metadata handling code into the store. #9877

Merged
merged 30 commits into from
Feb 20, 2020

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Feb 17, 2020

Part one of a @storybook/client-api refactor.

Bring metadata (parameters + decorators) logic into the story store.

The client_api file is now a simpler wrapper around the store and does not include any state.

In the future we may keep parameters "decomposed" and pass them over the channel as such, for now we still combine them together at story add time.

For other key changes in the PR, please see the MIGRATION.md notes:https://github.com/storybookjs/storybook/pull/9877/files#diff-177dcef0a5b481c2c87db87f7cdf200d

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2020

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 5b56f44

@shilman
Copy link
Member

shilman commented Feb 17, 2020

@tmeasday does this also include the "don't merge arrays" change?

@tmeasday
Copy link
Member Author

@tmeasday does this also include the "don't merge arrays" change?

Not yet but it will

@tmeasday
Copy link
Member Author

tmeasday commented Feb 19, 2020

@ndelangen / @shilman - any idea about this failing knobs cypress test?

I think the issue comes from here: https://github.com/storybookjs/storybook/blob/0a2d97267376e803ac9ad82c18f661100dd15354/addons/knobs/src/registerKnobs.ts#L12:L12

The test grabs the knob value without waiting the 325ms so I think the story re-renders after it begins to check it.

However I am not sure why it would have started failing on this branch. I don't really think I touched the code path that goes from the knob changing a value, emitting the force re-render event and then rendering the story (the last bit, sure but I don't think the new code would render it significantly differently).

Should I just add a cy.wait(325) to the test?

@ndelangen
Copy link
Member

There should be no need for a cy.wait

@ndelangen
Copy link
Member

I tested it locally, and it seemed to work, i think it's fixed now

@shilman
Copy link
Member

shilman commented Feb 19, 2020

@tmeasday can you take a look at the failing chromatic test? it looks like the parameters got reordered and i'm not sure i understand why -- seems like it could potentially introduce subtle bugs in user code?

@tmeasday
Copy link
Member Author

@tmeasday can you take a look at the failing chromatic test? it looks like the parameters got reordered and i'm not sure i understand why -- seems like it could potentially introduce subtle bugs in user code?

Yeah I don't get it, it doesn't order them in that order here :/

@tmeasday
Copy link
Member Author

Actually the order seems correct to me; docs is the first addon loaded by official-storybook. I'm not sure why it was different before 🤔

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks great!! 💯

import { withA11Y } from '../index';

addDecorator(withA11Y);
export const decorators = [withA11Y];
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to document this in MIGRATION.md?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure if we document new features in there or not. The old way is still supported 🤷‍♂

@tmeasday tmeasday merged commit a7dc544 into next Feb 20, 2020
@tmeasday
Copy link
Member Author

Going to merge this and follow up on some other points later.

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.

3 participants