Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Action args emit wrong handler #12854

Closed
profanis opened this issue Oct 21, 2020 · 3 comments
Closed

The Action args emit wrong handler #12854

profanis opened this issue Oct 21, 2020 · 3 comments

Comments

@profanis
Copy link
Contributor

Describe the bug
In an Angular component, the Actions display the name of the event handler that is being invoked from the template, and not the actual @Output() event name

Steps to reproduce

I have created an Angular component that emits the event toggle on every state change. The event is emitted by the method onChange() as seen in the code below:

For clarity, I have removed lines of code from the snippets, and I kept only the required parts

<input  type="checkbox"  
        (ngModelChange)="onChange($event)"  />
<span>{{ label }}</span>
import { Component, EventEmitter, Output } from '@angular/core'

@Component({
  selector: 'app-toggle',
  templateUrl: './toggle.component.html',
  styleUrls: ['./toggle.component.scss'],
})
export class ToggleComponent {
  @Output('toggle') _toggle = new EventEmitter<boolean>()

  onChange = (value: any) => {
    this._toggle.emit(value)
  }

}

Story

The following code is for the story:

export default {
  title: 'Essential/Toggle',
  component: ToggleComponent,
} as Meta;

const Template: Story<ToggleComponent> = (args: ToggleComponent) => ({
  component: ToggleComponent,
  props: args,
  moduleMetadata: {
    declarations: [ToggleComponent]
  },
});

Expected behavior
I expect to see in the Actions the toggle event, but instead, I see the onChange method that is being triggered from the template. (screenshot below)

Screenshots
image

Additional context
I am not sure if I use the Actions in the wrong way, but I would expect to see the @Output() events and not the method names that handle the template events.

I tried several ways, and I managed to have the correct event in the Actions, but this is not described in the docs. An example is shown in the code below:

export default {
  title: 'Essential/Toggle',
  component: ToggleComponent,
  argTypes: { toggle: { action: 'toggle' } },
} as Meta;

const Template: Story<ToggleComponent> = (args: ToggleComponent) => ({
  component: ToggleComponent,
  props: args,
  moduleMetadata: {
    declarations: [ToggleComponent]
  },
  template: '<app-toggle  (toggle)="toggle($event)"></app-toggle>'
});

and this is the screenshot of the Actions

image

Thank you for your time!

@Marklb
Copy link
Member

Marklb commented Oct 28, 2020

This is related to two potential changes.

  1. Auto matched actions in the Angular framework.
  2. Aliased inputs/outputs.

The two are independent changes, but 1 is the "action" issue and 2 is the "action name" issue.

I assume you are using the argTypesRegex configuration mentioned in the documentation, which is using a property name regex matching, so I wouldn't expect the aliased name to show, since it is decorating a different property with nothing to programmatically relate them.

Auto Generated Actions

I don't see actions being set for the output argTypes identified by compodoc, which is what I would expect.

Also, the Storybook documentation here shows a snippet that sets parameters: { actions: { argTypesRegex: '^on.*' } }. It look like this is how actions should be configured in Angular, which is actually correct right now, if I am right that actions aren't automatically being set for outputs identified by compodoc. Should the docs be changed when this is fixed to make the argTypesRegex look more like an extra, framework independent, feature? One reason I say that is, because it currently goes against the Angular style guide. I had set argTypesRegex: "^on.*" in my preview.ts, when I was upgrading to Storybook 6, but actions have never automatically worked for me, since I try to follow the Angular style guide.

Aliased Input/Output

I know I have discussed this on Discord and thought there was also an issue, but I don't see it. So, I must have only discussed it on there.

Storybook's Angular framework doesn't map aliased inputs/outputs right now. It is not difficult to do and I have a local branch that I quickly implemented it on, but there were some reasons I didn't submit it as a PR yet. I will explain some of the reasons below.

The following is not directly related to this issue, but explains why I haven't submitted a PR with my aliased Input/Output handling change.

The confusion is because of how Storybook uses Angular components. Aliased Inputs/Outputs are basically just for using components in a template, which is how a component will primarily be used in an Angular app, but sometimes components are created in js for more advanced features that can't be done in a template. Angular exposes functions to help create a component from it's factory in js, but they don't provide any utilities to help you do the mapping. I hardly ever alias my inputs/outputs, because I want the names to match when looking at my components used in a template and their underlying js class.

Storybook is creating the components in js, so it has to deal with the inputs/outputs on it's own, where it has to look up the mapping on the component's factory.

There are two issues with that as well.

  1. Storybook creates the argTypes, which are used to automatically create actions and create the ArgsTable, before the Story is rendered, since it passes that information into the renderer in the props. If the component's factory is needed for that mapping, there isn't really an api to access that information, unless the compiled factory symbols that contain that information don't change much across versions.

  2. Storybook passes the props as js, so does it make sense to use the name you would use in a template or the type-safe js name, since we are working in Typescript and would have to use the actual property name that exists on the class to have type-safe code and that is also what you would have to do if interacting with that component instance from a ComponentRef<C> in your application.

