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

Angular: Use parameters.component if available #8673

Closed
shilman opened this issue Nov 2, 2019 · 7 comments
Closed

Angular: Use parameters.component if available #8673

shilman opened this issue Nov 2, 2019 · 7 comments

Comments

@shilman
Copy link
Member

shilman commented Nov 2, 2019

In angular stories, you currently need to pass a component argument as part of the story input:

export const basic = () => ({
  component: AppComponent,
  props: {},
});

However, in 5.2 and later, it's recommended to pass this as part of the component metadata:

export default {
  title: 'App Component',
  component: AppComponent,
};

export basic = () => ({ ... });

This gets translated into the underlying API call:

storiesOf('App Component', module)
  .addParameters({ component: AppComponent })
  .add('Basic', () => ...);

Therefore Angular should use parameters.component if it's available and the story's component ISN'T available. In 6.0 we should deprecate the Story's component, and in 7.0 we should remove support entirely.

@shilman shilman changed the title Angular: Use parameters.component if available Angular: Use parameters.component if available Nov 3, 2019
@shilman shilman added the todo label Nov 8, 2019
@ThibaudAV
Copy link
Contributor

@shilman always up to date ?

I rephrase to make sure I understand.
For v6.x we have to add the parameters.component and use it first otherwise fallback on StoryFnAngularReturnType.component ?
And in v7 it will be deleted

if it's ok, it can be done quickly

@shilman
Copy link
Member Author

shilman commented Dec 4, 2020

@ThibaudAV Given that we've already shipped 6.0, I'd say:

  • Deprecate in 6.2
  • Remove in 7.0 (or later?)

And the logic I had in mind is slightly different, maybe poorly phrased in the original description:

  • If user provides StoryFnAngularReturnType.component, use it but issue a deprecation warning.
  • If NOT, use parameters.component (and recommend this as best practices in the docs)
  • If there is no parameters.component, throw an error

WDYT @kroeder @Marklb @ndelangen @tmeasday

@ThibaudAV
Copy link
Contributor

ThibaudAV commented Dec 4, 2020

If there is no parameters.component, throw an error

🤔 the component is optional if the user uses a template, no ?
unless making it mandatory is also necessary and desired ?

@shilman
Copy link
Member Author

shilman commented Dec 5, 2020

@ThibaudAV sorry, you're correct. i was only trying to document the precedence structure between the default.component and StoryFnAngularReturnType.component

@ThibaudAV
Copy link
Contributor

ThibaudAV commented Dec 5, 2020

@shilman can you detail a little bit more why pass "component" in parameters ? or is it explained in another issue? 🙃

I started a PR here #13383
I don't know if there are particular warning formats or what to put in the MIGRATION.md to make it as clear as possible

@shilman
Copy link
Member Author

shilman commented Dec 5, 2020

@ThibaudAV of course. here's the rationale:

we encourage story authors to group their stories by component (formerly known as "kind"). the default export in CSF represents metadata that applies to a component, including the component itself. currently, default.component is used to auto-generate the ArgsTable for the component (and is available at runtime in parameters.component). in the future, we plan to use it for more than that, e.g. to collect test coverage for the component ... even to auto-generate stories for the component.

Angular's StoryFnAngularReturnType.component was created before CSF, so there was no component available at the "kind" level. Once the user has specified default.component, we no longer need to specify StoryFnAngularReturnType.component for each story in the file.

Hope that helps, or feel free to discuss if it doesn't! 🙏

@shilman
Copy link
Member Author

shilman commented Dec 16, 2020

Olé!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-alpha.8 containing PR #13383 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

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

No branches or pull requests

2 participants