Add moduleMetdata decorator for supplying common Angular metadata #2959

jonrimmer · 2018-02-11T15:29:37Z

What I did

This PR adds a decorator called moduleMetadata() to @storybook/angular that can be used to supply common metadata to all the Angular stories registered by a call to storiesOf().

How to test

The new decorator has an associated test that is ran as part of the core:

yarn test --core

The angular-cli example has been updated to demonstrate use of the new decorator.

The Angular guide has been updated to describe the use of the existing moduleMetadata parameter on stories, and the use of the new decorator.

codecov · 2018-02-12T14:50:46Z

Codecov Report

Merging #2959 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2959      +/-   ##
==========================================
+ Coverage   37.28%   37.32%   +0.04%     
==========================================
  Files         435      435              
  Lines        9432     9427       -5     
  Branches      926      896      -30     
==========================================
+ Hits         3517     3519       +2     
+ Misses       5336     5335       -1     
+ Partials      579      573       -6

Impacted Files	Coverage Δ
app/angular/src/client/index.js	`0% <ø> (ø)`	⬆️
app/vue/src/server/utils.js	`0% <0%> (-100%)`	⬇️
app/react/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
app/react/src/server/babel_config.js	`0% <0%> (-76.67%)`	⬇️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/api/index.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Array.js	`34.14% <0%> (ø)`	⬆️
...s/actions/src/lib/types/function/createFunction.js	`53.33% <0%> (ø)`	⬆️
...react-native/src/manager/components/PreviewHelp.js	`0% <0%> (ø)`	⬆️
app/angular/src/server/ts_config.js	`38.09% <0%> (ø)`	⬆️
... and 67 more

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 4f545e8...4f135ec. Read the comment docs.

jonrimmer · 2018-02-13T08:25:23Z

I'm not really sure why those CI tests are failing?

igor-dv · 2018-02-13T08:46:43Z

I'll check your PR today =) Thanks !

igor-dv · 2018-02-13T15:31:18Z

examples/angular-cli/src/stories/custom-metadata.stories.ts

@@ -1,4 +1,4 @@
-import { storiesOf } from '@storybook/angular';
+import { storiesOf, moduleMetadata } from '@storybook/angular';


I think better having a separate example of this usage, also because it's necessary to check it in conjunction with addon-knobs.
How will it work with template instead of component prop?

Added. Seems to work fine with template. I don't see any reason why they should affect each other, as this only touches the metadata, and passes other parts of the story through untouched.

igor-dv · 2018-02-13T15:38:21Z

app/angular/src/client/preview/angular/decorators.test.ts

+    })(() => ({
+      component: MockComponent,
+      moduleMetadata: {
+        providers: MockService,


should it be an array?

igor-dv · 2018-02-13T15:41:44Z

app/angular/src/client/preview/angular/decorators.test.ts

@@ -0,0 +1,42 @@
+import { moduleMetadata } from './decorators';


I think we also need to support an arrays concatenation...

for example:

const result = moduleMetadata({ imports: [FooModule], })(() => ({ component: MockComponent, moduleMetadata: { imports: [BarModule], } }));

I would expect to have both FooModule and BarModule

I have modified it to combine rather than override.

igor-dv · 2018-02-13T15:48:20Z

Also, I see "netlify" is failing. DId you manage to run yarn bootstrap without any issues?

jonrimmer · 2018-02-13T19:34:51Z

@igor-dv Yeah, I can run bootstrap without any problems. Not really sure what netlify is, or how I could have broken it? It was the CI CLI step that was failing earlier.

igor-dv

LGTM.
@alterx, do you wanna add your two cents?

igor-dv · 2018-02-15T16:08:37Z

app/angular/src/client/preview/angular/decorators.ts

+export const moduleMetadata = (metadata: Partial<NgModuleMetadata>) => (storyFn: () => any) => {
+  const story = storyFn();
+  const storyMetadata = story.moduleMetadata || {};
+  metadata = metadata || {};


Can the default be introduced in the method signature?

That's how I did it originally, but default params affect undefined or no value, but not null. So I'd need to add a check for null and assignment to {} anyway.

igor-dv · 2018-02-15T16:15:14Z

examples/angular-cli/src/stories/custom-metadata.stories.ts

@@ -64,3 +64,40 @@ storiesOf('Custom ngModule metadata', module)
      },
    };
  });
+
+storiesOf('Common ngModule metadata', module)


Can you please extract it to a separate file? So it will play nicely with #2885 and #2918

I agree, it'd be better to have a separate example that shows:

An example with this decorator (basic usage)

Decorator + Knobs addon

Decorator + Knobs + template

Decorator + knobs + component

We don't have official docs for most of this and we usually point people to the examples so we try to keep as many as we can with all the use cases

I guess I'm not really seeing what value these examples would have? All this decorator does is allow you to declare a set of metadata that is shared amongst your subsequent stories. Whether you're using knobs, template, component, etc. is completely orthogonal to it. Since there is already examples of those things, I'm not sure how duplicating them here would really help understanding it?

Wouldn't it make more sense to modify all the other examples to use this decorator where appropriate, i.e. instead of supplying duplicate metadata, as in custom-pipes and custom-providers? Then the metadata example file could just be:

An example with metadata supplied on the story.

Metadata supplied using this decorator.

Metadata supplied on the story that combines with that from the decorator.

alterx · 2018-02-15T16:38:27Z

I added a small comment regarding the examples, other than that it looks good to me.

Also, let's make sure that this works if we add it as a 'global' decorator (https://storybook.js.org/addons/introduction/#storybook-decorators) instead of using it inside a story (it should work but let's test it, just in case)

