Skip to content

Commit

Permalink
Add Type Safety Guards to turbo/fetch_requests
Browse files Browse the repository at this point in the history
Without this change, GET `<form>` submissions without a Fetch Options `{
body: }` raise the following:

```
Uncaught TypeError: s.body is null
    <anonymous> fetch_requests.js:10
    w turbo.es2017-esm.js:351
    requestStarted turbo.es2017-esm.js:770
    perform turbo.es2017-esm.js:520
    start turbo.es2017-esm.js:744
    formSubmitted turbo.es2017-esm.js:3369
```

Through the exercise of attempting to [port the `turbo/fetch_requests`
module to
TypeScript](hotwired#392 (comment)),
we've identified some potential edge cases in the algorithm that
determines a request's HTTP method.

Even if the migration to TypeScript doesn't come to fruition for some
time, those edge cases should be addressed sooner rather than later.

This commit adds more "type safety" motivated conditionals and guards to
make sure that values are present before overriding them.
  • Loading branch information
seanpdoyle committed Nov 19, 2022
1 parent 2a80b2c commit 1720181
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 8 deletions.
36 changes: 33 additions & 3 deletions app/assets/javascripts/turbo.js
Original file line number Diff line number Diff line change
Expand Up @@ -4025,12 +4025,13 @@ function encodeMethodIntoRequestBody(event) {
if (event.target instanceof HTMLFormElement) {
const {target: form, detail: {fetchOptions: fetchOptions}} = event;
form.addEventListener("turbo:submit-start", (({detail: {formSubmission: {submitter: submitter}}}) => {
const method = submitter && submitter.formMethod || fetchOptions.body && fetchOptions.body.get("_method") || form.getAttribute("method");
const body = isBodyInit(fetchOptions.body) ? fetchOptions.body : new URLSearchParams;
const method = determineFetchMethod(submitter, body, form);
if (!/get/i.test(method)) {
if (/post/i.test(method)) {
fetchOptions.body.delete("_method");
body.delete("_method");
} else {
fetchOptions.body.set("_method", method);
body.set("_method", method);
}
fetchOptions.method = "post";
}
Expand All @@ -4040,6 +4041,35 @@ function encodeMethodIntoRequestBody(event) {
}
}

function determineFetchMethod(submitter, body, form) {
const formMethod = determineFormMethod(submitter);
const overrideMethod = body.get("_method");
const method = form.getAttribute("method") || "get";
if (typeof formMethod == "string") {
return formMethod;
} else if (typeof overrideMethod == "string") {
return overrideMethod;
} else {
return method;
}
}

function determineFormMethod(submitter) {
if (submitter instanceof HTMLButtonElement || submitter instanceof HTMLInputElement) {
if (submitter.hasAttribute("formmethod")) {
return submitter.formMethod;
} else {
return null;
}
} else {
return null;
}
}

function isBodyInit(body) {
return body instanceof FormData || body instanceof URLSearchParams;
}

addEventListener("turbo:before-fetch-request", encodeMethodIntoRequestBody);

var adapters = {
Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/turbo.min.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion app/assets/javascripts/turbo.min.js.map

Large diffs are not rendered by default.

37 changes: 34 additions & 3 deletions app/javascript/turbo/fetch_requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,48 @@ export function encodeMethodIntoRequestBody(event) {
const { target: form, detail: { fetchOptions } } = event

form.addEventListener("turbo:submit-start", ({ detail: { formSubmission: { submitter } } }) => {
const method = (submitter && submitter.formMethod) || (fetchOptions.body && fetchOptions.body.get("_method")) || form.getAttribute("method")
const body = isBodyInit(fetchOptions.body) ? fetchOptions.body : new URLSearchParams()
const method = determineFetchMethod(submitter, body, form)

if (!/get/i.test(method)) {
if (/post/i.test(method)) {
fetchOptions.body.delete("_method")
body.delete("_method")
} else {
fetchOptions.body.set("_method", method)
body.set("_method", method)
}

fetchOptions.method = "post"
}
}, { once: true })
}
}

function determineFetchMethod(submitter, body, form) {
const formMethod = determineFormMethod(submitter)
const overrideMethod = body.get("_method")
const method = form.getAttribute("method") || "get"

if (typeof formMethod == "string") {
return formMethod
} else if (typeof overrideMethod == "string") {
return overrideMethod
} else {
return method
}
}

function determineFormMethod(submitter) {
if (submitter instanceof HTMLButtonElement || submitter instanceof HTMLInputElement) {
if (submitter.hasAttribute("formmethod")) {
return submitter.formMethod
} else {
return null
}
} else {
return null
}
}

function isBodyInit(body) {
return body instanceof FormData || body instanceof URLSearchParams
}
12 changes: 12 additions & 0 deletions test/application_system_test_case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
driven_by :selenium, using: :headless_chrome, screen_size: [1400, 1400]

def assert_no_javascript_errors
before = page.driver.browser.logs.get(:browser)

yield

logs = page.driver.browser.logs.get(:browser) - before
errors = logs.select { |log| /severe/i.match?(log.level) }
error_messages = errors.map(&:message)

assert_equal [], error_messages
end
end

Capybara.configure do |config|
Expand Down
4 changes: 4 additions & 0 deletions test/dummy/app/views/articles/edit.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
<h1>Edit Article</h1>

<form action="<%= articles_path %>">
<button>articles#index</button>
</form>

<%= form_with model: @article, url: article_path(@article.id) do |form| %>
<%= form.label :body %>
<%= form.text_area :body %>
Expand Down
10 changes: 10 additions & 0 deletions test/system/form_submissions_test.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
require "application_system_test_case"

class FormSubmissionsTest < ApplicationSystemTestCase
test "form submission [method=get]" do
article = Article.create! body: "My article"

visit edit_article_path(article.id)

assert_no_javascript_errors do
click_on "articles#index"
end
end

test "form submission method is encoded as _method" do
article = Article.create! body: "My article"

Expand Down

0 comments on commit 1720181

Please sign in to comment.