-
Notifications
You must be signed in to change notification settings - Fork 908
Prevent possible duplicate email sending. #315
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
Conversation
I was getting duplicate email sending so I figured out that if you do the XHR request sync you will only send it once for sure [1]. In addition, I added a conditional statement inside the XHR callback so you only hide the form + show the thank you message when the request is achieved correctly [2]. Reference: [1] [https://xhr.spec.whatwg.org/#the-open()-method](xhr.open) [2] [https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange](xhr.onreadystatechange)
Synchronous XHR requests are deprecated; I do not think we should use them.
Do you have a reproducible test case for duplicate emails? Was your submit button being disabled, or can users click on it more than once, possibly sending the form multiple times?
I also read that onReadyStateChange shouldn't pair with synchronous requests.
Other changes all look good though 👍 Thank you for submitting a PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we cannot accept this change. It will become deprecated since this is not run within a service worker but within the main window object. The spec has this warning in a yellow box and it is on MDN as well. Mentioned below for convenience.
Synchronous XMLHttpRequest outside of workers is in the process of being removed from the web platform as it has detrimental effects to the end user’s experience. (This is a long process that takes many years.) Developers must not pass false for the async argument when current global object is a Window object. User agents are strongly encouraged to warn about such usage in developer tools and may experiment with throwing an "InvalidAccessError" DOMException when it occurs.
Thanks to @tarasyarema for discovering the fix for this. Originally pulled from PR #315. Makes sure that the XHR request is completed and successful before clearing and hiding the form.
Thanks for the help improving the code! See the follow-up PR #326 for further comments.
Thanks to @tarasyarema for discovering the fix for this. Originally pulled from PR #315. Makes sure that the XHR request is completed and successful before clearing and hiding the form.
* Ensure XHR success before clearing the form Thanks to @tarasyarema for discovering the fix for this. Originally pulled from PR #315. Makes sure that the XHR request is completed and successful before clearing and hiding the form. * Remove extraneous console logging Cleans up console output. Manual logging can be added by users themselves as desired. * Remove old honeypot function (dead code) Removed as part of another PR, but I forgot to remove the unused function. It just checks if there is a value and returns the result of that if statement, with unnecessary logging, so clean that all up.
I was getting duplicate email sending so I figured out that if you do the XHR request sync you will only send it once for sure [1]. In addition, I added a conditional statement inside the XHR callback so you only hide the form + show the thank you message when the request is achieved correctly [2].
Reference:
[1] XMLHttpRequest: xhr.open
[2] MDN web docs: xhr.onreadystatechange