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

Add render to turbo:before-stream-render event #684

Merged
merged 2 commits into from
Sep 13, 2022

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Aug 13, 2022

Follow-up to #479
Related to hotwired/turbo-site#107

As an alternative to exposing an otherwise private and internal
StreamActions "class", support custom <turbo-stream action="...">
values the same way as custom <turbo-frame> rendering or <body>
rendering: as part of its turbo:before-render-stream event.

For example, if Turbo were to rename StreamActions to something else,
it would need to continue to export StreamActions publicly for the
sake of backwards compatibility. By depending on an event listener
instead, client applications are insulated from any internal restructuring.

To change how Turbo renders the document during page rendering, client
applications declare a turbo:before-render event listener that
overrides its CustomEvent.detail.render function from its
default.

Similarly, to change how Turbo renders a frame, client applications
declare a turbo:before-frame-render and override its
CustomEvent.detail.render function from its default.

This commit introduces the StreamElement.renderElement function, and
extends the existing turbo:before-stream-render event to support the
same pattern with StreamElement.renderElement serving as the default
CustomEvent.detail.render value.

With those changes in place, callers can declare a document-wide event
listener and override based on the value of StreamElement.action:

const CustomActions = {
  customUpdate: (stream) => { /* ... */ },
  customReplace: (stream) => { /* ... */ },
  // ...
}

document.addEventListener("turbo:before-stream-render", (({ target, detail }) => {
  const defaultRender = detail.render

  detail.render = CustomActions[target.action] || defaultRender
}))

@seanpdoyle seanpdoyle force-pushed the before-render-stream-element branch 2 times, most recently from 5ca24bf to b62b839 Compare August 13, 2022 17:47
@marcoroth
Copy link
Member

marcoroth commented Aug 13, 2022

Nice, I like where this is heading! This also brings some consistency over which parts we have control over via event-listeners.

I like that you can intercept the rendering of a stream element, this allows the host application to intercept non-custom actions (like update or replace) but also custom actions introduced by third-party packages.

I can also see this feature useful when you conditionally want to "discard" a stream action or if you want to handle "the rendering" of certain actions by yourself.

In order to properly support a new custom action (like a custom morph action via morphdom) you also need to teach your server-side about it, which is not always desirable.

With this feature however, you would be able to intercept the rendering of the default actions and a) use morphdom without the need for a new custom action and b) you don't need to have any specific logic on the server-side for when to use morph over update/replace. You can simplify delegate this decision to the client.

Using the same example from above: I can also see a use-case where you have a server-side part which emits update / replace actions over which you don't have control over (like a part of the app which is served via a third-party Rails engine). With this feature you would be able to intercept and override the render function on the client-side without having to touch the emit-logic on the server-side.

With all of that said, I still think the export of the StreamActions module is beneficial. It feels more straightforward to let Turbo know about all the available Custom Actions. If you just rely on event-listeners to "register" and support custom actions you are never letting Turbo (or other packages) know that such custom actions even exist.

It feels more appropriate for packages to register new actions via StreamActions. Host applications can then intercept actions via the proposed event-listeners.

Versus the package needs to add an event-listener to override the render function in order to even "support and add" a new custom action. If the host application then wants to intercept the action it would need to add another event listener and override the function again, which feels kinda hacky (not to the mention the uncertainty that the two event-listener need to fire in the right order).


TL;DR: In my opinion your proposed changes have neat use-cases on their own and would be worth to add by themselves. In my opinion they don't interfere with the concept of defining Custom Actions via the exported StreamActions module, on the contrary, it even enhances them.

If it's about the public API / leaking internals, we could also add some helper functions on the exported Turbo object instead.

@hopsoft
Copy link

hopsoft commented Aug 24, 2022

Agree that a turbo:before-stream-render event enhances the extensibility provided by StreamActions. The approaches aren't mutually exclusive. For example, I'd like to extend Turbo via StreamActions while allowing consumers of my extension(s) to change behavior via the event.

Hoping to see both of these features make it into the library. 🤞🏻

@dhh
Copy link
Member

dhh commented Sep 12, 2022

Agree with @marcoroth's assessment that this seems a nice AND but needn't be an OR. If all you want to do is add a custom action, the currently exposed approach seems more intuitive. But this then allows you to go even further. Happy to see the AND go in.

When serializing `CustomEvent.detail` objects from the browser to the
Playwright test harness, the current `serializeToChannel` implementation
is prone to recurse infinitely if an object nests another object that
refers to the outer object.

To prevent that, this commit tracks which objects have been visited
during the current serialization process, and avoid an infinitely
recursing loop.

Related to that change, this commit also add more descriptive types to
the `EventLog` and `MutationLog` arrays to clarify which positional
elements correspond to their serialized values.

Finally, extend the test server's `<turbo-stream>` creation actions to
support serializing an element with an `[id]` value, so that it can be
serialized from the browser to the test harness, and serve as a
`target.id` value.
@seanpdoyle
Copy link
Contributor Author

I've pushed up changes to restore the StreamActions symbol's export declaration.

Follow-up to hotwired#479
Related to hotwired/turbo-site#107

As an alternative to exposing an otherwise private and internal
`StreamActions` "class", support custom `<turbo-stream action="...">`
values the same way as custom `<turbo-frame>` rendering or `<body>`
rendering: as part of its `turbo:before-render-stream` event.

To change how Turbo renders the document during page rendering, client
applications declare a `turbo:before-render` event listener that
overrides its `CustomEvent.detail.render` function from its
[default][PageRenderer.renderElement].

Similarly, to change how Turbo renders a frame, client applications
declare a `turbo:before-frame-render` and override its
`CustomEvent.detail.render` function from its [default][].

This commit introduces the `StreamElement.renderElement` function, and
extends the existing `turbo:before-stream-render` event to support the
same pattern with `StreamElement.renderElement` server as the default
`CustomEvent.detail.render` value.

With those changes in place, callers can declare a document-wide event
listener and override based on the value of `StreamElement.action`:

```javascript
const CustomActions = {
  customUpdate: (stream) => { /* ... */ }
  customReplace: (stream) => { /* ... */ }
  // ...
}

document.addEventListener("turbo:before-stream-render", (({ target, detail }) => {
  const defaultRender = detail.render

  detail.render = CustomActions[target.action] || defaultRender
}))
```

[PageRenderer.renderElement]: https://github.com/hotwired/turbo/blob/256418fee0178ee483d82cd9bb579bd5df5a151f/src/core/drive/page_renderer.ts#L7-L13
[FrameRenderer.renderElement]: https://github.com/hotwired/turbo/blob/256418fee0178ee483d82cd9bb579bd5df5a151f/src/core/frames/frame_renderer.ts#L13-L24
@dhh dhh merged commit 9e6e0b9 into hotwired:main Sep 13, 2022
@seanpdoyle seanpdoyle deleted the before-render-stream-element branch September 13, 2022 14:06
seanpdoyle added a commit to seanpdoyle/turbo-site that referenced this pull request Dec 29, 2022
Documents [hotwired/turbo#431][]
Documents [hotwired/turbo#684][]

Share examples for `turbo:before-render`, `turbo:before-frame-render`,
and `turbo:before-stream-render` events that override the
`event.detail.render` function.

Additionally, document pausable rendering for `<turbo-frame>` elements.

[hotwired/turbo#431]: hotwired/turbo#431
[hotwired/turbo#684]: hotwired/turbo#684
seanpdoyle added a commit to seanpdoyle/turbo-site that referenced this pull request Dec 29, 2022
Documents [hotwired/turbo#431][]
Documents [hotwired/turbo#684][]

Share examples for `turbo:before-render`, `turbo:before-frame-render`,
and `turbo:before-stream-render` events that override the
`event.detail.render` function.

Additionally, document pausable rendering for `<turbo-frame>` elements.

[hotwired/turbo#431]: hotwired/turbo#431
[hotwired/turbo#684]: hotwired/turbo#684
seanpdoyle added a commit to seanpdoyle/turbo-site that referenced this pull request Dec 29, 2022
Documents [hotwired/turbo#431][]
Documents [hotwired/turbo#684][]

Share examples for `turbo:before-render`, `turbo:before-frame-render`,
and `turbo:before-stream-render` events that override the
`event.detail.render` function.

Additionally, document pausable rendering for `<turbo-frame>` elements.

[hotwired/turbo#431]: hotwired/turbo#431
[hotwired/turbo#684]: hotwired/turbo#684
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.

4 participants