Extract `HTMLFormSubmission` tuple #885

seanpdoyle · 2023-03-02T00:28:28Z

The HTMLFormElement, HTMLElement? data clump occurs in various places across the internals of the codebase.

This commit extracts that pairing into an HTMLFormSubmission class that knows how to properly extract various pieces of HTML Form Submission context from the pairing, like the method, action, or target values (which can be overridden by formmethod, formaction, and formtarget, respectively).

With the new extraction various call sites can be simplified to delegate to the object, with the most impactful changes being made in the FormSubmission class (mostly changes to the constructor and the removal of various dynamic properties).

seanpdoyle · 2023-03-02T00:30:29Z

I don't love the HTMLFormSubmission name, but it felt better than SubmissionTuple, FormSubmissionTuple, FormSubmissionRequest, or SubmitRequest (derived from HTMLFormElement.requestSubmit). I'm open to any and all naming suggestions!

@kevinmcconnell Additionally, changing the FormSubmission properties to be pre-determined and readonly will help enable (and shrink!) the changes proposed in #691.

seanpdoyle · 2023-03-02T00:32:05Z

src/core/drive/form_submission.ts

+    this.submitter = submission.submitter
+    this.formData = submission.formData
+    this.location = submission.location
+    this.method = submission.fetchMethod


The majority of these readonly properties were once defined as get-powered properties, which are part of the FormSubmission public API. Since turbo:submit-start and turbo:submit-end event listeners are able to access FormSubmission instances, it feels like we should go through a deprecation cycle before moving them to be private or replaced by this.submission[...] lookups.

If the next version of Turbo is a major version bump to 8.0, that might have implications for deprecation.

seanpdoyle · 2023-08-25T16:48:37Z

@afcapel @kevinmcconnell are either of you able to review these changes (and #691, the PR that these changes are in service of).

The `HTMLFormElement, HTMLElement?` [data clump][] occurs in various places across the internals of the codebase. This commit extracts that pairing into an `HTMLFormSubmission` class that knows how to properly extract various pieces of HTML Form Submission context from the pairing, like the `method`, `action`, or `target` values (which can be overridden by `formmethod`, `formaction`, and `target`, respectively). With the new extraction various call sites can be simplified to delegate to the object, with the most impactful changes being made in the `FormSubmission` class (mostly changes to the `constructor` and the removal of various dynamic properties). [data clump]: https://refactoring.guru/smells/data-clumps

seanpdoyle · 2023-09-11T00:33:34Z

@afcapel @kevinmcconnell I've rebased these changes to account for the migration from TypeScript. It's ready for review.

afcapel

@seanpdoyle I think the change makes sense, but I'm not convinced that replacing element, submitter argument tuples with an elementOrSubmission argument helps readability. The method then have to figure out what the elementOrSubmission really is, and extract the the HTML elements that it needs from there. All while those elements were readily available without any ambiguity in the calling place.

afcapel · 2023-09-13T13:06:47Z

src/core/drive/html_form_submission.js

+  plain: "text/plain"
+}
+
+export function formEnctypeFromString(encoding) {


We can move this function to the bottom and remove the export. It's only used in this file, further down.

afcapel · 2023-09-13T13:16:09Z

src/core/drive/html_form_submission.js

+    if (this.submitter?.hasAttribute("formtarget") || this.form.hasAttribute("target")) {
+      return this.submitter?.getAttribute("formtarget") || this.form.getAttribute("target")
+    } else {
+      return null
+    }


Suggested change

if (this.submitter?.hasAttribute("formtarget") || this.form.hasAttribute("target")) {

return this.submitter?.getAttribute("formtarget") || this.form.getAttribute("target")

} else {

return null

}

return this.submitter?.getAttribute("formtarget") || this.form.getAttribute("target") || null

afcapel · 2023-09-13T13:19:17Z

src/core/drive/html_form_submission.js

+    if (this.submitter?.hasAttribute("formaction")) {
+      return this.submitter.getAttribute("formaction") || ""
+    } else {
+      return this.form.getAttribute("action") || formElementAction || ""
+    }


Suggested change

if (this.submitter?.hasAttribute("formaction")) {

return this.submitter.getAttribute("formaction") || ""

} else {

return this.form.getAttribute("action") || formElementAction || ""

}

return this.submitter?.getAttribute("formaction") || this.form.getAttribute("action") || formElementAction || ""

afcapel · 2023-09-13T13:34:51Z

src/core/drive/html_form_submission.js

+      const formDataAsStrings = [...this.formData].reduce((entries, [name, value]) => {
+        return entries.concat(typeof value == "string" ? [[name, value]] : [])
+      }, [])
+
+      return new URLSearchParams(formDataAsStrings)


Suggested change

const formDataAsStrings = [...this.formData].reduce((entries, [name, value]) => {

return entries.concat(typeof value == "string" ? [[name, value]] : [])

}, [])

return new URLSearchParams(formDataAsStrings)

const params = new URLSearchParams()

for (const [name, value] of this.formData.entries()) {

if (typeof value === "string") params.append(name, value)

}

We could extract a this.formDataAsURLParams helper method from this bit, for symmetry with this.formData.

afcapel · 2023-09-13T13:40:43Z

src/core/drive/form_submission.js

-    if (this.method == FetchMethod.get) {
-      mergeFormDataEntries(this.location, [...this.body.entries()])
-    }
+    this.submission = submission


We can call this argument and instance var htmlSubmission, to avoid confusion with FormSubmission.

afcapel · 2023-09-13T13:46:29Z

src/core/frames/frame_controller.js


    this.#withCurrentNavigationElement(element, () => {
      frame.src = url
    })
  }

-  proposeVisitIfNavigatedWithAction(frame, element, submitter) {
-    this.action = getVisitAction(submitter, element, frame)
+  proposeVisitIfNavigatedWithAction(frame, elementOrSubmission) {


I don't think overloading the second argument here is helping readability. I'd leave the arguments as frame, element, submitter and unwrap the element and submitter in the only caller that uses an HTMLFormSubmission.

afcapel · 2023-09-13T13:49:22Z

src/core/frames/frame_controller.js

-  #findFrameElement(element, submitter) {
-    const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target")
-    return getFrameElementById(id) ?? this.element
+  #findFrameElement(elementOrSubmission) {


Same here, I'd leave the arguments as (element, submitter) and modify the one caller using an HTMLFormSubmission. Being assertive about what the arguments are makes the code more confident and easier to follow.

afcapel · 2023-09-13T13:50:52Z

src/core/frames/frame_controller.js

-    const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target")
-
-    if (element instanceof HTMLFormElement && !this.#formActionIsVisitable(element, submitter)) {
+  #shouldInterceptNavigation(elementOrSubmission) {


afcapel · 2023-09-13T13:54:09Z

src/core/frames/frame_redirector.js

  }

-  #shouldRedirect(element, submitter) {
+  #shouldRedirect(elementOrSubmission) {


seanpdoyle commented Mar 2, 2023

View reviewed changes

seanpdoyle force-pushed the html-form-submission branch 2 times, most recently from 5ce6c08 to b76cba8 Compare July 21, 2023 15:32

seanpdoyle force-pushed the html-form-submission branch from b76cba8 to 813cdc1 Compare September 11, 2023 00:29

afcapel reviewed Sep 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract `HTMLFormSubmission` tuple #885

Extract `HTMLFormSubmission` tuple #885

seanpdoyle commented Mar 2, 2023 •

edited

Loading

seanpdoyle commented Mar 2, 2023

seanpdoyle Mar 2, 2023

seanpdoyle Sep 11, 2023

seanpdoyle commented Aug 25, 2023

seanpdoyle commented Sep 11, 2023

afcapel left a comment •

edited

Loading

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

afcapel Sep 13, 2023

Extract HTMLFormSubmission tuple #885

Are you sure you want to change the base?

Extract HTMLFormSubmission tuple #885

Conversation

seanpdoyle commented Mar 2, 2023 • edited Loading

seanpdoyle commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpdoyle commented Aug 25, 2023

seanpdoyle commented Sep 11, 2023

afcapel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Extract `HTMLFormSubmission` tuple #885

Extract `HTMLFormSubmission` tuple #885

seanpdoyle commented Mar 2, 2023 •

edited

Loading

afcapel left a comment •

edited

Loading