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

Async> cache async response regardless of req order #2012

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

timhwang21
Copy link
Contributor

Addresses #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!

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!
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.994% when pulling d0d0396 on timhwang21:timhwang21/async-fallback into bd1fb2c on JedWatson:master.

Addresses the second issue mentioned in the linked issue. While the
previous commit makes this change unnecessary for regular use cases, the
issue is still present when debouncing. By clearing out
`this._callback`, intermediary searches (e.g. 'Ti' when 'Tim' is already
fetched) won't cause the options for 'Tim' to change when the request
for 'Ti' completes.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 92.006% when pulling 79f478e on timhwang21:timhwang21/async-fallback into bd1fb2c on JedWatson:master.

@JedWatson
Copy link
Owner

Thanks @timhwang21

This looks good to merge, I'll see what I can do to get the test working

@JedWatson JedWatson merged commit e199ad3 into JedWatson:master Sep 21, 2017
JedWatson added a commit that referenced this pull request Sep 21, 2017
@JedWatson
Copy link
Owner

@timhwang21 I fixed the test so it works correctly now. There are a couple of changes I made (including slightly different math for the delay and different cache sets for each input) but the key here was making the test asynchronous by adding the cb argument and then calling it after the assertions.

@timhwang21 timhwang21 deleted the timhwang21/async-fallback branch September 21, 2017 12:23
timhwang21 added a commit to timhwang21/react-select that referenced this pull request Oct 8, 2017
From @youtogod in the comments of JedWatson#2012:

> Add this would be better : `this.setState({ isLoading: false, options: cache[inputValue] });`. Otherwise when hitting the cache during a promise, the loading icon will remain and the menu won't appear.
@timhwang21
Copy link
Contributor Author

Hey @JedWatson ! Was wondering if you plan to cut a new release any time soon? This feature would be valuable for my use case 🙂

(Additionally, I filed this follow-up based on @youtogod 's suggestion: #2042. Thanks!)

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