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

Core: wait for pendingRequests to finish before submitting form #2369

Merged

Conversation

bytestream
Copy link
Member

@bytestream bytestream commented Dec 18, 2020

JS Fiddle

https://jsfiddle.net/bytestream/a16ehn0L/18/

Description

When jquery-validation is used with a WYSIWYG editor there can be a delay in synchronisation between the div[contenteditable] and the underlaying textarea. This delay combined with remote validation leads to a state where the form is valid but does not submit.

The issue is hopefully fairly obvious when looking at stopRequest:

if ( valid && this.pendingRequest === 0 && this.formSubmitted && this.form() ) {

this.form() can cause new remote validation rules to fire. In the vast majority of cases, the input data is the same and so jquery-validation looks at the cached result and doesn't bother with another AJAX request. In this case however, the delay in synchronisation causes the request data to change so jquery-validation fires another AJAX request. The line noted above currently does not wait for any new remote validation checks to return before submitting the form.

Thank you!

@staabm does this make sense?

@bytestream
Copy link
Member Author

@bytestream bytestream commented Dec 21, 2020

I worked around this using:

submitHandler: function (form) {
    // CodeMirror saves when form submit events are fired. We need to manually trigger a save
    // so that jquery-validation has the correct form data.
    $(form).find('.CodeMirror').each(function(i, el){
        el.CodeMirror.save();
    });

    // more stuff here
},

That said, I believe there is still merit to this PR as this.form() should not be allowed to submit the form if there are pending requests. Though the chances of this happening are slim for the reasons mentioned in my first message.

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 4, 2022

@staabm what's necessary to see this merged and released?

@staabm
Copy link
Member

@staabm staabm commented Apr 4, 2022

The PR implies a synchronous remote validation step, right?

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 4, 2022

The remote validation is still asynchronous. this.form() fires asynchronous remote validation if the input value changes from the cached value, and stopRequest runs after each of those requests finish.

@staabm
Copy link
Member

@staabm staabm commented Apr 4, 2022

yeah.. I was more about that the newly added condition will check directly after .form() was invoked, which looks like a race condition involved when the validation is async?

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 4, 2022

I don't see a race condition. Eventually the pending remote validation requests will be 0, unless the input values repeatedly change then it would loop endlessly.

@staabm staabm merged commit eb88df0 into jquery-validation:master Apr 4, 2022
1 check passed
@staabm
Copy link
Member

@staabm staabm commented Apr 4, 2022

thx!

@bytestream bytestream deleted the stop-request-pending-requests branch Apr 5, 2022
@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 7, 2022

@staabm would you mind publishing 1.19.4-pre to npm?

npm publish --tag next

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 11, 2022

@staabm ?

@staabm
Copy link
Member

@staabm staabm commented Apr 11, 2022

I will have a look this afternoon..

@staabm
Copy link
Member

@staabm staabm commented Apr 12, 2022

didn't find the time - sorry about that.

@bytestream are you willing to help, if I grant you permission todo it yourself?

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 12, 2022

@staabm sure, username is bytestream on npm too

@staabm
Copy link
Member

@staabm staabm commented Apr 12, 2022

just invted you on npm @bytestream

please see the necessary steps at

/* Release checklist
- Run `git changelog` and edit to match previous output (this should make use of jquey-release instead)
- make sure the correct 'x.y.z-pre' version is defined in package.json
- `cd jquery-release` into your local https://github.com/jquery/jquery-release fork
- `git pull` latest https://github.com/jquery/jquery-release
- disable _generateChangelog task in release.js (BOOOO), by commenting this lines: https://github.com/jquery/jquery-release/blob/a9823df8a5dff4c96d1f6645b09daa591adc2f06/release.js#L43-L44
- run
node release.js --remote=jquery-validation/jquery-validation
- Wait a while, verify and confirm each step
- Create GitHub release: Pick the new tag, add changelog, upload zip (from __release/repo/dist/*.zip)
- Upload to NPM
cd into your local jquery-validation fork
git fetch --tags upstream
git checkout tags/X.YY.Z
npm publish
- double check NPM for new release https://www.npmjs.com/package/jquery-validation
- Update MS CDN (Ping Chris Sfanos)
- Check jsdelivr CDN: new git tags are automatically pulled, tested & merged via https://github.com/jsdelivr/jsdelivr/pulls
- Check cdnjs CDN: new git tags are automatically committed into https://github.com/cdnjs/cdnjs/commits/master or ping @cdnjs
- Update validation-content/pages/index.html (may have to hold off on CDN updates until available)
- Write blog post: Some highlights, changelog, download links
*/

@bytestream
Copy link
Member Author

@bytestream bytestream commented Apr 12, 2022

@staabm thanks. I need GitHub access for some of those things, but I think I'm OK without to publish a -pre release. #2422 also needs merging as it blocks running the release

@staabm
Copy link
Member

@staabm staabm commented Apr 12, 2022

I just added you to the github repo

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.

None yet

2 participants