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

1.0.0.rc10 async "caching" error #2009

Closed
timhwang21 opened this issue Sep 15, 2017 · 4 comments
Closed

1.0.0.rc10 async "caching" error #2009

timhwang21 opened this issue Sep 15, 2017 · 4 comments

Comments

@timhwang21
Copy link
Contributor

Bug: When user types faster than responses can finish, "intermediary" search queries do not ever get attempted.

Example:
sep-15-2017 18-31-47

I type "asdf" while throttling the network. Queries are fired for "a", "as", "asd", and "asdf". However, when I start deleting letters, the queries for the first three search strings are never attempted. Note the placeholder text says 'Type to search' rather than 'No results.'

@timhwang21
Copy link
Contributor Author

Some more detail: it seems like part of the issue relates to the "intelligent caching" of input strings that have already been fetched. When the user types faster than the fetches can complete, it seems like "no options" are being cached for the intermediate search strings.

@timhwang21
Copy link
Contributor Author

Ah, it seems like the original issue is due to the component hitting the Github API rate limit 😄 oops.

Regardless, I think there is room for optimization here:

Let's say the user types 'asdf'. Queries will be sent for 'a', 'as', 'asd', and 'asdf'. However, because only the most recent callback is ran, only the last result is cached, even though the first three requests may complete successfully.

@JedWatson what do you think about moving lines 104-106 outside the if (callback === this._callback) block? This means that completed network requests will still be cached, even if they're not used. The result would be that when the user hits delete from 'asdf', there won't be a redundant request sent for 'asd', as we've already cached that response. If it sounds good to you, I can file a PR. Thanks!

@timhwang21
Copy link
Contributor Author

A related issue occurs when handling paginated endpoints:

Let's say I do a search for 'Bob'. Three requests are fired for 'B', 'Bo', and 'Bob', but only the third is cached.

Then, I clear the input and search for 'Bob' again. Upon entering 'Bo', the search is re-fired, and upon entering 'Bob', I cache hit and a new this._callback is not set, so the callback for 'Bo' is fired, resetting the options array with a new set of options.

This normally would be fine, but with paginated endpoints, the options for 'Bob' may not be a subset for those of 'Bo' (there may be a hundred 'Boa's for example). Thus, we end up in a state where the search string is 'Bob' but the options are for 'Bo', which can result in 'No results found.'.

The solution suggested above should fix it, but do you think it would also be good to set this._callback = null; in the cache hit condition? Thanks!

timhwang21 pushed a commit to timhwang21/react-select that referenced this issue Sep 18, 2017
Addresses JedWatson#2009.

There is a WIP test -- I can't figure out how to simulate the exact
scenario that this patch fixes in the test. The current test returns
positive even without my change. Suggestions would be appreciated --
thanks!
@JedWatson
Copy link
Owner

Thanks for the comprehensive issue report and fix, @timhwang21

Closed by #2012

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

No branches or pull requests

2 participants