Don't set argType.defaultValue in angular/compodoc
#19935
Conversation
|
@IanVS it seems the angular tests are disabled right now, do you know why? I want to fix/check the |
|
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. |
|
@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 No changes should be necessary if want to run the tests in |
|
Thanks @Marklb I appreciate that! It sounds like this is mergeable then, we can merge your other branch when it's ready. |
|
@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: It passes This is problematic, firstly because the way that We could change that, probably by passing the context into the 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 |
|
@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 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
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 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'] })
),
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 @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 @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';
}
}
}
I don't see a reason to change |
|
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 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? |
|
@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: AlwaysDefineTemplateOrComponentexport const AlwaysDefineTemplateOrComponent: StoryFn = () => ({});It does not define a At this point the StoryFn will return Now the decorators need to be called. There are none at the story level, so we will jump to the 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 Now the story renders and the result is as expected. Now the tricky part Ideally you would want to put all 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 By removing Now let's use the control to change the The StoryFn will return Now let's call the decorator. The StoryFn's return object will now be Both the parent 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 The parent renders as '#FFFFFF' and the child renders as '#000000', so it is correct. Now let's change the value of Both child and parent are whatever |
|
Hey @Marklb thanks for those details, let me see if I can chart a path forward here: Renderer generating template / story object
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
So is our angular renderer dynamically changing the template to add a And how does it cope when the prop changes? -- I guess that's what you mention in the second part. CSF3Note in CSF 3, we have a I would expect it would get set by the combination of the I wonder if the template generation should happen in the These storiesI 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(
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 |
|
I do not know if these visual changes are intended? @tmeasday |
|
@ndelangen They aren't. I'll resolve them once the discussion with @Marklb tells me how to do that ;) |
|
@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 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
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 don't think users are typically relying on being able to use the story's component in |
|
@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 |
|
Sorry I haven't responded yet @Marklb I have had a very busy 24 hours but I'll hopefully have time in the morning. |
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.
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.
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? |
|
@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 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 @Component({
template: 'Hello<h1>World</h1>',
...
})
class StorybookWrapperComponent {
...
}
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. |
|
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? |
|
@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. |
|
Ok, merging! Thanks for all your advice/help @Marklb |
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.defaultValuein 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.defaultValueentirely.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).