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

@uppy/form: bring back submit() instead of requestSubmit() #5299

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

Murderlon
Copy link
Member

Closes #5278

Sometime ago I switched our form.submit() to form.requestSubmit() (#4852) so that we first trigger form validation and only submit when there are no errors, while submit() always submits.

Unfortunately I didn't realize that unlike submit() requestSubmit() triggers the 'submit' event again, which caused a loop. This was fixed in #5058.

Unfortunately I didn't realize again that requestSubmit() no longer works after event.preventDefault(), which we need for the triggerUploadOnSubmit in order to hold off the submit to upload with Uppy. If you also enable submitOnSuccess, then once we reach requestSubmit() it no longer works because of preventDefault().

AFAIK for these specific set of conditions we support in Uppy, we can't reliably use requestSubmit so I'm putting submit back.

@Murderlon Murderlon requested review from mifi and aduh95 July 2, 2024 14:51
@Murderlon Murderlon self-assigned this Jul 2, 2024
Copy link
Contributor

github-actions bot commented Jul 2, 2024

Diff output files
diff --git a/packages/@uppy/form/lib/index.js b/packages/@uppy/form/lib/index.js
index 3f1d23e..a1f8cbb 100644
--- a/packages/@uppy/form/lib/index.js
+++ b/packages/@uppy/form/lib/index.js
@@ -1,13 +1,3 @@
-function _classPrivateFieldLooseBase(receiver, privateKey) {
-  if (!Object.prototype.hasOwnProperty.call(receiver, privateKey)) {
-    throw new TypeError("attempted to use private field on non-instance");
-  }
-  return receiver;
-}
-var id = 0;
-function _classPrivateFieldLooseKey(name) {
-  return "__private_" + id++ + "_" + name;
-}
 import BasePlugin from "@uppy/core/lib/BasePlugin.js";
 import findDOMElement from "@uppy/utils/lib/findDOMElement";
 import toArray from "@uppy/utils/lib/toArray";
@@ -30,17 +20,12 @@ function assertHTMLFormElement(input) {
   }
   return input;
 }
-var _completed = _classPrivateFieldLooseKey("completed");
 export default class Form extends BasePlugin {
   constructor(uppy, opts) {
     super(uppy, {
       ...defaultOptions,
       ...opts,
     });
-    Object.defineProperty(this, _completed, {
-      writable: true,
-      value: false,
-    });
     this.type = "acquirer";
     this.id = this.opts.id || "Form";
     this.handleFormSubmit = this.handleFormSubmit.bind(this);
@@ -50,22 +35,22 @@ export default class Form extends BasePlugin {
     this.getMetaFromForm = this.getMetaFromForm.bind(this);
   }
   handleUploadStart() {
-    _classPrivateFieldLooseBase(this, _completed)[_completed] = false;
     if (this.opts.getMetaFromForm) {
       this.getMetaFromForm();
     }
   }
   handleSuccess(result) {
-    _classPrivateFieldLooseBase(this, _completed)[_completed] = true;
     if (this.opts.addResultToForm) {
       this.addResultToForm(result);
     }
     if (this.opts.submitOnSuccess) {
-      this.form.requestSubmit();
+      if (this.form.reportValidity()) {
+        this.form.submit();
+      }
     }
   }
   handleFormSubmit(ev) {
-    if (this.opts.triggerUploadOnSubmit && !_classPrivateFieldLooseBase(this, _completed)[_completed]) {
+    if (this.opts.triggerUploadOnSubmit) {
       ev.preventDefault();
       const elements = toArray(ev.target.elements);
       const disabledByUppy = [];

@aduh95
Copy link
Contributor

aduh95 commented Jul 2, 2024

Should we use reportValidity()?

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we should add reportValidity to the list of things one needs to polyfill in order to use Uppy with e.g. IE, but we didn't for requestSubmit and no one complained, so I guess we're fine.

@aduh95 aduh95 merged commit 03a4cd8 into main Jul 2, 2024
17 checks passed
@aduh95 aduh95 deleted the requestSubmit branch July 2, 2024 15:52
github-actions bot added a commit that referenced this pull request Jul 2, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/companion        |  4.15.0 | @uppy/drag-drop        |   3.1.1 |
| @uppy/companion-client |   3.8.2 | @uppy/form             |   3.2.2 |
| @uppy/core             |  3.13.1 | uppy                   |  3.27.2 |

- @uppy/form: do not emit `'submit'` event more than once (Merlijn Vos / #5299)
- @uppy/companion: add `s3.forcePathStyle` option (Nadeem Reinhardt / #5066)
- meta: fix broken workflow status badges in README (Alexander Zaytsev / #5298)
- @uppy/core: add `clearUploadedFiles` to type definition (Augustine Smith / #5295)
- @uppy/companion: add `oauthOrigin` option (Antoine du Hamel / #5297)
- meta: add dark-mode Transloadit logo in README (Alexander Zaytsev / #5291)
- docs,@uppy/drag-drop: `uppy.io/docs` - fix typos/broken links (Evgenia Karunus / #5296)
- meta: Bump docker/build-push-action from 6.1.0 to 6.2.0 (dependabot[bot] / #5290)
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.

Form with an Uppy-element without uploads must be submitted twice
2 participants