googlearchive / core-ajax

Polymer Core Ajax
26 stars 52 forks source link

[0.8 preview] lastresponse and lasterror introduce race conditions #77

Open rictic opened 9 years ago

rictic commented 9 years ago

Consider this mini-app:

<input value='{{query}}'>
<core-ajax auto url='/search/{{query}}' lastresponse='{{response}}'></core-ajax>

Search results for <span>{{query}}</span>
<template repeat='{{searchResult in response.searchResults}}'>
    <div>{{searchResult.title}}</div>
</template>

Imagine that the user first types [cat] into the input. The backend is very slow to respond to that search, and the user changes their query to [dog]. The backend responds quickly with [dog] results, then immediately afterwards it responds to the old [cat] query. The user of this app now sees that they have typed in [dog] but are seeing results for [cat].

This sort of race condition is very common, and very easy to write. Polymer can solve it transparently, and has done so in 0.5 by exposing the response value for the most recently made request (which is determined by the application's behavior and intent), rather than the most recently received response (which is determined by the backend, the network, and luck).

It's possible that I'm biased towards this solution due to the kinds of apps that I work on, but every time I've used a core-ajax element over the past year, this is the behavior that I wanted. Is there a use case where taking the most recently received response is better?

@cdata

Related: https://github.com/Polymer/core-ajax/pull/14

cdata commented 9 years ago

Thank you for the feedback; this anecdote is very informative.

I won't attempt to characterize the design in 0.8 as "better" than what you described. I think that for the auto-complete / instant-search-results use-case, it is preferable to be able to avoid such a race condition, and core-ajax in 0.8 will not avoid it as you already made clear.

Instead, core-ajax is opting for simplicity (and punting the responsibility for transactional state management) by exposing lastRequest as a bindable, notifying property. This creates the opportunity for the user of core-ajax to do her own book-keeping of in-flight requests as needed. However, it would be a shame if most users ended up having to write boilerplate when using core-ajax as a network primitive.

It may be better in the long run for core-ajax to handle the kind of specific conditions you described. Would bindable properties that always reference the response or error of the most recently dispatched request handle the described usage adequately?

rictic commented 9 years ago

Would bindable properties that always reference the response or error of the most recently dispatched request handle the described usage adequately?

Yeah, I think that that's an elegant solution.

I'm happy to put together a pull request for this sometime this weekend.

cdata commented 9 years ago

Sure, send up a PR and we'll figure it out :+1:

rictic commented 9 years ago

Quick ping @cdata, I think this is ready to review.

rictic commented 9 years ago

Another friendly ping! +@jmesserly as well, since he merged a very similar patch back in the 0.4 days.

jmesserly commented 9 years ago

I bet @garlicnation or @addyosmani could help w/ that pull (#81)