Angular: Unsubscribe prop subscriptions

[Marklb]

Issue: #12507

When a prop was set for an EventEmitter type, the EventEmitter would be subscribed to. The problem was that each time setProps was called, a new subscription was created without maintaining a reference to that subscription.

In the following example, every time the label control changed, the onClick EventEmitter would be subscribed to. Since new subscriptions were being created, the props function would get called once for each subscription. So if label changed 2 times, then you clicked the button, there would be 3 actions (initial subscription, first label change subscription, second label change subscription), but there should only be one action.

// Component
@Component({ template: '<button (click)="onClick.emit($event)">{{ label }}</button>' })
class ButtonComponent {
	label: string
	onClick = new EventEmitter<Event>()
}

// Story
export const Example = (args) => ({
	props: {
		...args,
		onClick: action('I should be called once per click')
	}
})
Primary.args = {
  label: 'Button'
};

What I did

Kept a reference to the subscriptions to try and make sure they get unsubscribed. I wrote it to work for any Observable, but it should only be used for EventEmitter props currently.

When setting a prop for an EventEmitter:

• If a subscription for that prop key hasn't been created.
  • Create a subscription to call the prop value. Store a reference to the props value and the subscription for that prop key.
• If a subscription for that prop key has been created.
  • If the props value hasn't changed.
    • Do nothing.
  • The the props value has changed.
    • Unsubscribe the previous subscription and create a subscription for the new value.

How to test

Set a value for an for an EventEmitter prop, then do something that updates props, such as changing a control. A new subscription should not be created if the value of that prop didn't change. If the value of that prop did change, the previous subscription should have been unsubscribed.

• Is this testable with Jest or Chromatic screenshots? Should be, but don't see any other tests for that component, so I am not sure how to set it up yet.
• Does this need a new example in the kitchen sink apps? No
• Does this need an update to the documentation? No

[shilman]

Great stuff @Marklb -- thanks for diagnosing this and fixing it! 🔥

[brandonpittman]

@shilman Tried twice to upgrade to this version. After doing so I started getting TypeScript errors I don't get with .21 and if I comment out my own code to get those errors to temporarily stop, Storybook errors in the browser with an invalid hook call error and something in the Manager Provider.

Each patch upgrade is a dice roll with @storybook/angular.

[shilman]

@brandonpittman really sorry about that and thanks for letting me know. i'll hold off on patching angular stuff then and go through the full 6.1-alpha/beta/rc process instead. did you try the standard remove node_modules/refresh lock? i'm guessing this is the only angular-specific change and i don't think it's causing any of the errors you mentioned. and i'm not going to stop issuing patches for other issues. :(

[brandonpittman]

Yeah, I did the usual rm node_modules package-lock.json twice. 😞

I know Angular's a pain in the ass. I appreciate your team supporting it at all. If my boss wasn't so dedicated to Angular, I'd force a change to React.

[Marklb]

I would be surprised if this caused an error like that, but I get errors every time I upgrade Angular, so I wouldn't say I am sure it wouldn't either.

I hadn't updated Storybook from 6.0.21 in my projects yet, but just upgraded to 6.1.0-alpha.18. It seems fine so far in my library project, which is my most complicated Angular project. I did have to delete node_modules and package-lock.json though.

Did you also upgrade Angular or just Storybook?

Last weekend I upgraded the main angular packages from 10.0.9 to 10.1.2, @angular-devkit/build-angular and @angular-devkit/build-ng-packagr from 0.1000.6 to 0.1001.2, ng-packagr from 10.0.0 to 10.1.0, and typescript from 3.9.5 to 4.0.2. There were a decent amount of problems, but nothing very unexpected. I did have to fork 2 libraries that haven't been merging the fixes that finally went from warnings to errors.

The problems I got were things I knew how to fix though, like having to use import type { for a lot of my interface imports and a few custom things where I was importing/accessing undocumented things that I shouldn't have been using anyway.

[brandonpittman]

@Marklb We're still on Angular 9. Not sure if that's what's causing issues, but .21 is fine. Saw similar issues during the 6.0 beta where anything about .37 just fell apart.