Allow component class in Angular storiesOf

[jonrimmer]

What I did

The need to use the moduleMetadata decorator when scaffolding a basic story is something of a pain when using Angular:

import { storiesOf, moduleMetadata } from '@storybook/angular';
import { MyAwesomeButtonComponent } from './my-awesome-button.component.ts';

storiesOf('MyAwesomeButton', module)
  .addDecorator(moduleMetadata({
    declarations: [MyAwesomeButtonComponent]
  }))
  .add('Default', () => ({
    template: `<my-awesome-button>Hello there</my-awesome-button>`
  });

This PR adds a small but, I feel, significant improvement. It allows the user to pass the component class in place of the story name to storiesOf():

import { storiesOf } from '@storybook/angular';
import { MyAwesomeButtonComponent } from './my-awesome-button.component.ts';

storiesOf(MyAwesomeButtonComponent, module)
  .add('Default', () => ({
    template: `<my-awesome-button>Hello there</my-awesome-button>`
  });

When passed a class, the storiesOf function will strip out any trailing "Component" from the name and use the rest as the name of story. It also automatically adds a moduleMetadata decorator declaring the component in the story module.

You can still nest stories by passing a string as well a component class:

storiesOf('My App/Buttons', MyAwesomeButtonComponent, module)

How to test

I haven't written tests or docs yet. I want to get feedback on whether this is something we are willing to go for?

If it is, I think there are a number of ways the Angular API could be more helpful. E.g. allowing the story to return a string and automatically wrap it in a { template: ... }. But that's for the future.

[shilman]

@jonrimmer This is great and consistent with changes @tmeasday and I have been discussing as part of the Storybook Docs project.

I'd prefer something more along the lines of:

storiesOf('My App/Buttons/MyAwesomeButtonComponent', module)...

OR

storiesOf(MyAwesomeButtonComponent, module)
  .addParameters({ group: 'My App/Buttons' })
  ...

What do you think about that syntax?

[jonrimmer]

@shilman Not sure I understand your first proposal. How is it going to use the class to register the metadata if you're just passing the name as a string?

With the second one, I can understand the appeal of an explicit API like that, and I could see how my approach might desugar to it. But I am also a very lazy programmer, and I think there is a virtue in tools like Storybook favouring convenience and terseness over explicitness for parts of the API that are very frequently used. While it's no great burden to type addParameters({ group: '...' }), when done on every story and every use of Storybook, it adds up.

Still, it's not a hill I want to die on. And I've no problem if that's the approach you want to take. One idea I've been toying with is spinning off a separate library that will contain a very opinionated wrapper of the @Storybook/Angular API, with lots of wacky, "magic" things (auto-generated stories and knobs, Angular decorator consumption, template functions, better component composition, etc.). So I'm happy to keep the base API relatively sane.

Incidentally, another possibility that occurred to me is:

storiesOf(['My App', 'Buttons', MyAwesomeButtonComponent], module)

That possibly fits more cleanly with the idea of the Storybook doing a .split('/') on the string param, and doesn't require overloading with a variable number of parameters.

[shilman]

The first proposal is about backwards compatibility. When we release a new API we'd still want to support the old way for at least one major version.

Re: terseness the "X|Y/Z" format actually communicates two things: the root grouping as well as the folder grouping. The array format loses that, unless we made it a convention that there's always a root grouping (which I'm ok with)

I like the idea of a wrapper package. It would be a good way to experiment freely and then migrate the best ideas into core. That's what we've been doing with the docs/story format as much as possible.

[tmeasday]

I think @jonrimmer's idea of storiesOf('root|group', Component, module) is pretty nice and seems backwards compatible, assuming we can easily distinguish between a Component and a module.

I actually did a similar PR to this for React way back when (can't recall why we didn't ultimately merge it) so I am very sympathetic to the approach.

[jonrimmer]

The code in this PR should be backwards compatible since it checks the number of arguments and their types. The runtime type checking is relatively minimal — anything that's typeof function is assumed to be an Angular component class — but the TypeScript checking is stricter. I'd be amazed if there's anyone using Angular storybook who isn't also using TypeScript.

There are ways to be more certain at runtime whether something is an Angular class (or an Angular NgModule or directive) but they require relying on private Angular APIs, which is something I'm a bit uncomfortable with doing, particularly since these APIs are going to change when Angular's new Ivy renderer arrives. Although on the plus side, that will supposedly introduce public APIs for getting component metadata, which will be a huge convenience for tools like Storybook.

[tmeasday]

Sure @jonrimmer but I guess I am thinking of the bigger picture than Angular; I think we should have a consistent API for all view layers.

Thinking it over again, I don't think we can rely on being able to distinguish a module from any possible "component" implementation (in any view layer) (or do others disagree?). So I wonder if we make things more explicit:

storiesOf('name', module); // <- two args has to be a module

storiesOf('name', Component, module); // <- three args is always this form

// OTOH, if we want things (eg. `module`) to be optional, let's make them named.
storiesOf({ group: 'root|group', Component, module }); 

What do you think @jonrimmer? Keep in mind we should also be thinking about the new module-based API, which might look something like:

export componentMetadata = {
  Component,
  group: 'root|group',
  module,
};

So naming the arguments is a sensible step in that direction.

[jonrimmer]

@tmeasday I was envisaging this as an Angular-only thing. The problem is, Angular's API is so radically different from React's (and other frameworks). And since Storybook's API was essentially designed for React, it does not fit very well with Angular. Certainly, if you were designing a Storybook-like tool for Angular from scratch, the API would not look like it does at present.

While there's a value in keeping the API consistent between different framework apps, I personally feel it should be a higher priority to make the experience of using Storybook with Angular, or whatever framework, as smooth and comfortable as possible. If that means having some differences in how the core functionality is exposed in different frameworks, then that's an acceptable tradeoff.

That said, I'm not as familiar with Storybook for React as I am with Angular. My understanding was that there is no need to "declare" a React component prior to using it, as there is with Angular. In your example, what do you imagine Storybook will do with the component you pass to storiesOf()?

[codecov]

Codecov Report

Merging #6350 into next will increase coverage by 0.03%.
The diff coverage is 58.82%.

@@            Coverage Diff            @@
##            next    #6350      +/-   ##
=========================================
+ Coverage   38.3%   38.33%   +0.03%     
=========================================
  Files        649      649              
  Lines       9853     9870      +17     
  Branches     388      388              
=========================================
+ Hits        3774     3784      +10     
- Misses      6019     6026       +7     
  Partials      60       60

Impacted Files	Coverage Δ
app/angular/src/client/preview/index.js	65% <58.82%> (-35%)	⬇️

---

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 37f8d33...559b09b. Read the comment docs.

[codecov]

Codecov Report

Merging #6350 into next will increase coverage by 0.03%.
The diff coverage is 58.82%.

@@            Coverage Diff            @@
##            next    #6350      +/-   ##
=========================================
+ Coverage   38.3%   38.33%   +0.03%     
=========================================
  Files        649      649              
  Lines       9853     9870      +17     
  Branches     388      388              
=========================================
+ Hits        3774     3784      +10     
- Misses      6019     6026       +7     
  Partials      60       60

Impacted Files	Coverage Δ
app/angular/src/client/preview/index.js	65% <58.82%> (-35%)	⬇️

---

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 37f8d33...559b09b. Read the comment docs.

[shilman]

@jonrimmer Sorry hit the "close" button by accident. There's a lot we can do with the React component if we have access to it. The most immediate use is extracting PropTypes or docgen information from it automatically for something like addon-info or the upcoming Storybook Docs.

[jonrimmer]

@shilman OK cool, it makes sense to standardise an approach then. I have a mild preference for allowing the two arg storiesOf(Component, module) version and requiring the framework implementation to be able to distinguish between a node module and a component, with the named params version available as a fallback. But I can live with the three-arg / named-params approach as well. Happy to go with the consensus here, if you want to break the tie ;-).

[ndelangen]

@jonrimmer There seems to be an error in a test:
https://circleci.com/gh/storybooks/storybook/114673

ERROR in /tmp/storybook/lib/cli/test/run/angular-cli-v6/node_modules/@angular/core/src/render3/ng_dev_mode.d.ts
ERROR in /tmp/storybook/lib/cli/test/run/angular-cli-v6/node_modules/@angular/core/src/render3/ng_dev_mode.d.ts(9,11):
TS2451: Cannot redeclare block-scoped variable 'ngDevMode'.

Could you take a look at this?

[ndelangen]

Is this related: ?
#6346

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[stale]

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!