# Conflicts: # examples/angular-cli/src/stories/__snapshots__/custom-providers.stories. storyshot # examples/angular-cli/src/stories/custom-metadata.stories.ts

jonrimmer · 2018-02-17T23:22:25Z

@alterx I've added the test, though it feels a little wrong to be testing something that's purely provided by the core module here.

igor-dv

What happened to the stories? Or you didn't finish yet?

igor-dv · 2018-02-18T07:36:46Z

app/angular/index.d.ts

@@ -29,4 +30,5 @@ declare module '@storybook/angular' {
  export function addDecorator(decorator: any): IApi;
  export function configure(loaders: () => NodeRequire, module: NodeModule): void;
  export function getStorybook(): IStoribookSection[];
+  export function clearDecorators(): void;


Adding this here makes it a public API. I think we don't want to expose this method.

jonrimmer · 2018-02-18T09:26:11Z

@igor-dv I've removed clearDecorators from the top level export, although it's still importable from @storybook/angular/client (and always was, if you cast the * export of that module to any). As I understand it, this just how the JS module system works, as there's no concept of "package private" exports.

WRT the stories, I added a reply about this elsewhere but it seems to have gone missing. I'm not sure it makes sense to just have a single example file using this decorator, containing examples with component, template, knobs, etc. The use of this decorator is entirely orthogonal to those use-cases, and I'm not sure they would aid users in understanding it. I think it would be better to use this decorator in all the examples, where it makes sense. E.g. in custom-pipes.stories.ts so you don't have to declare the same metadata twice.

The specific metadata example(s) could then be focussed on demonstrating scenarios relating to metadata only. I.e.

Adding metadata data on individual stories.
Adding common metadata to be shared between several stories.
Adding common metadata, and metadata on an individual story that is combined with it.

I'll update the PR to reflect this approach.

igor-dv · 2018-02-19T14:03:03Z

As I understand it, this just how the JS module system works, as there's no concept of "package private" exports.

Yeah, but if it's not on a top level, IMO we can breaking change easier =)

WRT the stories

Makes sense to me

igor-dv

🍺

Hypnosphi · 2018-02-19T21:23:36Z

Please update integration test screenshot for angular:

cd examples/angular-cli
yarn build-storybook
cd ../..
yarn test --integration --update

jonrimmer · 2018-02-19T23:35:35Z

🎉 🍻 Thanks for all your help guys!

jonrimmer added 2 commits February 11, 2018 15:13

Add moduleMetdata decorator for supplying common Angular metadata

e29d0e7

Merge branch 'master' into angular-modulemetadata-decorator

e8f08e0

danielduan added feature request angular labels Feb 12, 2018

Merge branch 'master' into angular-modulemetadata-decorator

07172c3

igor-dv reviewed Feb 13, 2018

View reviewed changes

jonrimmer added 3 commits February 13, 2018 21:10

Combine rather than replace and add examples

d2bbf3a

Merge branch 'master' into angular-modulemetadata-decorator

37a904c

Merge remote-tracking branch 'storybooks/master'

e8117fd

igor-dv reviewed Feb 15, 2018

View reviewed changes

jonrimmer added 3 commits February 17, 2018 19:04

Merge remote-tracking branch 'storybooks/master'

fc05a7c

Merge branch 'master' into angular-modulemetadata-decorator

648e6e0

# Conflicts: # examples/angular-cli/src/stories/__snapshots__/custom-providers.stories. storyshot # examples/angular-cli/src/stories/custom-metadata.stories.ts

Add test for global decorator usage

1ee3e0a

igor-dv reviewed Feb 18, 2018

View reviewed changes

Remove top level export of clearDecorators

246fc20

Add metadata example stories

76ebe43

igor-dv approved these changes Feb 19, 2018

View reviewed changes

igor-dv and others added 3 commits February 19, 2018 16:05

Merge branch 'master' into angular-modulemetadata-decorator

6dbdb59

Merge branch 'master' into angular-modulemetadata-decorator

770678a

Merge branch 'master' into angular-modulemetadata-decorator

b650ca5

igor-dv and others added 3 commits February 19, 2018 23:37

Merge branch 'master' into angular-modulemetadata-decorator

36bace3

Update Angular integration test screenshot

613b4cb

Merge branch 'master' into angular-modulemetadata-decorator

4f135ec

Hypnosphi merged commit 6335274 into storybookjs:master Feb 19, 2018

jonrimmer deleted the angular-modulemetadata-decorator branch February 19, 2018 23:35

dependencies bot mentioned this pull request Jan 15, 2021

Update 13 dependencies from npm #2555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add moduleMetdata decorator for supplying common Angular metadata #2959

Add moduleMetdata decorator for supplying common Angular metadata #2959

jonrimmer commented Feb 11, 2018

codecov bot commented Feb 12, 2018 •

edited

Loading

jonrimmer commented Feb 13, 2018

igor-dv commented Feb 13, 2018

igor-dv Feb 13, 2018

jonrimmer Feb 13, 2018

igor-dv Feb 13, 2018

igor-dv Feb 13, 2018 •

edited

Loading

jonrimmer Feb 13, 2018

igor-dv commented Feb 13, 2018 •

edited

Loading

jonrimmer commented Feb 13, 2018

igor-dv left a comment

igor-dv Feb 15, 2018

jonrimmer Feb 16, 2018

igor-dv Feb 15, 2018

alterx Feb 15, 2018

jonrimmer Feb 17, 2018 •

edited

Loading

alterx commented Feb 15, 2018

jonrimmer commented Feb 17, 2018

igor-dv left a comment

igor-dv Feb 18, 2018

jonrimmer commented Feb 18, 2018

igor-dv commented Feb 19, 2018

igor-dv left a comment

Hypnosphi commented Feb 19, 2018 •

edited

Loading

jonrimmer commented Feb 19, 2018

		@@ -1,4 +1,4 @@
		import { storiesOf } from '@storybook/angular';
		import { storiesOf, moduleMetadata } from '@storybook/angular';

		@@ -0,0 +1,42 @@
		import { moduleMetadata } from './decorators';

Add moduleMetdata decorator for supplying common Angular metadata #2959

Add moduleMetdata decorator for supplying common Angular metadata #2959

Conversation

jonrimmer commented Feb 11, 2018

What I did

How to test

codecov bot commented Feb 12, 2018 • edited Loading

Codecov Report

jonrimmer commented Feb 13, 2018

igor-dv commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-dv Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-dv commented Feb 13, 2018 • edited Loading

jonrimmer commented Feb 13, 2018

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrimmer Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

alterx commented Feb 15, 2018

jonrimmer commented Feb 17, 2018

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrimmer commented Feb 18, 2018

igor-dv commented Feb 19, 2018

igor-dv left a comment

Choose a reason for hiding this comment

Hypnosphi commented Feb 19, 2018 • edited Loading

jonrimmer commented Feb 19, 2018

codecov bot commented Feb 12, 2018 •

edited

Loading

igor-dv Feb 13, 2018 •

edited

Loading

igor-dv commented Feb 13, 2018 •

edited

Loading

jonrimmer Feb 17, 2018 •

edited

Loading

Hypnosphi commented Feb 19, 2018 •

edited

Loading