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 validation #1958

Merged
merged 18 commits into from
Mar 24, 2021
Merged

Async validation #1958

merged 18 commits into from
Mar 24, 2021

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Mar 4, 2021

resolves #1270 Async Validations

This is the first try at async validation.

All unit tests for current validation are passing just fine.

New async validation is also working but there are some issues. If ChangeTextOnKeyPress is enabled then async race condition will happen and the cursor will jump around the input while the user is typing. I still need to come up with a good solution for this. So far DelayTextOnKeyPress seems to work fine with new async validation.

@stsrki stsrki mentioned this pull request Mar 4, 2021
@stsrki
Copy link
Collaborator Author

stsrki commented Mar 8, 2021

Hey @David-Moreira, it would be great if you could give me some feedback and ideas regarding this PR. I have implemented support for async validation but the problem is when ChangeTextOnKeyPress is enabled to the field that is validated. If you run the demo and go to the Validations page and start typing in the first field you will see what happens. The caret starts jumping randomly. I have tried many things and was not able to fix it.

@David-Moreira
Copy link
Contributor

I'll take a look when I've got the time, see if I have a different idea or smthg,

@David-Moreira
Copy link
Contributor

I was taking a brief look at this. What is this for?

  • var caret = await JSRunner.GetCaret( ElementRef );
    Removing
  • await JSRunner.SetCaret( ElementRef, caret );
    Solves it, for both wasm and server, since there's a race condition happening.

Also what's up with all of these:

  • cancellationToken.ThrowIfCancellationRequested();
    Can't we exit gracefully? This could potentially bring slowdowns on real applications with a few hundred of users, if exceptions keep being throw like that, I would say.
    Maybe we could do some benchmarks, just to be sure, if we're gonna keep it.

I haven't really looked indepth into the other stuff, but looks good to me from a brief look.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 22, 2021

Hmm, var caret = await JSRunner.GetCaret( ElementRef ); was an old "fix" for the problem with caret jumping around on Blazor server when typing fast. Maybe I can remove it now 🤔 ...more investigation is needed.

All those cancellationToken.ThrowIfCancellationRequested(); was my try to come up with something that would make it work. To tell you the truth I don't have much experience with canceling tasks. Never needed them before. If you have an elegant idea I'm open to suggestions. Or we can completely remove that part if it's not needed.

BTW, thank you for looking into this from another perspective, you really helped!

@David-Moreira
Copy link
Contributor

No problem. More then one eye looking at a problem helps!! Strange, how that fixed it 😅 I would remove and test it. Since which version is it on?

If it ends up fixing the the typing fast problem for server might be worthy of a hotfix on 0.9.3 and even 0.9.2
Let me know how it goes. If it still happens I can help look at solutions. I use blazor server, and I ended up disabling that feature, kinda would like to have that feature working if possible.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 23, 2021

I suspect 0.9.2 is not an option because there were also some major changes in 0.9.3 that made JSRunner.GetCaret not needed. And I really don't want to go support another version 😅

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 23, 2021

@David-Moreira After I removed JSRunner.GetCaret and SetCaret, works like a charm. Thank you!!!

Do you think I should remove CancellationToken? Not sure if I need it at all.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 23, 2021

🤯🤦‍♂️ I have discovered the original problem that led to the implementation of GetCaret and SetCaret. When you type fast in the middle of a text the caret would jump to the end of the input. This problem is not specific to Blazor only. It can also be seen in react and angular. https://stackoverflow.com/questions/40931845/angular2-keyup-event-update-ngmodel-cursor-position-jumps-to-end

Basically when the value is updating the browser needs to rerender input and in doing so it will jump the caret to the end. It is default browser behavior. Now, I don't want to revert to GetCaret and SetCaret but I will need to find another way to make it work in every scenario.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 23, 2021

@David-Moreira
Copy link
Contributor

Haha I didn't know about that. So The code was there for a reason!!! Altought that does not seem like the greatest solution 😆 I'll see if I have any ideia.

As for the cancellation token I see a commit removing it. It should be fine without it for like most of the cases. What about if someone does validation with more work, like invoking a service with variable response times. Can we potentially have code that's vulnerable to race conditions there? Where async validation 2 finishes first then async validation 1? We should test that.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 23, 2021

That's why I wanted to have a cancellation token. It's not that hard to return if needed. Do you want to try? Or maybe point me to some good article about CT?

@David-Moreira
Copy link
Contributor

I generally just read Microsoft docs, to see what do they themselves recommend.
You will find that cancellationToken.ThrowIfCancellationRequested(); is recommended, but again I'm worried about it throwing exceptions all over the place since it's gonna be happening on every users key stroke, so exiting gracefully should be preferred if we can. It's also something we can also eventually benchmark.

It's a bit old but straight to the point:
https://devblogs.microsoft.com/premier-developer/recommended-patterns-for-cancellationtoken/

I remember learning about tokens like one or two years ago from an YouTube video about async stuff, think it was from Tim Corey.

I could also try, I do want to help you with stuff on blazorise, but I've been busy with other stuff. I just don't want to keep piling on work and having you have to wait on me.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 24, 2021

I understand, no worries. You've been a big help so far and I cannot help you enough :)

Well for now I can just skip the CT. It is going for 0.9.4. and we have more than enough time to add support.

@stsrki
Copy link
Collaborator Author

stsrki commented Mar 24, 2021

In the end, I reverted to CancellationToken. Without it, validation would not work all the time. For example, if I clear the text and write one letter fast then an error would stay even though the input is valid.

@David-Moreira
Copy link
Contributor

Alright. Yea race conditions are always an issue on these async stuff. You can never be 100% sure the previous "action/post" won't finish after the current "action/post"...

Agree if that works. Just leave it be for now. And we'll worry about performance improvements if needed later.

@stsrki stsrki changed the title Async validation prototype Async validation Mar 24, 2021
@stsrki stsrki merged commit cd76ade into dev094 Mar 24, 2021
@stsrki stsrki deleted the dev094-async-validation branch March 24, 2021 15:54
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.

2 participants