This problem may be more difficult for me to see a clear solution, because I use components from Typescript in my apps more than I probably should. That is one of the reasons I have refactored almost all my components that were using an aliased input/output. Can't really avoid it when writing dynamic json-schema driven features though.


The more I type, the more I think of, so I will just end the wall of text there and add a working example with some stories that don't have a clear solution to this problem, in my opinion. The main thing is that we are mixing Angular into plain javascript and this is a problem that Angular can deal with at compile time and we are attempting to emulate in javascript. Someone like myself that is used to dealing with Angular components in javascript is probably going to assume template features, like aliased inputs/outputs, is done right at the point they are being assigned, but someone that only ever interacts with components in a template may assume Angular's template "magic" is done much earlier.

/**
 * This code is not good, but I tried to simplify a mix of
 * scenario's that are similar to things I have seen or written
 * in my apps.
 */


/**
 * This may be some service wrapping a non-angular library or endpoint.
 */
@Injectable()
class ExampleStore {

  private _activeSubject = new BehaviorSubject<boolean>(false)
  private _pendingSubject = new BehaviorSubject<boolean>(false)

  public active = this._activeSubject.asObservable().pipe(
    tap(() => this._pendingSubject.next(true)),
    // Fake delay that may be caused by some 3rd party library or server call.
    delay(2000),
    tap(() => this._pendingSubject.next(false)),
    startWith(false),
    shareReplay({ bufferSize: 1, refCount: true })
  )

  public pending = this._pendingSubject.asObservable()

  public setActive(active: boolean): void {
    console.log('setActive', active)
    this._activeSubject.next(active)
  }

}

@Component({
  selector: 'example-component',
  template: `
    <div class="pending-spinner" *ngIf="pending | async">Pending...</div>
    Active: [{{ active | async }}]
  `,
  providers: [ ExampleStore ]
})
class ExampleComponent implements OnChanges {

  @Input('active') isActive: boolean

  public readonly active: Observable<boolean>
  public readonly pending: Observable<boolean>

  constructor(public _store: ExampleStore) {
    this.active = this._store.active
    this.pending = this._store.pending
  }

  ngOnChanges(changes: SimpleChanges): void {
    console.log('changes', changes)
    if (changes.hasOwnProperty('isActive')) {
      // Coerce 'active' input from template
      const _value = changes['isActive'].currentValue
      const value = _value != null && `${_value}` !== 'false'
      this._store.setActive(value)
    }
  }

}

//
// Story example scenarios
//

/**
 * Simple example for current implementation.
 */
export const Example1: Story = (args) => ({
  component: ExampleComponent,
  props: { isActive: true }
})

/**
 * This example uses a knob to set a prop used in the story, but the knob us
 * using a prob that is the same name as a property on the component.
 *
 * Since, this story defines a template, we are managing the
 * prop-to-input/output manually, so it doesn't matter what our prop names are.
 */
export const Example2: Story = () => ({
  moduleMetadata: { declarations: [ ExampleComponent ] },
  props: {
    active: boolean('Info Active', false),
    isActive: true
  },
  template: `
    <example-component [active]="isActive"></example-component>
    <ng-container *ngIf="active">
      Extra non-component info to show on the story
      and toggled with a knob.
    </ng-container>
  `
})


/**
 * This is pointless in this example, but let's say there is a decorator that
 * reads the `active` prop.
 *
 * In this current implementation, this breaks our component by overwritting the
 * observable with a boolean, because of how props are set in Storybook's
 * Angular framework, but that isn't what we are discussing right now. It's fix
 * may depend on this issue's fix.
 */
export const Example3: Story = () => ({
  component: ExampleComponent,
  props: {
    active: boolean('Info Active', false),
    isActive: true
  }
})

/**
 * In this example, we will try and be type-safe with our args. Unless we define
 * some custom args for our story, the args should only be our component
 * inputs/outputs.
 *
 * This example shows why we can't use the mapped names when defining the Story,
 * unless we break the ability to be type-safe in order to support some "magic"
 * that Angular only does in templates.
 */
export const Example4: Story<ExampleComponent> = (args: ExampleComponent) => {
  args.isActive = true
  // We aliased 'active' as the name for our `isActive` input in templates, but
  // our class actually has a property named 'active' that is a readonly Observable.
  args.active = false // Error: Cannot assign to 'active' because it is a read-only property.

  return {
    component: ExampleComponent,
    props: args
  }
}

@ThibaudAV
Copy link
Contributor

@profanis With the latest version of storybook aliased Input/Output is supported.
if you can test and close this issue if it's resolved it would be good :)

FYI :

Yo-ho-ho!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-alpha.2 containing PR #13215 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.

@profanis
Copy link
Contributor Author

profanis commented Dec 7, 2020

@profanis With the latest version of storybook aliased Input/Output is supported.
if you can test and close this issue if it's resolved it would be good :)

FYI :

Yo-ho-ho!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-alpha.2 containing PR #13215 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.

@ThibaudAV I tested it and it works like a charm! :)
I am closing the issue

@profanis profanis closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants