Reorganize Turbo Events and declare events on global event map

[marcoroth]

This Pull Request reorganizes the events Turbo knows about and puts them into a single src/events.ts file.

Additionally it declares the Turbo events on GlobalEventHandlersEventMap and ElementEventMap so that the callbacks-arguments of event-listeners can be typed.

For example

Before:

import { TurboBeforeFetchRequestEvent } from "@hotwired/turbo"

document.addEventListener("turbo:before-fetch-request", (e) => {
  // `e` is typed as `Event`

  const event = e as TurboBeforeFetchRequestEvent
  // `event` is now typed as `TurboBeforeFetchRequestEvent`
})

After:

import { TurboBeforeFetchRequestEvent } from "@hotwired/turbo"

document.addEventListener("turbo:before-fetch-request", (event: TurboBeforeFetchRequestEvent) => {
  // `event` is typed as `TurboBeforeFetchRequestEvent`
})

Previously you would get a No overload matches this call error:

error TS2769: No overload matches this call.
  Overload 1 of 2, '(type: keyof WindowEventMap, listener: (this: Window, ev: MessageEvent<any> | Event | UIEvent | AnimationEvent | MouseEvent | ... 22 more ... | StorageEvent) => any, options?: boolean | ... 1 more ... | undefined): void', gave the following error.

---

Additional note: this came up as part of hotwired/turbo-rails#392

[seanpdoyle]

I love this! Thank you for taking it on.

There are a bunch of event listeners in the test suite, would you be able to remove their type casting and type guards as part of this change?

[marcoroth]

Good point, will do 👍🏼

[marcoroth]

I'm kinda confused on why and how the tests are failing. I get the same errors locally, trying to figure out what's going on.

[seanpdoyle]

@marcoroth I've encountered the CI flakiness as well, and have opened #833 to resolve it. Could you try pulling in that change and seeing if you have better luck in CI?

[seanpdoyle]

@marcoroth if you rebase the latest, you should be able to pass CI.

[marcoroth]

Thanks @seanpdoyle! It seems like the failures I was seeing are gone now. Now it just fails with the same failures as on main

[seanpdoyle]

@marcoroth 🎉 ! I've opened #838 to try and fix main.

[seanpdoyle]

@marcoroth if you want to get this branch passing while we wait for #838 to be merged, you can apply this diff:

diff --git a/src/tests/fixtures/navigation.html b/src/tests/fixtures/navigation.html
index 48953b7..f2fd53d 100644
--- a/src/tests/fixtures/navigation.html
+++ b/src/tests/fixtures/navigation.html
@@ -15,7 +15,7 @@
 
     <a href="#ignored-link" id="ignored-link">Skipped Content</a>
 
-    <section id="main" style="height: 200vh">
+    <section id="main">
       <h1>Navigation</h1>
       <p><a id="same-origin-unannotated-link" href="/src/tests/fixtures/one.html">Same-origin unannotated link</a></p>
       <p><a id="same-origin-unannotated-link-search-params" href="/src/tests/fixtures/one.html?key=value">Same-origin unannotated link ?key=value</a></p>
diff --git a/src/tests/functional/rendering_tests.ts b/src/tests/functional/rendering_tests.ts
index ba5555e..2ac873d 100644
--- a/src/tests/functional/rendering_tests.ts
+++ b/src/tests/functional/rendering_tests.ts
@@ -88,6 +88,7 @@ test("test reloads when tracked elements change due to failed form submission",
   })
 
   await page.click("#tracked-asset-change-form button")
+  await nextBeat()
 
   const reason = await page.evaluate(() => localStorage.getItem("reason"))
   const unloaded = await page.evaluate(() => localStorage.getItem("unloaded"))

[marcoroth]

Thanks @seanpdoyle 🙏🏼

Looking good now!

[seanpdoyle]

if you think we just just limit it to HTMLElement let me know.

I was thinking it'd make sense since the custom elements that dispatch those events descend from HTMLElement. Thinking more on it now, the type of element that dispatches the event matters a lot less than the type of element that listens for the event.

Having said that, I'm not even sure which tags in a Real World application descend from Element instead of HTMLElement, and HTMLElement descends from Element, so it might all be a moot point.

[stof]

Given that querySelector and querySelectorAll are typed as returning Element, adding those events only for HTMLElement would force apps to refine the type of the element to be able to register the events with those type benefits. So I would prefer registering them on Element.

[marcoroth]

I think it makes sense to keep them on Element since HTMLElement is anyway inheriting from Element, so we are not really loosing anything by doing that.

That being said, I'm still looking to incorporate @seanpdoyle's suggestion to use GlobalEventHandlersEventMap.

[jdelStrother]

Anything we can do that would help get this merged?

[brunoprietog]

Does this still make sense with the switch to JavaScript?

[jdelStrother]

@marcoroth hope it's ok, I reused a lot of your work here to update @types/hotwired__turbo at DefinitelyTyped/DefinitelyTyped#69392 .

[marcoroth]

@jdelStrother this is great, thank you!