[Angular] Addon Controls resets the component's local variables initialised during Angular life cycle events

[akhilesh85]

Describe the bug
When a field in the Controls is updated it resets the value of the components instance variable which is altered in the Angular lifecycle events

To Reproduce
Steps to reproduce the behavior:

1. Create a component with @input and an instance variable with a default value
2. Update the instance variable in ngOnInit with a new value.
3. Go to the storybook controls tab and alter the @input entry.
4. The controls will reset the instance variable value to its default value when any of the control value changes.

Expected behavior
The changes in the @input variables should not impact or alter the component's instance variable values.

Screenshots

Code snippets
1-Label.stories.ts.txt
label.component.ts.txt

System:
System:
OS: Windows 10 10.0.18362
CPU: (4) x64 Intel(R) Core(TM) i5 CPU M 460 @ 2.53GHz
Binaries:
Node: 12.16.1 - C:\Program Files\nodejs\node.EXE
npm: 6.13.4 - C:\Program Files\nodejs\npm.CMD
Browsers:
Edge: 44.18362.449.0
npmPackages:
@storybook/addon-actions: ^6.0.0-rc.11 => 6.0.0-rc.11
@storybook/addon-controls: ^6.0.0-rc.11 => 6.0.0-rc.11
@storybook/addon-docs: ^6.0.0-rc.11 => 6.0.0-rc.11
@storybook/addon-links: ^6.0.0-rc.11 => 6.0.0-rc.11
@storybook/angular: ^6.0.0-rc.11 => 6.0.0-rc.11

