Add Type Safety Guards to turbo/fetch_requests (hotwired#403)

* Add Type Safety Guards to `turbo/fetch_requests`

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.

* Resolve Turbo Stream test flakiness

Replace looping mechanisms with Capybara assertion for the presence of a
`<turbo-cable-stream-source>` element.

app/assets/javascripts/turbo.js

@@ -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";
       }
@@ -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 = {

app/javascript/turbo/fetch_requests.js

@@ -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
+}

test/application_system_test_case.rb

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

test/dummy/app/views/articles/edit.html.erb

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

test/system/broadcasts_test.rb

@@ -43,7 +43,7 @@ def subscribe_to_broadcasts
 
     assert_no_button "Start listening for broadcasts"
 
-    Timeout.timeout(Capybara.default_max_wait_time) { wait_for_subscriber }
+    assert_selector :element, "turbo-cable-stream-source", visible: false
   end
 
   def assert_broadcasts_text(text, to:, &block)
@@ -53,18 +53,4 @@ def assert_broadcasts_text(text, to:, &block)
 
     within(:element, id: to) { assert_text text }
   end
-
-  def wait_for_subscriber
-    loop do
-      subscriber_map = ActionCable.server.pubsub.instance_variable_get(:@subscriber_map)
-      if subscriber_map.is_a?(ActionCable::SubscriptionAdapter::SubscriberMap)
-        subscribers = subscriber_map.instance_variable_get(:@subscribers)
-        sync = subscriber_map.instance_variable_get(:@sync)
-        sync.synchronize do
-          return unless subscribers.empty?
-        end
-      end
-      sleep 0.1
-    end
-  end
 end

test/system/form_submissions_test.rb

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