-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Server Side Search Deletes Search Text #1081
Comments
@mhmcdonald Same issue here. There's is a 'racing' condition when typing: Typing triggers a server call. This does not occur when typing fast since the debouncing takes care of it. Not that if you have a slow connection, it would make the matter even worse - as in you will have to type fast, or very slow. But if you hit the 'wrong' speed when typing, it gets out of hand 😃 Notice on the video the server calls on the right. When they return, they override the input value. Note: I'm typing 'hope' and then 'keyword' grid-js-server-search-issue.mp4I believe the way to work around that would be to decouple the searching from the rendering. I'm happy to open a PR for that, I found the issue, just give me a day or two 👍 |
Made a bit of progress here - not as much as expected. So, it turns out it is an issue related to the way we debounce the user input - there might be no racing conditions with the server as I initially thought. As a quick fix, you can disable the debouncing until I fix this properly: This will get rid of the bug. I'll try to keep you updated soon once I've done more digging 🔧 👍 |
Thanks for the update @aloysb - I really appreciate you digging into this |
There you go! 🍻 @mhmcdonald Feel free to pull on that branch until the PR is merged. It seems that sometimes the answer is just KISS Keep it simple, stupid. There is no need to have the search input controlled (at least, at this stage) - and it does create a racing condition. Rather than having the current input and the current search stored separately, I've simply turned the input into a plain ol' uncontrolled component since it doesn't HAVE to be controlled and ... tada! 🎉 no more racing condition 👍 |
Amazing, thank you @aloysb! You rock! I will be watching the PR. |
Hey @mhmcdonald , Just to let you know that we'll aim to get this reviewed soon, maybe next week 👍 |
thanks again for the update @aloysb! |
Could you please elaborate how to turn the input into a plain of uncontrolled component? |
Sure,
Just check out the open PR, it's literally removing one line of code.
Basically, you want to let the input control its own state rather than
managing it from the outside (by *not* setting `value={ ... }`).
The search is watching the onChange event, so there is no real need to
control the input value.
There is a race condition on the onChange, due to the fact that there is a
debounce on the call.
"onChange -> debounce -> call server -> return and update the value".
However, if the user had typed more characters during the trip to the
server, then it is overriden when the server respond. That's where the bug
is coming from.
It only happens if you type in sync with the debounce period.
If you type slowly, each event is sent to the server and returned in time.
If you type fast, then you never trigger the debounce - only when you stop
typing.
But if you type at the *wrong* speed, you create this racing condition.
…On Fri, Sep 30, 2022, 10:05 PM nitinmalik1997 ***@***.***> wrote:
There you go! 🍻 @mhmcdonald <https://github.com/mhmcdonald>
Feel free to pull on that branch until the PR is merged.
It seems that sometimes the answer is just *KISS* Keep it simple, stupid.
Look at how small the code changes I've made are 😄
There is no need to have the search input controlled (at least, at this
stage) - and it does create a racing condition.
Rather than having the current input and the current search stored
separately, I've simply turned the input into a plain ol' uncontrolled
component since it doesn't HAVE to be controlled and ... tada! 🎉 no more
racing condition 👍
Could you please elaborate how to turn the input into a plain of
uncontrolled component?
—
Reply to this email directly, view it on GitHub
<#1081 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEHYZ4QZCRRUD5TX2GUXTMLWA3JPTANCNFSM55YQKENQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Increasing the wrong speed is quick fix from the config side meanwhile. |
Where did you set the value of debounceTimeout:1000 ? |
In the search block It's in the docs: |
Just a matter for RTFM, thank you for that. |
#1267 can potentially fix this issue. Can someone run some tests on that branch please? |
The issue still persists. |
@abitbetterthanyesterday i feel like your solution is optimal but there's a small catch which is not handled and that's why we have to keep the search keyword in the state. Like if you want to pass an initial value to the grid than your solution would not honor that and behave inappropriately. I have a small fix for that which I am thinking to create a PR. It's more like persist the input keyword in the state using useState and in addition to that call the debounce method of the text input as is. @afshinm let me know your thoughts on the solution. |
I'm interested in this, I'll increase the debounce timeout for now but a better solution is appreciated, thanks to everyone involved! |
This happens for client side as well with a reasonable amount of data (~800 total rows, 100 rows/page, 10+ cols) ETA: Not sure if the client side problem is related to the same root cause as the server side, but it seems to be mostly mitigated by removing the debounce altogether:
I think the "next event cycle" is still causing a bit of an issue with removing text if you type really fast, but its mostly fine now. Ideally we can no-op the debounce if the entire grid is loaded in the client |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Describe the bug
I've noticed that when I use server side search and type in search text at a reasonable pace, some of the most recent text will get deleted. This behavior can be seen in this gif. This results in mistakes while entering your search phrase because the text you're typing in gets changed so easily/unexpectedly.
I've noticed this bug on my own projects as well as on the server-side search example on the grid.js documentation site.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
The server-side search feature returns filtered table records as soon as you start typing. The text that I type into the search box should not be altered/edited.
Screenshots
You can see this bug in this gif.
In this gif, I'm typing in the word 'empire' into the search bar. After typing in the first three letters, 'emp' the table returns results for 'em'. However the in the search box, the 'p' has been deleted from my search text (you can see the 'emp' for a split second before the 'p' gets removed). It accepts my fourth letter typed in ('i') so then I have the incorrect search text of 'emi'. Towards the end of the gif you can see I have the same buggy experience trying to type in the letter 'r' (it gets deleted from the search text box very quickly after I've typed it in).
Desktop (please complete the following information):
The text was updated successfully, but these errors were encountered: