Fire 'turbo:frame-render' event after turbo frame renders the view

[kapantzak]

Hello all!

This PR introduces the turbo:frame-render event which
gets fired right after a turbo-frame element renders its view.

You can access the current turbo-frame id with event.detail.frameId with
event.target and the fetch response object with event.detail.fetchResponse.

[t27duck]

Any reason why event.detail.frame couldn't be this.element which should be the actual turbo frame element instead of only its id?

[kapantzak]

Any reason why event.detail.frame couldn't be this.element which should be the actual turbo frame element instead of only its id?

Not really, just 2 concerns:

• Is it better to carry the whole frame object inside the event's detail instead of just its id?
• this.assert.equal(frame.id, 'part') fails (id is undefined)

If you think that we should send the object itself, please let me know and I'll change it!

[t27duck]

* `this.assert.equal(frame.id, 'part')` fails (`id` is undefined)

I'm curious then how this is working in the code:

  const frame = this.element.id;
  dispatch("turbo:after-frame-render", { cancelable: true, detail: { fetchResponse, frame } })

If frame = this.element.id is getting the correct id, then why would frame = this.element being sent as part of the event detail result in frame.id being undefined?

If the id were to be the only thing passed, then perhaps naming it frameId instead of frame would make it more descriptive. To me, "frame" implies some form of object rather than its id.

[kapantzak]

I'm curious then how this is working in the code:

I guess that the testing suite represents the elements in a different way, causing the frame element to have an undefined id.

If the id were to be the only thing passed, then perhaps naming it frameId instead of frame would make it more descriptive. To me, "frame" implies some form of object rather than its id.

Agree with the naming, if we pass only the id, frameId would be a better name 👍

[dhh]

I think the right event name should probably be turbo:frame-render to match turbo:render, no? Since the style is that these are automatically "after".

[dhh]

Also, we should probably stick with the style where we declare this in Session following the form of notifyApplicationAfterFrameRender.

[dhh]

We should pull in turbo:frame-load as well: #59

[dhh]

#59 is using this.element to put in the event, rather than the ID. I think that's a better form to follow here as well.

[kapantzak]

Great! So, I'll try to implement notifyApplicationAfterFrameRender inside session and pass the element itself rather than the id, asap.

I will also pull in turbo:frame-load as well.

[kapantzak]

I've pushed this commit which introduces the notifyApplicationAfterFrameRender inside session.

There, I've passed the frame element as the target of the event and removed it from the detail object, so, now we can get the frame element from the event.target property. I think it makes more sense this way, WDYT?

[dhh]

Looks good 👍

[kapantzak]

I also cherry-picked #59's commit and resolved some conflicts

[zeto]

@kapantzak (thanks for the contribution!) context:

const html = await fetchResponse.responseHTML
if (html) {
  const { body } = parseHTMLDocument(html)
  const snapshot = new Snapshot(await this.extractForeignFrameElement(body))
  const renderer = new FrameRenderer(this.view.snapshot, snapshot, false, false)
  if (this.view.renderPromise) await this.view.renderPromise
  await this.view.render(renderer)
  session.frameRendered(fetchResponse, this.element)
  session.frameLoaded(this.element)
  this.fetchResponseLoaded(fetchResponse)
}

Assuming 🤔 the main intention to provide fetchResponse in the Event detail is to have access to the HTML Document, the most likely (?) use case would be something like:

document.addEventListener("turbo:frame-render", () => {
  event.detail.fetchResponse.responseHTML.then((html) => {
    const body = (new DOMParser()).parseFromString(html, "text/html")
  })
})

In order to avoid reparsing what was just parsed, the detail of the Event could have the already parsed body instead of the fetchResponse (or in addition to), seeing as at the time the Promise has already been fulfilled. Similar to the turbo:before-render event. Would this make sense ?

[kapantzak]

Hello @zeto , I think it makes sense! But maybe we should just add it as an extra prop instead of replacing the old one in order to avoid breaking existing imementations