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

Debounce source function by debounceMs on keystroke #611

Closed
wants to merge 5 commits into from

Conversation

rgarner
Copy link
Contributor

@rgarner rgarner commented Nov 18, 2023

When debounceMs is greater than 0, delay the calling of the source
function for that number of milliseconds. In the circumstance where the
source function is talking to an API with a potentially large result
set – the querying of which has potential to overload hosting – even
a modest value of 300ms or so can halve or quarter the number of
queries each user sends without appreciably degrading their experience.

When debounceMs is 0, pass the source function through unchanged.
This commit should not change the behaviour of any existing consumer.

Don't display the tNoResults message while debounce is happening, and likewise
silence the Status announcer to avoid "no results available" occurring back to back
with "50 results available".

Caveat

I've been running the functional tests developing this. I'm unable to run the integration tests locally; they all fail with async errors of the form

Chrome Headless 99.0.4844.0 darwin #0-0] AssertionError: expected {} to equal 'Accessible Autocomplete examples'
[Chrome Headless 99.0.4844.0 darwin #0-0]     at Context.equal (/Users/russ/dev/accessible-autocomplete/test/integration/index.js:215:35)
[Chrome Headless 99.0.4844.0 darwin #0-0]     at Context.executeAsync (/Users/russ/dev/accessible-autocomplete/node_modules/@wdio/utils/build/shim.js:325:27)
[Chrome Headless 99.0.4844.0 darwin #0-0]     at Context.testFrameworkFnWrapper (/Users/russ/dev/accessible-autocomplete/node_modules/@wdio/utils/build/test-framework/testFnWrapper.js:45:32)
[Chrome Headless 99.0.4844.0 darwin #0-0]
[Chrome Headless 99.0.4844.0 darwin #0-0] 2) basic example should show the input
[Chrome Headless 99.0.4844.0 darwin #0-0] expected {} to equal true
      actual expected

      Promise [Promise] {}true

@rgarner rgarner force-pushed the debounce branch 2 times, most recently from 973b7e0 to a2915ea Compare November 18, 2023 21:42
@rgarner rgarner changed the title Debounce typed queries by debounceMs Debounce source function by debounceMs on keystroke Nov 19, 2023
@rgarner rgarner force-pushed the debounce branch 3 times, most recently from 54702a4 to 32655c7 Compare November 19, 2023 13:32
@colinrotherham colinrotherham added the awaiting triage Needs triaging by team label Nov 20, 2023
@rgarner
Copy link
Contributor Author

rgarner commented Nov 20, 2023

I have rebased this onto #612 locally; integration tests do pass. I've added an example for debounceMs to examples/index.html, but I'm unclear on whether or not I should add an integration test for it (or indeed whether one is required).

@rgarner
Copy link
Contributor Author

rgarner commented Nov 21, 2023

Pausing this; looks like wdio bits got moved to #615 so my local rebase with some working debounce integration tests is stale. I'll have another go in a day or two!

@colinrotherham
Copy link
Contributor

Thanks @rgarner

I've split out the WebdriverIO v8 upgrade into #615 since the breaking changes go a step beyond maintenance

The package updates in #612 should be a relatively stable base now and drop fibers as before

@rgarner
Copy link
Contributor Author

rgarner commented Nov 21, 2023

@colinrotherham thanks again for all this. I know how intense and discursive this sort of work is, so it's really appreciated.

I just rebased this onto the latest package-updates I can pull, ran npm install, but I get this on npm test (call stack very truncated):

21 11 2023 16:53:51.614:ERROR [config]: Error in config file!
  /Users/russ/dev/accessible-autocomplete/node_modules/puppeteer/node_modules/puppeteer-core/lib/cjs/puppeteer/util/disposable.js:19
Symbol.dispose ??= Symbol('dispose');
               ^^^

SyntaxError: Unexpected token '??='
    at wrapSafe (internal/modules/cjs/loader.js:1029:16)
    at Module._compile (internal/modules/cjs/loader.js:1078:27)
    at Module._compile (/Users/russ/dev/accessible-autocomplete/node_modules/pirates/lib/index.js:117:24)

@colinrotherham
Copy link
Contributor

@rgarner Ah hopefully your README suggestion will come in handy now

The Node.js version in .nvmrc is now the latest LTS release so nvm install will switch you to Node.js 20

@rgarner
Copy link
Contributor Author

rgarner commented Nov 21, 2023

I did notice iron over fermium but I just didn't notice enough this late in the day to think that through, thanks 😃

@rgarner
Copy link
Contributor Author

rgarner commented Nov 22, 2023

@colinrotherham I've updated this after having rebased it onto package-updates. Can confirm it works there and here (mostly your package updates let me remove stuff from here!), but not sure I want to open it to review while you're working on All That And A WebDriverIO Bun because that would be, you know, kind of overwhelming. I'll hang on until package-updates is merged?

@rgarner
Copy link
Contributor Author

rgarner commented Nov 30, 2023

On the basis that this works on the package-updates branch and to give this a reasonable chance of being looked at while there aren't any conflicts, I'm un-drafting it.

@rgarner rgarner marked this pull request as ready for review November 30, 2023 22:38
@kellylee-gds kellylee-gds removed the awaiting triage Needs triaging by team label Dec 20, 2023
@romaricpascal romaricpascal added this to the Next milestone Jan 11, 2024
Status is using it, but autocomplete will be in a moment.
When debounceMs is greater than 0, delay the calling of the `source`
function for that number of milliseconds. In the circumstance where the
`source` function is talking to an API with a potentially large result
 set – the querying of which has potential to overload hosting – even
 a modest value of 200ms or so can halve or quarter the number of
 queries each user sends without appreciably degrading their experience.

 When debounceMs is 0, pass the `source` function through unchanged.
 This commit should not change the behaviour of any existing consumer.
When a `source` query has yet to be called, it's not necessary to show
"no results found". Pass the debouncing state of the autocomplete down
to the Status component through Status.props.autocompleteDebouncing
such that the status component does not announce "no results available"
immediately before announcing "50 results available".

From the autocomplete, this is difficult to test. Tests already exist
showing that `this.state.debouncing` is set appropriately when
`debounceMs` is in use. It is used in conjunction with the
`showNoOptionsFound` prop to suppress tNoResults in render().

No good option exists for testing it that doesn't involve extra
redundant state to service the tests. It can't be done by rendering
tests alone, and it can't be done by behaviour tests alone.
@colinrotherham
Copy link
Contributor

Given this a rebase with main to resolve conflicts

@rgarner
Copy link
Contributor Author

rgarner commented Jan 17, 2024

@colinrotherham many thanks indeed, I would not have had your level of understanding

@romaricpascal
Copy link
Member

Hi @rgarner,

Thanks for proposing this and calling our attention to debouncing when loading from a server!

While reviewing it, I've noticed that your changes introduce a little glitch in the UI, displaying a thicker border when the user starts typing on an empty field. Looks like the listbox containing the results becomes made visible staight away, but because the calls are debounced, the message gets hidden until results actually come back.

This is probably better seen than described, so I've captured two recordings (one with a silly long timeout to have enough time to see what happens).

The quick debounce:

Kapture.2024-01-18.at.11.59.46.mp4

The longer delay

Kapture.2024-01-18.at.12.40.11.mp4

While an option would be to update the check that makes the listbox visible to account for the source being debounced, this would deprive users from any feedback, making the UI feel laggy. And keeping the message visible would provide incorrect feedback as it's not that there's 'No results found', but results are being retrieved.

As an alternative, I've explored what was already possible in terms of debouncing with the current state of the project. With the help of the tNoResult option, it's possible to both debounce source and provide accurate feedback to users while things are loading.

On top of providing feedback that results are loading and debouncing to not put servers on their knees (and avoid wasting requests while users are typing), there are also other concerns to handle when fetching suggestions from a server:

  • rendering feedback when an error occurs
  • making sure that the only the last request response get displayed.

With the debouncing happening inside the component, this becomes a little trickier to add. And like the feedback and loading, it seems it can all be dealt with by crafting the function fetching the results appropriately, which would allow not to expand the responsibilities of the component.

Kapture.2024-01-18.at.18.29.35.mp4

Would you be able to confirm if the implementation in this example on the debounced-ajax-example branch would have been suitable for reducing the load of your server or let us know were the gaps would be?

@rgarner
Copy link
Contributor Author

rgarner commented Jan 18, 2024

With the debouncing happening inside the component, this becomes a little trickier to add. And like the feedback and loading, it seems it can all be dealt with by crafting the function fetching the results appropriately, which would allow not to expand the responsibilities of the component.

Hi @romaricpascal — thanks for the videos and the example, they're super helpful. I noticed the same thing with the border when testing our fork in our project. We hedged our bets and tNoResults is indeed how we've already handled it in our project. Our Stimulus lookup controller's connect() looks like

  connect() {
    this._id = this.textfieldTarget.id

    this.handleQueryDebounced = debounce(async (query, populateResults) => {
      await this.handleQuery(query, populateResults);
    }, this.debounceMsValue);

    this.debouncedSource = async (query, populateResults) => {
      this.#debouncing = true;
      try {
        await this.handleQueryDebounced(query, populateResults);
      } finally {
        this.#debouncing = false;
      }
    }

    accessibleAutocomplete({
      element: this.autocompleteRootTarget,
      id: this.textfieldTarget.id, // To match it to the existing <label>.
      defaultValue: this.textfieldTarget.value,
      source: this.debouncedSource.bind(this),
      onConfirm: this.lookupSelected.bind(this),
      minLength: this.minLengthValue,
      tNoResults: () => this.#debouncing ? this.searchingValue : this.noResultsValue,
      templates: {
        suggestion: this.renderSuggestion.bind(this)
      }
    })
    this.textfieldTarget.id = null
    this.autoCompleteInput.addEventListener('input', () => this.clearLookupId())
    this.searching = this.lookupIdBlank() && this.lookupNameBlank() && this.noErrorMessages()
  }

tNoResults: () => this.#debouncing ? this.searchingValue : this.noResultsValue, is doing much the same thing as the example.

I don't see any gaps, it was pretty nice to implement with the args already available to us.

The only real "gap" as such we've noticed (and this could easily be another thing I missed) are not at all debounce-related. They're that the component doesn't report certain events we'd be interested in as they arrive at the internal input — you'll notice the this.autoCompleteInput.addEventListener('input', () => this.clearLookupId()) line where we need to clear out a selected reference if the user starts typing again following a successful onConfirm. this.autoCompleteInput does have to reach into the HTML with this.element.querySelector('.autocomplete__input') to allow this. But as I say, that's not in the least bit related to debounce!

Given the glitches you've noticed — and the complexity of having two debounces (one for source and the fixed 1400ms one for the announcer), I'll close this. Your debounce example is very well thought-through and would be hugely useful as user docs instead.

Thanks hugely for your time, it's very much appreciated — even if they never make it to main, if I'd have seen something like your example first I'd never even have submitted a PR 😄

@rgarner rgarner closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants