Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add an ajaxCounter to the pager object so it can index each request and ... #443

Merged
merged 1 commit into from
Nov 27, 2013

Conversation

christhomas
Copy link
Contributor

...refuse to process old requests and only process the last one, this is important because when you execute multiple requests, sometimes the old requests return AFTER the new request has returned, meaning you get a mixed up data, this will stop that, even though really it should cancel the ajax request, but once the server has it, I don't know how you can do that.

…nd refuse to process old requests and only process the last one, this is important because when you execute multiple requests, sometimes the old requests return AFTER the new request has returned, meaning you get a mixed up data, this will stop that, even though really it should cancel the ajax request, but once the server has it, I don't know how you can do that.
@thezoggy
Copy link
Collaborator

shouldnt this be done on the user side to handle async calls?

@christhomas
Copy link
Contributor Author

I can't think of a single reason why you'd want to filter the table and want updates out of order, surely if you type "gr" and then it searches and you type another "ou" (gr+ou) then you don't care about the results from "gr" and only the results which come from "grou", but if you get served the results from "grou" and then the "gr" also completes, it'll change the table and overwrite all your filters.

this overwriting of the filters is actually something I want to change also, I think it shouldn't do that, but thats another topic of discussion :)

or perhaps you can explain a reason why it might be desirable?

@Mottie
Copy link
Owner

Mottie commented Nov 27, 2013

Honestly, I don't really like the counter idea. What if it gets really out of sync somehow? Then nothing will get through.

Why not increase the filter_searchDelay option or simply set the filter_liveSearch option to false and force the user to press Enter to initialize a query?

@christhomas
Copy link
Contributor Author

the counter is only updated in one place and it's used in a specific circumstance, there is only one location where the code performs an ajax call and if there is more than one, we can refactor it together.

no, the options you've suggested are not enough to prevent the problem that I've mentioned here, you are just hoping it doesn't trigger the race condition, but ultimately, it's not a solution, it's a workaround.

this solves the problem by making it not possible to accept out of order requests, it doesn't prevent future requests cause if you trigger another update, it'll execute again and attempt to find the last one, if the ajax is constantly failing, then there is nothing you can do to prevent the situation in the first place since it's out of your hands.

perhaps we can add an option which states "ajaxAccept: [any,last]" and any == no restriction, last == last counter value, then it's configurable for any situation where this counter is not desirable

@TheSin-
Copy link
Collaborator

TheSin- commented Nov 27, 2013

Why not increase the filter_searchDelay option or simply set the filter_liveSearch option to false and force the user to press Enter to initialize a query?

Honestly this is what I ended up doing, my users take forever to get a thought out and thus type very slowly, the delay I needed was insane, so I just set it to false. I love search as you type but it just didn't work for me I ran into this same issue, hitting enter is safer and works 100%.

@christhomas
Copy link
Contributor Author

for that particular use case I can understand it's desirable, I know users like this also, but in the scenario that you want dynamic updates as you type, then the counter trick really does help a lot, I've attached a screenshot of how it stopped my table from doing random updates in any old order and at least made it useful.

screen shot 2013-11-27 at 10 39 18

@Mottie
Copy link
Owner

Mottie commented Nov 27, 2013

Well, as I said, I keep an open mind so I'm not against adding this.. but wouldn't it be better to use a less than comparison versus a not equal to comparison?

if (counter < p.ajaxCounter) {

@christhomas
Copy link
Contributor Author

I think you're assuming it might become larger than the testable condition somehow? I guess there is no harm to that, I mean, it's largely not changing anything, still works the same way, so I'm ok with that.

do you want the option to configure it? or do you think it should be automatic? I personally can't see a situation where multiple updates occur and they arrive out of order is desirable to use an older update over a newer one, but if somebody can provide a use-case description where it would be something useful, I'm all ears.

@Mottie
Copy link
Owner

Mottie commented Nov 27, 2013

I don't think an option would be necessary - as long as it works. But, I was wondering if you tested this with a filter search or sorting? Looking at the screen shot, I see different page requests but neither the filter nor sort change.

@christhomas
Copy link
Contributor Author

yeah, it works as well, see the screenshot, I suppose it'll work with anything that attempts to go through this code and do an ajax request, meaning it won't make any difference when working with a static table, since no ajax comes into play

screen shot 2013-11-27 at 16 56 12

Mottie added a commit that referenced this pull request Nov 27, 2013
add an ajaxCounter to the pager object so it can index each request and ...
@Mottie Mottie merged commit 633cc3e into Mottie:master Nov 27, 2013
Mottie added a commit that referenced this pull request Dec 2, 2013
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants