[addon-knobs] addon forces story to be recreated on every knob change

[andrei-ilyukovich]

Bug or support request summary

Addon-knobs for Angular works in a very strange manner that impacts story loading performance.

1. Every time I change some knob, the whole story is reinitialized like during story first loading (I assume that based on initializing of modules needed by story, changing value in knob causes modules to be recreated)
2. Stories with added knobs are initialized twice during first opening(some stories even could open with flickering because of that)

Steps to reproduce

You can open browser console for this page:
https://storybooks-angular.netlify.com/?selectedKind=Addon%7CKnobs&selectedStory=All%20knobs&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs&background=%2300aced

There should be "constructor" log message which logged by AllKnobsComponent. Then try to open another story then get back to "All knobs" again - you'll see double "constructor" message. Also changing any value in knob panel forces AllKnobsComponent to be recreated

Expected behavior

Changing values in knobs should only cause change detection for already loaded component without re initialization of component itself

Please specify which version of Storybook and optionally any affected addons that you're running

"@storybook/addons": "^4.0.0-alpha.18",
"@storybook/angular": "^4.0.0-alpha.18",
"@storybook/addon-knobs": "^4.0.0-alpha.18"

[andrei-ilyukovich]

Has anybody some thoughts about that issue?

[ndelangen]

Changing values in knobs should only cause change detection for already loaded component without re initialization of component itself

Yeah I understand that's the assumptions, I'm interested in supporting it. @igor-dv would you have an idea how to change this?

[igor-dv]

There is the same problem (also an issue) with vue. Knobs became a framework agnostic, and probably it doesn't play well with angular/vue life-cycles. Maybe we need to return back the addon-knobs/angular and addon-knobs/vue, while other frameworks that don't need this special treatment will use the common one...

CC @Hypnosphi

[Hypnosphi]

I think it can still be solved on app/* level. We just need to update the data instead of recreating instances when forceRender === true. Both Vue and modern Angular use some kind of virtual-dom-like mechanism, so it should be possible with those

[igor-dv]

Do you have any specific changes in mind?

[ndelangen]

@igor-dv I think @Hypnosphi is unto something.

I don't have a clear idea what the code would look like.

[andrei-ilyukovich]

Is there any chance to have that fixed in near future?

[wgrabowski]

It's extremely annoying especially when using knob with slider

[jonrimmer]

I just tried using Storybook v5 in an Angular project and the performance with knobs is so atrocious I'm reconsidering using it at all. 2 - 3 second pauses for every keypress I type something into a knobs text field.

The knobs addon is not "framework agnostic" if its design is fundamentally incompatible, performance-wise, with Angular and Vue. How did a patch with such a massive, massive performance regression even land?

Needless to say, I'm very disappointed, and wondering why I wasted time trying to improve the Angular side of Storybook in the past. If Storybook isn't intended for use with anything other than React, then those other frameworks should be removed from the website and docs so as to not keep misleading people.

[CodeByAlex]

Hey @jonrimmer, looks like there is a lot of talk about this performance issue, especially when using angular. I have a PR open that should help increase performance here: #5811

@shilman , could we take a look at merging this in. It hasn't received any feedback yet but follows what @tmeasday and I were solutioning here: #4740 (comment)

@andrei-ilyukovich, @wgrabowski

[jonrimmer]

@CodeByAlex Yeah I saw that. Debouncing should help disguise the problem somewhat, but the fundamental problem would remain: Angular is not a framework where you just re-render the entire component tree whenever something changes. This an approach that is 100% perfect for React, and 0% perfect for anything that does not work in the same way. Knobs should never have been rewritten to work like this, and it absolutely baffles me that it was. So long as this broken approach is kept, issues like this will keep popping up.

Nothing has changed since @igor-dv's comment 5 months ago. Knobs cannot work in a framework agnostic way. It needs to understand how the lifecycle / change detection process of Angular (and Vue and whatever else) works and how it can notify them when props change.

[CodeByAlex]

@jonrimmer I agree that the current force-rendering approach is not best for all frameworks. I develop primarily with Angular so I understand your frustration. The PR that I put out, which uses the debouncing technique, was implemented to help improve what currently exists in the addon knobs codebase. @shilman + @igor-dv + @tmeasday, is the approach that @igor-dv brought up something that's currently being considered for the future of this addon?

[jonrimmer]

@ndelangen Can 2e30aeb#diff-d3a8a461a0bf5672a74aa09b8808b50e be reverted if there is no ability to fix this?

[shilman]

@CodeByAlex how is the debouncing solution working for you in your project, and can we apply similar heuristics e.g. for the slider component? I've also suggested we post the update only on blur, which is a more extreme measure, but could further mitigate things.

@jonrimmer are you volunteering to maintain knobs for angular? 😉

[jonrimmer]

@shilman There should be no need to add debouncing for anything. All of the frameworks Storybook supports can easily handle thousands of updates per second. The only reason debouncing is necessary is because Storybook is tearing down and recreating the entire Angular app when anything changes.

Imagine if, any time the user clicked a button, you completely destroyed and reinitialised your entire Angular app. Would you consider that a sane strategy? Would it pass code review? I'm guessing not. But this is exactly what Storybook is doing!

And yes, I'd be happy to help out maintaining a working version of knobs for Angular if the alternative is having to accept Storybook shipping with a perpetually broken version.

[ndelangen]

@jonrimmer I'd love to chat with you, would you be able to connect on our discord?

https://discord.gg/sMFvFsG

[shilman]

Great Caesar's ghost!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.10 containing PR #6094 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.