[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!

[mmacret]

We encounter the exact same issue in our project. It is a show blocker for us as a lot of our components initialize internal variables in ngInit.

[tmeasday]

@akhilesh85 @mmacret

So the issue here is that the component re-initializes when an arg is changed? It's interesting that seems to be the opposite behaviour to what we were seeing in #11614.

I don't really know enough about angular's internals or how @storybook/angular uses them to say exactly what's happening here (although I have recently fixed a similar bug in react). Is there any chance either of you could trace a little into what Angular is doing and see if you can work out what might be going wrong?

[ThibaudAV]

For me the problem comes from the doc addon which returns the local properties of the component with their initial values.

you can see it with a log here in your example :

const LabelStory = (args: Label) => {
  console.log(args);

  return {
    component: Label,
    props: args,
  };

if you are wondering why there is a change between Updated localText -> Original localText
This is because the addition of the properties made here is done before the ngOnInit of the sotry component so the final value will be that of ngOnInit (-> Updated localText)

then at the next property change (using the controls, for example) the component is not reloaded. But only these properties are updated.
And this time the property sent by the doc addon will overwrite the property of the component (->Original localText)

The doc addon at compile time seems to complete the defaultValue value.

"propertiesClass": [
  {
    "name": "localText",
    "defaultValue": "'Original localText'",
    "type": "string",
    "optional": false,
    "description": "",
    "line": 36
  }
],

then it always sends it when you can't modify it and see it in the UI (whereas for the Input it is possible)

I don't know what the right behaviour is 🤷‍♂️
IMHO I think that the doc should not send the propertiesClass only the inputsClass with the default values
if in the story you want to overload a propertiesClass you have to do it manually inside. but don't let it be a default behavior.

but I'm not sure I have all the history and all the possible behaviors.

aannd, I still haven't found out where this feature is in the code. 🙈

@shilman @Marklb @gaetanmaisse what do you think about it ?

[shilman]

I don't know Angular well enough to comment on this. If properties are always internal to the component and not settable from outside, we could easily remove their controls from the table. LMK what you guys think!

[Marklb]

@shilman They aren't always internal, but I don't think their defaultValue from docs should show up in args if they aren't an input(I don't know if the current implementation would support a default in the ArgsTable/Control and only send it's value in args if the control was changed). That would fix this specific case, but the core problem is still there and I don't know exactly how to fix it for every scenario. If you decorate localText with @Input(), in this issue's example, then the same problem would happen. It would be kind of pointless to have a default and set the property in ngOnInit, like in the provided example, but there are plenty of valid reasons to do it, like the default changing based on some condition from an injected value in ngOnInit.

I think the optimal fix would be to not set the property unless the value actually changed, like if controls were the only thing that could change them then it would be nice to only hear back from the addon (with a way to know which arg changed, instead of every arg) if the control actually received user interaction(or whatever interaction causes it to change).

Without a potentially large refactor to how args work, I think the best solution may be to diff the props against their previously emitted value, before setting them on the component, because that would probably be accurate most of the time.

Synchronizing the args value with the component instance's actual value would also be nice for avoiding some of this, but I don't think there is a clear way to do that. One problem would be set inputs without a get, but I already explained that in another issue. I will try to find it and reference my comment there, but it isn't necessarily related to this issue.

It may cause issues that I am not thinking of, but I don't think default values discovered automatically by docs should even be set in args for inputs(it should still be available in argTypes though), because if it is the default, then there should be no reason it needs to be set. It is arguably bad code most of the time, but I have seen and written plenty of inputs with setters that use the input for more than simply setting a value. One example may be an input that accepts a large object that isn't simple to diff (ex: @Input() set thing(value: any) { isValid(value) ? this._thing = value : this.showError() } in that snippet isValid may be doing some long calculation). In an Angular template, Angular would prevent setting the input until the reference changes, but it would be difficult to preserve the reference when using controls, because each time it returns from Storybook's channel the reference be different, I assume.

If we change to the way #13215 renders, then this issue's specific case wouldn't be an issue, but the core problem would not be fixed. Though, it would be a breaking change for users that don't use template stories. We could also make sure that only inputs are set in the current implementation by, but the breaking change would still be the same. There are a few things that would be a little more complicated to implement, but they would probably be rare enough that we could probably document workarounds for those situations, since they aren't relying on Angular at that point.

I will try to give a brief explanation for non Angular users, in case it helps from Storybook's code.
Normally components are going to be used from a template and Angular only allows properties to be set from a template if they are decorated with @input()(or the not recommended inputs property of the @Component({ ..., inputs: [ ... ]})). In Storybook's current implementation for component stories, the component is created in JS, so @Input() doesn't do anything(they can be looked up from the metadata though), and the inputs would just be set like you would set any other property on the class, because that is all they actually are. That is why we manually trigger a change detection tick and call the ngOnChanges lifecycle method after props are set, because Angular only does that automatically when done from a template.

*The following example is a valid situation that would break by forcing props to only set input/output properties. There are plenty of other ways to do the following that are clearly better code, but I am just pointing it out as a valid scenario that can be done with Storybook's current Angular framework.

// content.component.ts

// This is a poorly written component, but assume it is a component that
// renders a scrollbar and status message based on how far the page has
// been scrolled.
@Component({
  selector: 'app-content',
  template: `
    {{ content }}
    {{ status }}
    <progress-bar [percentage]="percent"></progress-bar>
  `
})
class ContentComponent {
  @Input() content: string

  status: string

  set percent(p: number) {
    this.status = `You are ${p}% done.`
    this._percent = p
  }
  get percent() { return this._percent }
  _percent: number = 0

  sub = Subscription.EMPTY
  ngOnInit() {
    this.sub = fromEvent(document, 'scroll')
      .pipe(toPercentageOfContent)
      .subscribe(p => this.percent = p)
  }
  ngOnDestroy() { this.sub.unsubscribe() }
}


// content.component.stories.ts

// I don't want `status` or `percent` to be inputs, because the are managed
// internally.
//
// Assuming this is legacy code that the user has been told to document with
// Storybook, but changes to the component's code are not allowed. I would
// just let props set percent on the component's class instance.
export const Example1 = (args) => ({
  props: args,
  component: ContentComponent
})
Example1.args = {
  percent: 30
}

// If we were to restrict props from setting properties that are not an
// input/output then one possible workaround could be to fake a scroll event
// after the props are set.
// The following may not work exactly like that, but I can't think of something
// compact enough for a small example at the moment.
@Injectable()
class FakeScrollPercentDirective {
  constructor(@Inject(STORY) private data: Observable<StoryFnAngularReturnType>) { }
  ngOnInit() { this.data.subscribe(d => triggerFakeScrollEvent(d.props?.percent)) }
  ngOnDestroy() { /* clean up data subscription */ }
}

export const Example2 = (args) => ({
  moduleMetadata: { providers: [ FakeScrollPercentDirective ] },
  props: args,
  component: ContentComponent
})
Example1.args = {
  percent: 30
}

[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!

[MickL]

@Stale just because an issue has no comments doesnt mean it has been fixed or can be closed

[Luminisc]

Issue is still present with following dependencies:
"@storybook/addon-actions": "^6.1.21",
"@storybook/addon-essentials": "^6.1.21",
"@storybook/addon-links": "^6.1.21",
"@storybook/angular": "^6.1.21",

[ThibaudAV]

yes same in 6.2

maybe this will help you :
#14732 (comment)
if you use compodoc you have to be careful

if the problem is the same 🤔

[Luminisc]

Yes, exactly the same - all properties and even methods are undefined, except listed in args of story.
Thanks for link, will try to use one of fixes.

[shilman]

Yee-haw!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.3.0-alpha.18 containing PR #14769 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.

[DirkHeinke]

With 6.4.8 I still have the problem. In my case I use enums in the template by assigning them to a class variable:

@Component({
  selector: 'app-ui-button',
  templateUrl: './button.component.html',
  styleUrls: ['./button.component.scss'],
})
export class ButtonComponent implements OnInit {
  [...]
  // enums for template
  ButtonType = ButtonType;
  [...]
}

In storybook ButtonType is set to the string "ButtonType" which breaks the template logic. The workaround here is to change the code like this:

  [...]
  // enums for template
  /** @ignore */
  ButtonType = ButtonType;
  [...]

Button in angular with type and ButtonType printed:

Button in storybook with type and ButtonType printed:

[nzacca]

I believe the problem is here:

https://github.com/storybookjs/storybook/blob/next/app/angular/src/client/preview/angular-beta/StorybookWrapperComponent.ts#L77

@ThibaudAV Should this be changed to set the props on _this.storyComponentElementRef instead?

After making this change to my local, my component is appears to properly initialize the Inputs on first load.

[Marklb]

@nzacca That would fix it in certain situations, but break it on others. After fixing most of the tests I have added in my local branch, I noticed the problem is more complicated than I expected.

I am currently trying to fix the problems with setting props by adding tests for every scenario I can think of that I may do in a story. After fixing a few more tests that are essential to be working, in my opinion, I will create a PR to see if my changes should be added. Some can't be simply fixed and I am proposing some potentially breaking or unwanted changes for them, but majority of stories should not be affected by my changes. I think my changes would mostly fix this issue, though.

[nzacca]

@Marklb Thank you for that. Could you provide an example where it breaks? I'd be interested in investigating that scenario in my codebase.

[Marklb]

@nzacca The most basic scenario I can think of, at the moment, would be broken is the following:

import { Story, Meta } from '@storybook/angular';
import { Component, Input, SimpleChanges, OnChanges } from '@angular/core';


@Component({ selector: 'ex-comp', template: `{{ bar }}` })
class FooComponent implements OnChanges {
  @Input() bar = 'initial';

  ngOnChanges(changes: SimpleChanges): void {
    console.log('changes', changes);
  }
}

export default {
  title: 'ExampleA',
  component: FooComponent,
} as Meta;

export const Basic: Story = (args) => ({
  props: args,
});
Basic.args = {
  bar: 'Other',
};

In that snippet the Story "Basic" currently would set "bar" to "Other" and ngOnChanges would be called with { previousValue: undefined, currentValue: 'Other', firstChange: true }, which should be correct.
Add your change and ngOnChanges will get called with { previousValue: undefined, currentValue: 'initial', firstChange: true }, which is wrong. "initial" is the default value and "Other" didn't even get set initially. I incorrectly applied the code change to cause that.
Add your change and ngOnChanges will get called with { previousValue: undefined, currentValue: undefined, firstChange: true }, which is wrong. An initial value "Other" was defined in the story args, but didn't get set initially. The reason is that Storybook builds a template containing your component and the inputs/outputs contained in props will be bound in the template. The context for that template is the wrapper component, so the props need to be set on the wrapper component. Next props change would cause the input to get set to the correct value, but it would be set directly on the component instance. By setting the input directly on the component instance the template binding is bypassed, so ngOnChanges will not get called.

I can probably come up with more if I go through all my tests. If I get a chance tonight, I will at least have a branch with my tests available.

[ThibaudAV]

@Marklb This is a behavior that happens when you use compodoc? but if you don't have it it works as expected? or not?

[ThibaudAV]

arf we just encountered the problem on one of our project. it seems to be a problem of the component mode and not template.
if we add a manual template with the input output it works again

[Marklb]

@Marklb This is a behavior that happens when you use compodoc? but if you don't have it it works as expected? or not?

I had actually made a mistake when I was quickly trying the suggestion, so my description of what would happen was wrong. The example still causes a problem, which is actually more like what I was expecting, so I was able to go into more detail.

arf we just encountered the problem on one of our project. it seems to be a problem of the component mode and not template. if we add a manual template with the input output it works again

Providing a template and manually binding your input would work initially. Each additional props update will update the property twice, Once by the binding in the template and once by Storybook directly on the component instance. That causes problems for me, because I often use @Input() set bar() { ... } when something, such as a recalculation, needs to be done each time the property changes. I prefer setter over ngOnChanges in a lot of situations, because I can detect the change from template or directly on the instance. The problem became very noticeable when I started trying to unit test my stories and my spys were showing that recalculations were happening twice as much as I expected. I was confused debugging my component for a while before realizing there was nothing wrong with my component.

Providing a template can help sometimes, but it also has different problems. The big ones, for my components, are that it updates properties on the component way to eagerly and if I add the binding in the template myself then the input gets set twice. I used to consider providing a template as a way to fallback to a basic story fully controlled by me, when Storybook happens to not support what I am trying to do or has a bug with props. Now I can't because Storybook is updating properties directly on the component instance. Also, if I want to use a pipe on one of my inputs then I need to define the input binding myself in the template, but Storybook would be updating my input without the pipe and I would be updating it with the pipe causing the input to get updated twice with different values each props change. So, one of my proposed changes is to add a Parameter to disable Storybook touching the component instance when a template is provided.

The tests I am adding are being done the same for stories with or without a template, to try and avoid the inconsistency between them.

[shilman]

Ta-da!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.0-alpha.11 containing PR #17156 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.

[shilman]

Good golly!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.16 containing PR #17156 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb upgrade

[famu1hundred]

This is still happening in 6.5.5 even after upgrading to prerelase...

If I have private properties within a component with @input setter/getter functions for another property, changing the private property using the controls in storybook directly works. If I change the public @input property, the component initially changes its value and then resets to its initial state.

@Component({ selector: 'ex-comp', template: `{{ bar }}` })
class FooComponent {
  private _bar: string = 'initial';
  
  @Input()
  get bar(): string {
      return this._bar;
  }
  set bar(value: string) {
     this._bar = value;
  }
}

export default {
  title: 'ExampleA',
  component: FooComponent,
} as Meta;

export const Basic: Story = (args) => ({
  props: args,
});

Basic.args = {
  bar: 'Other',
};

This results in the _bar as a Property in the Controls panel in storybook and bar as an Input in the Controls panel. Changing bar briefly changes the string but then resets after another render happens

The only way I've gotten around this (seems hacky) is by overriding the private property in the template

export const Basic: Story = (args) => {
  return {
    props: {
      ...args,
      _bar: args.bar,
    },
  };
};

[OutOfNutella]

Still happening on 6.5.7 too.

Workaround from @famu1hundred is working, but rewriting all things we do in ngOnInit() from our component seems a bit counter productive

Component:

@Component({
  selector: 'storybook-labels',
  template: `
  <div *ngFor="let label of labelText">
    {{ label }}
  </div>`
})
export default class ButtonComponent implements OnInit {
  @Input()
  legend: any[];
  
  labelText: string[];

  ngOnInit() {
    this.labelText = this.legend.map(label => label.label);
  }
}

Storybook:

const Template: Story<Button> = (args: Button) => ({
  props: {
    ...args,
    labelText: args.legend.map((label => label.label))
  }
});

export const Primary = Template.bind({});
Primary.args = {
  primary: true,
  legend: [{ label: '1'}, { label: '2'}],
};

Without the override of labelText, nothing is displayed inside the component

[petrusgomes]

Same issue on 6.5.10