Adding addon-options makes input from addon-knobs sluggish and unpredictable

[andersaloof]

Describe the bug
When adding the withOptions decorator, input from knobs became sluggish.

To Reproduce
Steps to reproduce the behavior:

1. Add addon-options to existing project which uses addon-knobs already, according to instructions.
2. Setup withOptions with an empty object addDecorator(withOptions({})).
3. View a story with knobs and edit text.

Expected behavior
Input behaviour should not feel sluggish, and the typed string should be correct.

Screenshots
Entering the string "Spain, a country with lots of fun and beaches" with the withOptions decorator added. Result is a garbled string of nonsense.

The same string "Spain, a country with lots of fun and beaches" without the withOptions decorator.

Code snippets
If applicable, add code samples to help explain your problem.

System:

• OS: Windows 8
• Device: Desktop computer
• Browser: Chrome 70.0.3538.77
• Framework: React
• Addons: knobs, a11y, notes, options
• Version: 4.0.4

Additional context
Having 13-14 components in my current project, and lots more to be done, I added the addon-options decorator to group components with the hierarchyRootSeparator option, but adding this made everything really sluggish. Commenting out the withOptions decorator makes every component work as expected again.

[VladimirPittner]

Hi @andersaloof did you also attached withOptions as a decorator (at the top-level) ?
There is this one thing

NOTE that you must attach withOptions as a decorator (at the top-level) for this to work.

Edit: Sorry I did not noticed it was version 4.0.4, in that case I can confirm I am seeing the same behaviour.

[igor-dv]

I can assume this is due to options now applied on every story change

CC @tmeasday, we probably need to have some optimization there

[tmeasday]

Yeah, I guess we need to put in some memoization close to the code that doesn't cope with options being reset to the same value.

I think this could be a good one for someone to help us out with! I assume it would be pretty easy to replicate by adding some options parameters to one of the knobs stories in our official-storybook; then start digging (starting here maybe?) into what code path it is that is expensive to set a given option to its same value

[yjcxy12]

I've had a look into this issue. I believe the slow performance happens when addDecorator(withOptions(...)) is used in multiple places (both globally and locally). This makes memoization quite difficult as setOptions is being called multiple times with different objects with no notion of each other.

The easiest solution I can think of is mention in the documentation to use withOptions() only in global config once and use storiesOf().addParameters({ options: {} }) for local options. Unless we can somehow get the final options object for each story before calling api.setOptions. @tmeasday

[andersaloof]

@yjcxy12 In my case, I only used withOptions once, as a global setting, no local settings, and it still makes i slow and sluggish.

[tmeasday]

I believe @ndelangen has probably been looking at this kind of thing as a part of the refactoring he has been doing in ui-overhaul. Is this something you've looked at norbert?

[CodeByAlex]

I'm seeing this issue in the Storybook angular environment. However, I have not used withOptions. I'm currently using version 4.0.12 because the a11y addon is broken in versions later than that. I checked the newest version, 4.1.2, and I am still seeing the sluggish input issue. Is there any update on the cause of this? I can confirm that the component is being recreated on knob change.

[anoopsinghbayes]

I found passing the following parameters does not change the view if i use withOptions global config version >= 4.0.0

full=0&addons=1&stories=1

but this used to work in 4.0.0-alpha.23

[CodeByAlex]

On line 26 in the link found below, the component is getting re-rendered which is resetting angular components on every change. This is a major performance hit and makes large components extremally difficult to change via the knobs unless the user copies and pastes the text into the field. @tmeasday and @ndelangen , is there any plan to fix this? I'm sure it is impacting other frameworks.

https://github.com/storybooks/storybook/blob/4c757d05f162925217751bb5d61518c108d0935b/addons/knobs/src/registerKnobs.js#L26

[tmeasday]

Do we need a denounce here? What would you suggest @CodeByAlex?

[CodeByAlex]

I apologize, I didn't mean to come off harsh or to offend anyone. I noticed that both of your names were mentioned so I included you in the message and I was looking to figure out if the ui-overhall would cover the issue.

I was looking through the code and Im trying to understand the timestamp usage. It seems as though the force rerender is being called upon every keystroke even when the timestamp is being used based on what I saw debugging. My understanding of the timestamp is that it would cause no events to be fired while the user is typing which would mean that the event would fire upon completion of a quick set of changes.

I see it being used here: https://github.com/storybooks/storybook/blob/5108da8e3b7c02a1890fdc4813cc97b3a1480762/addons/knobs/src/components/Panel.js#L61

and here it doesnt do a check but it sets the most recent edit time:
https://github.com/storybooks/storybook/blob/5108da8e3b7c02a1890fdc4813cc97b3a1480762/addons/knobs/src/components/Panel.js#L121

Im thinking that it may be better to move the check closer to the on changes function. Do you think that would be a good idea or am I misunderstanding the usage?

[tmeasday]

Hi @CodeByAlex! Sorry I wrote the above on my phone and I see it has autocorrected "debounce" to "denounce"!! 😅

Don't worry, no-one is offended 😄 . I just figured you had more context than me and might be able to suggest a solution, which it seems you can!

I think it would definitely make sense to look at this in the ui-overhaul (now release/5.0 branch) as I believe knobs has changed there and in fact @shilman is currently working to get it (knobs) operational again before we merge release/5.0 to next. I'm sure he would appreciate any assistance and it may be a good time to fix this issue if it is still occurring there.

[CodeByAlex]

No worries @tmeasday! Thanks for the info. Is there a schedule of when 5.0 might be released to next? I just got my team on board with using storybook with Angular so I'm excited to see where it's going. Also, I agree that debouncing may be the best bet here.

[tmeasday]

We are looking to merge release/5.0 into next as soon as we get the tests passing, probably in the next 24-48 hours

[CodeByAlex]

That sounds great! @shilman, do you think it would be possible to get the code to do the debouncing in before release 5.0 is merged into next? or should that be taken on separately?

I believe it should look something like this:

function knobChanged(change) {
    clearTimeout(this.timeoutId);
    this.timeoutId = setTimeout(() => { // debouncing occurs here to increase performance
     const { name, value } = change;

     // Update the related knob and it's value.
     const knobOptions = knobStore.get(name);

     knobOptions.value = value;
     knobStore.markAllUnused();

     forceReRender();
     }, DEBOUNCE_DELAY) // this is an arbitrary amount of time used to ensure that the user has time to type before re-rendering
}

[tmeasday]

@CodeByAlex I'm sure that fix can make its way into next too (after it is merged). We are looking at having a couple of weeks bugfix period before the v5 release. I think this counts as a bugfix

[ndelangen]

In V5 the options addon is deprecated, as it's now using the addParameters api.

This should resolve this issue completely.