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

Knobs seem slow to update #10019

Closed
lucaswerkmeister opened this issue Mar 2, 2020 · 5 comments
Closed

Knobs seem slow to update #10019

lucaswerkmeister opened this issue Mar 2, 2020 · 5 comments

Comments

@lucaswerkmeister
Copy link

Describe the bug
When using the knobs addon to test a timing-dependent component (a loading bar that should appear/disappear after a certain time), we found that changes to the knobs seem to propagate rather slowly, compared to our previous solution (a plain <input type="checkbox"> with v-model).

To Reproduce
Steps to reproduce the behavior:

  1. Clone the brief reproducer at https://github.com/lucaswerkmeister/storybook-knobs-slow
  2. Read the cloned code, to ensure that I didn’t put anything malicious there (it’s not much)
  3. npm install
  4. npm run storybook
  5. In the story, check and uncheck the “plain checkbox” within the canvas, as well as the “knob” in the knobs tab, and compare how quickly the text in the canvas (“The plain checkbox is x. The knob is y.”) updates.

If you don’t trust the reproducer, you can also create a story similar to test.story.js in your own environment, then repeat step 5 there.

The slowness of the knob can also be observed at the official storybook (linked from the addon-knobs readme), though that doesn’t have the direct comparison to the plain input. (Also, that storybook happens to use React whereas my reproducer happens to use Vue.) Similar to step 5 above, adjust some of the knobs (e. g. the “Nice” boolean) and observe how long it takes for the canvas to update.

Expected behavior
Both checkboxes, the plain one and the knob, update the content more or less instantaneously.

Screenshots
GIF:
Peek 2020-03-02 16-11

Code snippets
See reproducer, especially test.story.js.

System:

  System:
    OS: Linux 5.3 Ubuntu 19.10 (Eoan Ermine)
    CPU: (4) x64 Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
  Binaries:
    Node: 13.8.0 - ~/.nvm/versions/node/v13.8.0/bin/node
    npm: 6.13.6 - ~/.nvm/versions/node/v13.8.0/bin/npm
  Browsers:
    Firefox: 73.0.1
  npmPackages:
    @storybook/addon-knobs: ^5.3.14 => 5.3.14 
    @storybook/vue: ^5.3.14 => 5.3.14 

Additional context
This appears to affect all knobs equally, at least according to some quick tests on the official storybook (linked from the addon-knobs readme).

@lucaswerkmeister
Copy link
Author

lucaswerkmeister commented Mar 2, 2020

This might be a feature instead of a bug? ^^ It sounds like some debouncing was added in #5811, though I didn’t spend too much time looking through the history.

I briefly tried to switch the debouncing to the leading edge, instead of the trailing one, by adding a { leading: true, trailing: false } third argument to the _debounce call in node_modules/@storybook/addon-knobs/dist/registerKnobs.js; but while this mostly eliminates the delay, it also means that the knob input and the canvas state can come out of sync if you click again during the debounce period. Using { leading: true, trailing: true } seems to fix that, though.

My colleague @wiese remarks that throttling might be even closer to what’s desired here. We want the leading edge, for immediate response; we need the trailing edge, for eventual consistency; but if, using _.throttle, we can also get some intermediate events and thus occasionally update the component while events keep coming in, that’s also nice to have.

@wiese
Copy link

wiese commented Mar 2, 2020

Looked at the reproducer in dev tools.
It appears the event gets debounced (mind the leadingEdge) - the browser is not busy computing the new result, but rather waits to do so.

Bildschirmfoto von 2020-03-02 18-12-00

I have a hard time coming up with a scenario where it would have a detrimental effect to instead react immediately and (comparable to what happened so far) only then prevent the same event from hitting downstream over and over - but maybe that is due to our narrow view on a binary input.

https://css-tricks.com/the-difference-between-throttling-and-debouncing/

@wiese
Copy link

wiese commented Mar 3, 2020

As a blunt work around I was looking into ways to disable the debouncing altogether.

In the documentation a flag timestamps is mentioned which sounds like it could be related ("Doesn't emit events while user is typing.") - it does however not affect the behavior and is not to be found in KnobManagerOptions.

Interestingly, looking at the source code of KnobManagerOptions, there is an option disableDebounce which is used in registerKnobs when deciding if debouncing is supposed to be applied, and results in the expected behavior.

It may be worth noting that the implementation of registerKnobs recently changed about this (#9447 - looks promising also for our use case) so maybe someone has this on their radar already.

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this issue Mar 3, 2020
So far we have no use for debounced values from knobs in storybook - we
only use few knobs and the updates seem to be quite performant.
We filed storybookjs/storybook#10019 [0] to check with the storybook
maintainers if the behavior we witnessed is as expected.
While researching we saw that work [1] was put into this area recently
so maybe more fine grained controls will be possible with the next
release, which we then can use if need arises.

[0] storybookjs/storybook#10019
[1] storybookjs/storybook#9447

Change-Id: I3ea30acb60e142dff5c9fbf4fe4cda9a40d4c990
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this issue Mar 3, 2020
* Update Wikibase from branch 'master'
  to e26d8240f723293ad450d38bdb0140f417b90096
  - Merge "bridge: disable value debounce for knobs in storybook"
  - bridge: disable value debounce for knobs in storybook
    
    So far we have no use for debounced values from knobs in storybook - we
    only use few knobs and the updates seem to be quite performant.
    We filed storybookjs/storybook#10019 [0] to check with the storybook
    maintainers if the behavior we witnessed is as expected.
    While researching we saw that work [1] was put into this area recently
    so maybe more fine grained controls will be possible with the next
    release, which we then can use if need arises.
    
    [0] storybookjs/storybook#10019
    [1] storybookjs/storybook#9447
    
    Change-Id: I3ea30acb60e142dff5c9fbf4fe4cda9a40d4c990
@stale
Copy link

stale bot commented Mar 26, 2020

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!

@stale stale bot added the inactive label Mar 26, 2020
@stale
Copy link

stale bot commented Apr 25, 2020

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

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

3 participants