Don't set argType.defaultValue in angular/compodoc

[tmeasday]

Issue: #17004 #17001 #16959 #16865

Discussion on #19492

What I did

• Back in Addon-docs: Remove all defaultValue eval-ing #14900 (6.3) we stopped setting argType.defaultValue in React/Svelte/Vue(3), but for some reason not Angular (don't think it was intentional). This left the same kind of bugs in angular that we had fixed in the other renderers 🤦

• In 7.0 as planned we are removing support for argType.defaultValue entirely.

• So let's drop default value in Angular too! See the original PR for justification.

How to test

Visit this story: http://localhost:6006/?path=/story/stories-frameworks-angular-argtypes-doc-button--basic

Rather than showing (in the controls panel):

It will show:

However, if the component actually uses the default value (it doesn't but you can tweak it to do so), it is unaffected (that's the whole point).

[tmeasday]

@IanVS it seems the angular tests are disabled right now, do you know why? I want to fix/check the compodoc tests with this change.

[IanVS]

They have been disabled for quite some time. @Marklb made some progress towards getting them passing in the jest 28 PR, but a few still fail.

[Marklb]

@tmeasday I will try to get my branch that fixes most of the Angular tests updated and open a PR today, unless I run into some unexpected problem. I did try the tests in frameworks/angular/src/client/docs/compodoc.test.ts, with this change and they passed.

No changes should be necessary if want to run the tests in compodoc.test.ts only. If you exclude the tests in frameworks/angular/src/server/framework-preset-angular-cli.test.ts then the rest of the Angular tests should also run, but many will fail without the changes to account for what has changed while they have been disabled.

[tmeasday]

Thanks @Marklb I appreciate that! It sounds like this is mergeable then, we can merge your other branch when it's ready.

[tmeasday]

@valentinpalkovic (or someone who knows more about angular), I am not quite sure how to think about the Chromatic changes here. The issue is this decorator:

storybook/code/frameworks/angular/template/stories/basics/component-with-ng-content/ng-content-about-parent.stories.ts

Line 28 in 3fad5a4

(story) => `<sb-button [color]="propsColor">${story}</sb-button>`,

It passes [color]="propsColor" into the sb-button component which has a default value for color. The issue is, that even though propsColor is undefined, the default value does not apply in the component. It seems (although I could be wrong about this, I can't find clear angular docs about it) that this is the intended behaviour in angular, and we'd actually need to change the template (i.e. to <sb-button>${story}</sb-button>) if the prop (/arg) is not set.

This is problematic, firstly because the way that componentWrapperDecorator is implemented, there is no way to change the template based on prop/args values.

We could change that, probably by passing the context into the element() function here:

storybook/code/frameworks/angular/src/client/decorators.ts

Line 43 in 3fad5a4

: element(story.template);

.

However, this seems like a fairly important change for angular, I guess we might need to make it (7.0 is a breaking release) but also, I wonder, what other places is this issue going to bite us? In the other renderers, setting a prop to undefined is equivalent to not setting it at all, but here it seems kind of annoying to have to do this careful writing of templates based on input values.

[Marklb]

@tmeasday You are correct about it being the intended behavior. If a property binding is declared in a template then the binding will always be set during the first change detection tick and only set during the next ticks if the property has changed. So, in this case, the color input is set to undefined during the first tick, because that is the value of props['propsColor'].

Angular is working on some changes that would make dynamically handling the inner content easier, so defining a template wouldn't be necessary for this story, but for now the following is probably the best solutions.

There are a few solutions I would consider to be more accurate than changing the template, but this SbButtonComponent is so simple that it wouldn't really be noticeable which ever solution is used.

1. Set the default value in the story args. Obviously this is defeats the purpose of the default value, but in my opinion the only reason you should put a binding in the template is if you intend to use it.

2. This is probably the most similar to the proposed solution of changing the template. In the following, the sb-button would be rerendered if the ngIf condition changes between true/false, but the content in the ng-template would not rerender. The reason I consider this the best, if you can't change the component, is because if the inner content is a component then it will not be reinitialized, but SbButtonComponent is so simple that reinitializing wouldn't even be noticed visually.

One downside to this is that the code is much longer and doesn't look good if visible in the "Show Code" display, but if I was actually doing this in an Angular app then this is how I would probably do it. I typically only do this for toggling directives, not inputs. Normally I try to just handle the null or undefined in the input, instead.

componentWrapperDecorator(
  (story) => `
  	<ng-template #innerContent>${story}</ng-template>
  	<sb-button *ngIf="propsColor" [color]="propsColor"><ng-container [ngComponentOutlet]="innerContent"></ng-container></sb-button>
  	<sb-button *ngIf="!propsColor"><ng-container [ngComponentOutlet]="innerContent"></ng-container></sb-button>
  `,

  ({ args }) => ({ propsColor: args['color'] })
),

3. In my components I try to assume any input may receive null or undefined and make sure the value is coerced before used, if neccessary. When strict typing is enabled, Angular actually requires the type for all @Input() decorated properties to include null | undefined.

There are multiple ways to do this, but the best way depends on the situation.

The most simple way for this would be the following. It does require the default to be maintained in two places, but that could be avoided by assigning the default value to another property. This is not the most efficient, because color || '#5eadf5' has to be evaluated each change detection tick, but in this case it would evaluate fast enough to not even notice a performance hit and may even be faster than alternatives anyway.

@Component({
  selector: 'sb-button',
  template: `<button [style.background-color]="color || '#5eadf5'"><ng-content></ng-content></button>`,
  styles: [`button {padding: 4px;}`],
})
class SbButtonComponent {
  @Input() color = '#5eadf5';
}

Another option is to use a setter for the input and coerce the value to a valid value before setting the property that will actually be used. I do this a lot, because it is not too much boilerplate. There are reasons this can be a bad solution, which I won't try to explain here, because this component is so simple they wouldn't matter. One main down-side for doing this with components used in Storybook is that Compodoc can't determine a default value to show in the default value column of the ArgsTable when viewing Docs.

@Component({
  selector: 'sb-button',
  template: `<button [style.background-color]="_color"><ng-content></ng-content></button>`,
  styles: [`button {padding: 4px;}`],
})
class SbButtonComponent {
  @Input()
  set color(value) { this._color = value || '#5eadf5' }
  _color = '#5eadf5';
}

Another option is using the ngOnChanges life-cycle hook, which is called if any inputs are set during a change detection tick.

@Component({
  selector: 'sb-button',
  template: `<button [style.background-color]="color"><ng-content></ng-content></button>`,
  styles: [`button {padding: 4px;}`],
})
class SbButtonComponent {
  @Input() color = '#5eadf5';
  
  ngOnChanges(changes: SimpleChanges) {
  	if (changes['color'] && (changes['color'].currentValue === undefined || changes['color'].currentValue === null)) {
  		this.color = '#5eadf5';
  	}
  }
}

---

This is problematic, firstly because the way that componentWrapperDecorator is implemented, there is no way to change the template based on prop/args values.

We could change that, probably by passing the context into the element() function here:

storybook/code/frameworks/angular/src/client/decorators.ts

Line 43 in 3fad5a4

: element(story.template);

.
However, this seems like a fairly important change for angular, I guess we might need to make it (7.0 is a breaking release) but also, I wonder, what other places is this issue going to bite us? In the other renderers, setting a prop to undefined is equivalent to not setting it at all, but here it seems kind of annoying to have to do this careful writing of templates based on input values.

I don't see a reason to change componentWrapperDecorator for this, because there are solutions that can be handled in Angular. If the template string is changed then it would require the story to fully re-render.

[tmeasday]

Thanks for that explanation @Marklb that's super interesting although I am now possibly even more intimidated by angular :). This will help as a resource for folks that run into issues with this.

What do you think about this particular story? I'm thinking that maybe it's not a great example anyway because we are doing something weird by rendering the same component twice, once in the decorator, and once in the story, but then passing the auto-generated color arg (from the story's component) into the decorator's component.

All this logic about default values not being required doesn't quite fit in this case anyway, it's just a coincidence (in a sense) that the same prop exists on the component in the decorator and it's not a pattern users should be doing (passing args that have been inferred from the story's component into a decorator's component).

A better version might just be to define a second arg, with a default value on the component, to be used by the decorator:

export default {
  ...
  // I might rename this to `decoratorColor` too, but this is what it's currently called in the template.
  args: { propColor: '5eadf5 },
  argTypes: { propColor: { control: 'color' } }
}

WDYT @Marklb?

[Marklb]

@tmeasday I have been thinking about it and I agree that these are weird and confusing stories that are not typically something that should be done, but I am still trying to decide if it is worth having as an advanced example. The way it sets the props is the part I see as bad and should not be done, but at the moment the story doesn't work correctly if props are set the way they should.

The short answer is that I think this is a valid scenario that could be better with some minor adjustments, which may require some changes to Storybook's Angular renderer. I explain some of the issues in my breakdown below. If these stories are intended to be simple introductory stories then I don't know if they add anything that isn't already covered in https://github.com/storybookjs/storybook/blob/3fad5a4d89/code/frameworks/angular/template/stories/basics/component-with-ng-content/ng-content-simple.stories.ts and https://github.com/storybookjs/storybook/blob/3fad5a4d89/code/frameworks/angular/template/stories/core/decorators/componentWrapperDecorator/decorators.stories.ts

---

I have a fairly major refactor to the Angular renderer that I am going to submit when tests are re-enabled, which should be soon, and this is a scenario that should be fixed by my refactor.

This is getting into details that don't really have anything to do with this issue's change, which you already mentioned, but I will try to break down my thoughts, by stepping through what is happening when constructing one of these stories.

First story: AlwaysDefineTemplateOrComponent

export const AlwaysDefineTemplateOrComponent: StoryFn = () => ({});

It does not define a template property in the StoryFn's return object. So, Storybook is going to generate a template for the component defined in Meta.

At this point the StoryFn will return { template: '<sb-button></sb-button>', props: {} }. Even if an input/output is set in args there will not be any bindings, because no properties were set in props.

Now the decorators need to be called. There are none at the story level, so we will jump to the Meta decorators. The following is the decorator.

componentWrapperDecorator(
  // `story` is the `template` property of the StoryFn's returned object.
  (story) => `<sb-button [color]="propsColor">${story}</sb-button>`,
  // This second parameter is optional and the return will be merged into `props`.
  ({ args }) => ({ propsColor: args['color'] })
),

After calling the decorator, the StoryFn's return object will be { template: '<sb-button [color]="propsColor"><sb-button></sb-button></sb-button>', props: { propsColor: undefined } }. Since there is a control propsColor in argTypes, if the control is used to change propsColor to '#FFFFFF' then the return object would then be { template: '<sb-button [color]="propsColor"><sb-button></sb-button></sb-button>', props: { propsColor: '#FFFFFF' } }

Now the story renders and the result is as expected.

Now the tricky part

Ideally you would want to put all args in props, so the args aren't ignored in the story. So, the following is how I would expect this story should be defined.

export const AlwaysDefineTemplateOrComponent: StoryFn = (args) => ({ props: { ...args } });

Now all the auto-generated input controls and output actions are considered, without having to update the stories props when an input or output is added to the component.

By removing defaultValue and not manually setting a value for color in args, the story initially renders as intended.

Now let's use the control to change the color arg to '#FFFFFF'. So, args is now { color: '#FFFFFF' }.

The StoryFn will return { template: '<sb-button [color]="color"></sb-button>', props: { color: '#FFFFFF' } }

Now let's call the decorator. The StoryFn's return object will now be { template: '<sb-button [color]="propsColor"><sb-button [color]="color"></sb-button></sb-button>', props: { color: '#FFFFFF', propsColor: '#FFFFFF' } }

Both the parent SbButtonComponent instance and child SbButtonComponent instance are receiving the same input, because the color arg is being mapped to the color and propsColor props.

Well, we should be able to solve that by creating a separate arg for both input bindings, right?

Let's update our file to the following:

export default {
  component: SbButtonComponent,
  decorators: [
    componentWrapperDecorator(
      (story) => `<sb-button [color]="propsColor">${story}</sb-button>`
    ),
  ],
  args: {
    color: '#000000',
    propsColor: '#FFFFFF',
  },
  argTypes: {
    color: { control: 'color' },
    propsColor: { control: 'color' },
  },
} as Meta;

export const AlwaysDefineTemplateOrComponent: StoryFn = (args) => ({ props: { ...args } });

The StoryFn return object will come out to be { template: '<sb-button [color]="propsColor"><sb-button [color]="color"></sb-button></sb-button>', props: { color: '#000000', propsColor: '#FFFFFF' } }.

The parent renders as '#FFFFFF' and the child renders as '#000000', so it is correct.

Now let's change the value of color or propsColor using controls.

Both child and parent are whatever color is and changing the propsColor control appears to have no effect. This is caused a side-effect of Storybook trying to be smart and update input properties on the component instance, even if a binding isn't in the template. The reason for this is that Storybook can dynamically add props, but Angular can't dynamically add bindings, so Storybook tries to copy what an Angular binding would do and manually set the property on the instance. Angular template bindings set instance properties before Storybook sets instance properties. Storybook also only sets properties directly on the first component instance, which is the parent instance in this story.
The refactor, that I will be proposing soon, fixes this by checking if the instance has a binding for the property before setting it and synchronizes properties set by Angular and Storybook to happen at the same time. I won't get into all the details of that here.

[tmeasday]

Hey @Marklb thanks for those details, let me see if I can chart a path forward here:

Renderer generating template / story object

export const AlwaysDefineTemplateOrComponent: StoryFn = () => ({});

At this point the StoryFn will return { template: '<sb-button></sb-button>', props: {} }

When you say "the StoryFn will return" you are being a little bit loose here, I suppose you mean that our angular renderer will take the output of the story function and automatically add a default template to it. (You mention that detail of course).

One question I have about this is, how does that template's bindings get decided? In the above there are no bindings, but then later you have (when the color prop is set):

The StoryFn will return { template: '<sb-button [color]="color"></sb-button>', props: { color: '#FFFFFF' } }

So is our angular renderer dynamically changing the template to add a color binding depending on whether the prop is set, similar to what I proposed the decorator "should" be doing in these stories?

And how does it cope when the prop changes? -- I guess that's what you mention in the second part.

CSF3

Note in CSF 3, we have a render function that automatically sets props, so if the above story was changed to:

export const AlwaysDefineTemplateOrComponent = {};

I would expect it would get set by the combination of the render function and our angular code's magic "template generation" to something similar to { template: '<sb-button [color]="color"></sb-button>', props: { color: '#FFFFFF' } }.

I wonder if the template generation should happen in the render function also? It might be less confusing (but we'd need to figure out how to do that in a back-compatible way for CSF2).

These stories

I guess what these stories are for is testing you can reference the story's component in a decorator, even when you do various things inside the actual story's render function, including using other components + templates.

TBH, this is really weird in my opinion and I am not sure why it is important that you are allowed to do this. I'm sure I am not quite across all the details, but if you want to use a component in a decorator, why doesn't componentWrapperDecorator just provide a way to reference that component? So something like:

   componentWrapperDecorator(
      SbButtonComponent,
      (story) => `<sb-button [color]="propsColor">${story}</sb-button>`,
      ({ args }) => ({ propsColor: args['color'] })
    ),

I also don't think there's a sensible rationale to try and make automatic arg detection work for such usages, it is too far away from the normal use case, people can wire things up manually in such cases, as I talked about above.

---

I think if people are relying on this behaviour of componentWrapperDecorator (accessing the story's component), then we keep these stories, but I'm not inclined to make the arg type stuff work automatically so I'll just drop that, as I had suggested. Does that make sense to you?

[ndelangen]

I do not know if these visual changes are intended? @tmeasday

[tmeasday]

@ndelangen They aren't. I'll resolve them once the discussion with @Marklb tells me how to do that ;)

[Marklb]

@tmeasday When I said "the StoryFn will return", Storybook is generating the template, but I don't think it is necessarily the renderer. The template needs to be generated before calling the decorators, so they can use it. This is where it can generate the template before the renderer, https://github.com/storybookjs/storybook/blob/next/code/frameworks/angular/src/client/decorateStory.ts#L41, but the renderer will also generate a template if it wasn't generated before.

The template bindings are decided based on the properties defined in props. When generating the template, properties of the props object are going to be iterated and if any are an input or output then a binding will be defined on the generated template. If props change and an input/output property is added props, but nothing else changes that would cause a full rerender, then Storybook will manually set the property on the component instance, instead of adding a binding.

CSF3 shouldn't change anything other than automatically setting props to arg, which is what I assume is being done. So, I think it would end up being like the way I suggested the story should be defined, but currently has some issues.

As for adding a component to the decorator to use it, that wouldn't really help with anything. Typically componentWrapperDecorator is just used for wrapping the story component. One way I use it is for adding a wrapper component to modal stories that adds an open modal looking container to put the modal in. Until all decorators have been called, Angular isn't involved at all. componentWrapperDecorator is just a way to modify the template string that will be used as the template of the root Angular component that will be rendered as the Story component. Any thing registered in the Story's NgModule is usable in the template string returned from the first arg of componentWrapperDecorator. If you want Storybook to automatically generate a template for the component then you can pass it as the first argument, instead of a function. I think I was slightly wrong in my thinking about how the second argument works, but not for way it is used here. If you pass a component, instead of a function then it would use the properties returned by the second arguments to generate the bindings in the template string then merge them into props. componentWrapperDecorator doesn't generate a template if the first argument is a function, because the function returns a template.

I also don't think there's a sensible rationale to try and make automatic arg detection work for such usages, it is too far away from the normal use case, people can wire things up manually in such cases, as I #19935 (comment).

I am not really sure what you mean here. If you are referring to the issue I mentioned about Storybook updating the input properties on the component defined decorator, even though a binding wasn't defined in the template returned by the decorator, I could get more into why that is happening, but I don't think it is too important for this issue. The refactor I will hopefully be proposing soon does add a way to disable updating properties that haven't been bound in the template.

I think if people are relying on this behaviour of componentWrapperDecorator (accessing the story's component), then we keep these stories, but I'm not inclined to make the arg type stuff work automatically so I'll just drop that, as I had suggested. Does that make sense to you?

I don't think users are typically relying on being able to use the story's component in componentWrapperDecorator, but there is no reason they wouldn't be able to. One of the cases I can think of that a user may want to use the Story's component in the componentWrapperDecorator would be something like a tree list component's item component. If items can be the content of other items to render as parent or child then stories for the item as a child may want to use componentWrapperDecorator to wrap each story's template with a parent component of the same type. The issue I mentioned about setting properties directly on the component instance, where there isn't a binding, would be an issue currently, but this isn't a very common situation. It is one reason my refactor would add a way to disable that behavior though.

[Marklb]

@storybookjs/angular Anyone else have an opinion on the stories in https://github.com/storybookjs/storybook/blob/3fad5a4d89234ffec0f406c72cb4f3235e492d92/code/frameworks/angular/template/stories/basics/component-with-ng-content/ng-content-about-parent.stories.ts or something to add about them. I think it is a sort of confusing story configuration that doesn't add anything that isn't already covered in https://github.com/storybookjs/storybook/blob/3fad5a4d89/code/frameworks/angular/template/stories/basics/component-with-ng-content/ng-content-simple.stories.ts?rgh-link-date=2022-11-25T08%3A26%3A27Z and https://github.com/storybookjs/storybook/blob/3fad5a4d89/code/frameworks/angular/template/stories/core/decorators/componentWrapperDecorator/decorators.stories.ts?rgh-link-date=2022-11-25T08%3A26%3A27Z

I think I understand the point, which is that the story's component is used in the decorator template that wraps each story. Doing that causes each story to always render the story's component and optionally allow the story to specify the component's ng-content by setting a template at the story level. Looking at the code, it took me a while to realize that was the point. I looked at the AlwaysDefineTemplateOrComponent story and saw it as a problem that the component instance Storybook sets properties on is the instance in the decorator's template, because I always assume whatever is defined at the story level as the component the story is about, but in this situation it is actually correct for the instance in the decorator to be the one updated.

[tmeasday]

Sorry I haven't responded yet @Marklb I have had a very busy 24 hours but I'll hopefully have time in the morning.

[tmeasday]

Typically componentWrapperDecorator is just used for wrapping the story component.

Any thing registered in the Story's NgModule is usable in the template string returned from the first arg of componentWrapperDecorator

I guess this is my point, because I'm not really seeing why the decorator wouldn't just be explicit about what components it wants to use in the template, rather than automatically being able to use the components the story uses. Typically a decorator wouldn't render the same components as the story, that's part of what makes these particular stories so confusing (a button wrapped in button is not really something you would ever want to do in reality).

It seems to me the point of these stories is to test that feature, but I'm not quite getting why that feature exists in the first place. But if it is a useful/important thing we want to keep, then it makes sense to keep these stories.

If items can be the content of other items to render as parent or child then stories for the item as a child may want to use componentWrapperDecorator to wrap each story's template with a parent component of the same type.

This use case makes sense. But recursive component usage like this is so rare, I don't see why we wouldn't just make the user declare the component twice; once on the story and once on the decorator.

I also don't think there's a sensible rationale to try and make automatic arg detection work for such usages, it is too far away from the normal use case, people can wire things up manually in such cases, as I #19935 (comment).

I am not really sure what you mean here. If you are referring to the issue I mentioned about Storybook updating the input properties on the component defined decorator, even though a binding wasn't defined in the template returned by the decorator, I could get more into why that is happening, but I don't think it is too important for this issue. The refactor I will hopefully be proposing soon does add a way to disable updating properties that haven't been bound in the template.

What I mean is that these stories as written rely on arg type inference on the component used by the story to lead to args that are then consumed by the decorator.

As I discussed above, it's unusual to use the same component in decorators as stories, so arg type inference is not really useful for decorators (as it acts on the story's component). You can use args in decorators, but you need to manually define the arg types yourself, not rely on inference.

This whole PR is about changing what the automatic arg inference does so the fact that it might break decorators that are relying on inference seems (to me) to be a non-issue (as decorators shouldn't/wouldn't be relying on it anyway). Am I making sense?

[Marklb]

@tmeasday As far as I can tell, the visual changes Chromatic is showing for these stories is the expected result. So, maybe they are fine and don't need a change. While they are confusing when first looking at them, there is nothing wrong with what is being done and may be useful for someone that is needing to do that. I agree that a button in a button isn't very useful, but the concept of a component with a child component of the same type is useful. Those stories aren't about a button, so the same concept could be shown without using a button, but I don't think that really matters.

I guess this is my point, because I'm not really seeing why the decorator wouldn't just be explicit about what components it wants to use in the template, rather than automatically being able to use the components the story uses. Typically a decorator wouldn't render the same components as the story, that's part of what makes these particular stories so confusing (a button wrapped in button is not really something you would ever want to do in reality).

I could be wrong, but I think it probably makes more sense to Angular developers, because I don't remember seeing anyone ask about that before. The decorator is only a convenience to modify the template of the story. It is not creating a new component for the decorator, so technically the string returned by the decorator replaces the template defined in the story or a previous decorator.

As an example, the following two stories are equivalent:

export const StoryA = () => ({ template: `Hello<h1>World</h1>` })

export const StoryB = () => ({ template: `World` })
StoryB.decorators = [componentWrapperDecorator((story) => `Hello<h1>${story}</h1>`)]

For both StoryA and StoryB, Storybook will create a component like the following and render it as the root of the Angular application.

@Component({
	template: 'Hello<h1>World</h1>',
	...
})
class StorybookWrapperComponent {
	...
}

I don't see why we wouldn't just make the user declare the component twice; once on the story and once on the decorator.

Angular doesn't allow a component to be declared by two NgModules. For two NgModules to use the same declared components, an NgModule would declare and export the component and the other two NgModules would import the one exporting the declared component. Storybook actually has to look through the NgModules imported by the story and only declare the Story's component in the NgModule it creates if the component hasn't already been imported by an NgModule the user imported.

[tmeasday]

OK, I guess it's fine that this utility exists in the current form if it makes sense to Angular folks and it isn't causing issues.

So you think we should accept the changes as-is and merge this PR @Marklb?

[Marklb]

@tmeasday I don't see any problem with this. Also, last night updated my test fixes branch and tried this change with the tests and none of the failing tests were related to this change.

[tmeasday]

Ok, merging! Thanks for all your advice/help @Marklb