Refactor renderElement static methods to instance methods

[botandrose]

Now that #1028 has landed, this seems like an obvious win. Any reason not to do this?

[seanpdoyle]

From what I can recall, it was important for these methods to be static in order to preserve their functional "anonymity" (in other words, to prevent access to properties through an object's this context). Since turbo:before-render event listeners can re-assign the render function through event.detail.render = ..., I believe it was important for the functions to be anonymous.

For example, consider something like:

addEventListener("turbo:before-render", ({ detail }) => {
  const originalRender = detail.render

  detail.render = (currentElement, newElement) => {
    // do something special ...
    originalRender(currentElement, newElement)
  }
})

If that style of override is still possible with instance methods, then this change is worthwhile.

Regardless of what's possible, it's also worth remembering that PageRenderer and FrameRenderer are exported classes, and therefore their interfaces are part of public API.

While Turbo doesn't yet have an established deprecation process, it's worth highlighting that removing these static methods would break backwards compatibility.

[botandrose]

Hey Sean, thank you for taking the time to look at this PR, and explain the context to me! It makes sense that static enforces them being functions, rather than methods. I also hear you about the API breakage making removing them a non-starter until Turbo 9.

With those reasons in mind, I'll close this PR.

... and I'd love your thoughts on whether or its worth doing something like this for an eventual 9.0! I submit that there may be some internal code quality to be gained by removing these static class functions from the public API, and normalizing how hooks work while we're at it.

Many of the other hooks work like this, a pattern also common in native APIs:

addEventListener("turbo:before-render", ({ detail }) => {
  const { currentElement, newElement }  = detail
  // do custom render ...
  event.preventDefault() // or not, to also let default render occur
})

This way, we wouldn't need to contort the internal API to expose its internals via static class functions, and we could even make renderElement a private method, narrowing the public API, while also making it easier to refactor. I can't imagine any real utility in providing the consumer with the concrete default renderElement function itself, but perhaps that's a failure of my imagination. Can you think of a use-case that requires it or benefits from it? The one downside to this strategy that I can see is that there would need to also be a turbo:after-render hook as well, to allow for post-render processing.

As an aside, I've noticed that a lot of other hooks could benefit from a consistency pass, as well. Some allow cancelling, some don't, etc.

Anyways, I've got more important fish to fry, like #1308 and #1311, but I'd be willing to explore this as a lower priority in a new PR, if you think it might have merit.

Thanks for your time!