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

Extract HTMLFormSubmission tuple #885

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Mar 2, 2023

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
Copy link
Contributor Author

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.

this.submitter = submission.submitter
this.formData = submission.formData
this.location = submission.location
this.method = submission.fetchMethod
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@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
Copy link
Contributor Author

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

Copy link
Collaborator

@afcapel afcapel left a comment

Choose a reason for hiding this comment

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

@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.

plain: "text/plain"
}

export function formEnctypeFromString(encoding) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +45 to +49
if (this.submitter?.hasAttribute("formtarget") || this.form.hasAttribute("target")) {
return this.submitter?.getAttribute("formtarget") || this.form.getAttribute("target")
} else {
return null
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +55 to +59
if (this.submitter?.hasAttribute("formaction")) {
return this.submitter.getAttribute("formaction") || ""
} else {
return this.form.getAttribute("action") || formElementAction || ""
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 || ""

Comment on lines +80 to +84
const formDataAsStrings = [...this.formData].reduce((entries, [name, value]) => {
return entries.concat(typeof value == "string" ? [[name, value]] : [])
}, [])

return new URLSearchParams(formDataAsStrings)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

if (this.method == FetchMethod.get) {
mergeFormDataEntries(this.location, [...this.body.entries()])
}
this.submission = submission
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

proposeVisitIfNavigatedWithAction(frame, element, submitter) {
this.action = getVisitAction(submitter, element, frame)
proposeVisitIfNavigatedWithAction(frame, elementOrSubmission) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target")

if (element instanceof HTMLFormElement && !this.#formActionIsVisitable(element, submitter)) {
#shouldInterceptNavigation(elementOrSubmission) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

}

#shouldRedirect(element, submitter) {
#shouldRedirect(elementOrSubmission) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants