Export PageRenderer #305

mrrooijen · 2021-07-10T18:00:19Z

There are situations where you don't want the entire body to be replaced. One of the reasons would be when you're using 3rd party libraries that inject code (i.e. an iframe) such as Intercom or HelpScout, as this would result in the removal- and state-loss of their UI component every time you navigate using turbo drive.

Looking through previous issues it seems that there's no desire to add the ability to select a different node to render turbo drive responses into. So instead, it would be nice to be able to monkey-patch this functionality in so we can opt-out of this constraint. However, in order to do this we'll need access to PageRenderer, which currently isn't exported. This PR exports it.

dhh · 2021-07-12T20:56:43Z

Please do look at turning that monkey patch into more direct flexibility. @domchristie was just mentioning this in #290.

michelson · 2021-07-13T05:16:05Z

FYI, I've tackled this issue in chaskiq.io (intercom-like embed) by wrapping the embed div in a turbo-permanent. That will persist their state between navigation.

mrrooijen · 2021-07-13T10:40:47Z

@dhh Thanks for the merge!

I've already tried wrapping the node with data-turbo-permanent but it breaks the underlying iframe post-navigation/render. This solution may work (such as in @michelson's case) with various services, but not with others. It seems this is unfortunately beyond our control.

I came across the following issues that describe the problem (or similar/related problems):

Being able to intercept the rendering operation as described in #290 might be a viable solution to the problem if it'd allow you to switch the rendering target from <body> to a different node, assuming that there isn't a better solution that avoids the necessity to render into a different node entirely.

@dhh just an idea, but perhaps being able to specify a different rendering target by simply adding an optional attribute (i.e. data-turbo-drive-body) would be a minimal and straightforward way to switch targets.

Example Layout HTML diff

- <body>
-   <%= yield %> 
-   <!-- code from various services such as Stripe, HelpScout, is often injected here -->
- </body>
+ <body>
+   <div data-turbo-drive-body>
+     <%= yield %>  
+   </div>
+   <!-- code from various services such as Stripe, HelpScout, is often injected here -->
+ </body>

Example PageRenderer diff

- assignNewBody() {
-   if (document.body && this.newElement instanceof HTMLBodyElement) {
-     document.body.replaceWith(this.newElement)
-   } else {
-     document.documentElement.appendChild(this.newElement)
-   }
- }
+ assignNewBody() {
+   if (this.turboBody && this.newTurboElement) {
+     this.turboBody.replaceWith(this.newTurboElement)
+   } else if (document.body && this.newElement instanceof HTMLBodyElement) {
+     document.body.replaceWith(this.newElement)
+   } else {
+     document.documentElement.appendChild(this.newElement)
+   }
+ }
+ 
+ get turboBody() {
+   return document.querySelector("[data-turbo-drive-body]")
+ }
+ 
+ get newTurboElement() {
+   return this.newElement.querySelector("[data-turbo-drive-body]")
+ }

I haven't tried the above yet, but this is essentially what I had in mind. The default behavior stays the same (render into <body>), and if there's an element with data-turbo-drive-body, then render into that element instead.

dhh · 2021-07-13T16:26:46Z

@mrrooijen Can you explain more the issue you're trying to solve? Why isn't data-turbo-permanent working for you? What's breaking and could we fix it?

It is an interesting idea to basically invert data-turbo-permanent, such that everything NOT marked is permanent. But I'm not sure why we're able to make that work but not specifically marking something as data-turbo-permanent? Is it about detaching and attaching the nodes that trigger something?

mrrooijen · 2021-07-13T17:49:16Z

@dhh correct. When you detach an element containing an iframe, and then re-attach it, it'll cause the iframe to reload. This behavior is intended and there's no way around it as far as I know. See: https://bugs.webkit.org/show_bug.cgi?id=13574#c14

There apparently used to be a "magic iframe" that could work around this issue, but it's been removed due to security implications.

Third party nodes and iframes (Stripe, HelpScout, etc) are commonly injected just before the closing </body> tag, so replacing the entire body and then re-attaching the nodes containing the iframes will result in an undesirable reload- and thus breakage and/or the loss of state.

Therefore, the only way that I currently know of that resolves this issue is to, rather than render your content into body, instead render it into a child node such as body > div[data-turbo-drive-body]. This way Turbo Drive never touches anything inside of body other than div[data-turbo-drive-body], so the third party nodes and iframes that reside inside body but outside of div[data-turbo-drive-body] are left alone and correctly persist when navigating.

It is an interesting idea to basically invert data-turbo-permanent, such that everything NOT marked is permanent. But I'm not sure why we're able to make that work but not specifically marking something as data-turbo-permanent? Is it about detaching and attaching the nodes that trigger something?

Just to clarify, the solution that I proposed (the diffs) doesn't involve the use of data-turbo-permanent. Rather, it avoids it and turbo drive's rendering process entirely. It's not doing any inverted operations or anything complicated, really. It merely moves the rendering of the application's content from body into a child node (i.e. body > div[data-turbo-drive-body]).

domchristie · 2021-07-13T20:47:39Z

I don't have a deep knowledge of the rendering process, but my feeling is that it won't be straightforward to just change the target render element, and that this might cause future maintenance issues. (It could become a reimplementation of Turbo frames :/)

Given the uncertainty around how this might be used, my preference would be for a "canary" API using the pausable render feature. For example something like:

addEventListener('turbo:before-render', function (event) {
  event.preventDefault()
  document.getElementById('root').replaceWith(event.detail.newBody.getElementById('root'))
  event.detail.resume({ skipSnapshotRendering: true })
})

internets · 2021-07-14T15:51:04Z

Our use case will require something similar. We are embedding Twilio Flex inside of an iframe in our CRM. The iframe reload during the visit causes state loss and breaks WebRTC connections which results in dropped calls.

I made a fork that did exactly what @mrrooijen suggested with data-turbo-drive-body, and had a problem with restoration visits. We haven't had time to look deeper, but we will soon. I'm happy to chip in and/or share anything that I learn along the way.

Something like what @domchristie suggests would definitely work for our use case.

domchristie · 2021-07-14T16:04:28Z

API using the pausable render feature.
#305 (comment)

I have spiked an idea for this here: main...domchristie:custom_rendering Not tried it, but might be worth testing it out.

internets · 2021-07-15T13:47:08Z

Thanks @domchristie! Your spike does allow rendering to a child element instead of body without monkey patching PageRenderer. Unfortunately you lose the permanent element preservation and script activation that are handled by PageRenderer. For this particular use case, being able to configure the function that assigns the new body would be preferable:

addEventListener('turbo:before-render', function (event) {
  event.preventDefault()
  event.detail.resume({ 
    assignNewBody: () => {
      document.getElementById('root').replaceWith(event.detail.newBody.querySelector('#root'))
    } 
  })
})

I'm not sure if that's much better than monkey patching PageRenderer, though. Another option would be to extract those features from PageRenderer and allow people to use them in their custom renderers. That allows a high amount of flexibility without sacrificing some of turbo's great features in the process.

Either way the problem with restoration visits still exists, but it looks like @tbo already discovered the cause of that. See #218 (comment) re: snapshotting mechanism. I would love to see that one line merged, it would allow us to switch off our fork.

domchristie · 2021-07-15T15:27:54Z

Unfortunately you lose the permanent element preservation and script activation that are handled by PageRenderer.

I suppose that if you were using a DOM diffing library like morphdom then you'd likely handle permanent elements yourself? Perhaps there's a way to implement both e.g. event.detail.resume({ skipBodyAssignment: true }) and event.detail.resume({ skipSnapshotRendering: true }).

internets · 2021-07-15T16:43:01Z

I'm not a morphdom expert, but I imagine there may be cases where you have some state that you would like to keep even though the server is returning a new body that contains some default state. I don't know if morphdom has its own way to express that.

skipBodyAssignment: true almost gets there, but I believe the mutation has to happen in the callback passed to PageRenderer.preservingPermanentElements for it to work, so we can't mutate and then continue the render skipping body assignment.

Letting people provide their own PageRenderer is a potential solution. Instead of letting people monkey-patch it, you could write your own or extend the one that Turbo exports. Example:

import { Turbo, PageRenderer } from '@hotwired/turbo';

class CustomPageRenderer extends PageRenderer {
  assignNewBody() {
    document.getElementById('root').replaceWith(this.newElement.querySelector('#root'))
  }
}

Turbo.setRenderer(CustomPageRenderer)

domchristie · 2021-07-15T17:15:28Z

I'm not a morphdom expert, but I imagine there may be cases where you have some state that you would like to keep even though the server is returning a new body that contains some default state. I don't know if morphdom has its own way to express that.

I think this is possible with the onBeforeElUpdated hook:

addEventListener('turbo:before-render', async function (event) {
  event.preventDefault()
  morphdom(document.body, event.detail.newBody, {
    onBeforeElUpdated: function (fromEl, toEl) {
      const permanent = (
        fromEl.dataset.turboPermanent &&
        toEl.dataset.turboPermanent &&
        fromEl.id === toEl.id
      )
      if (permanent) return false
      return true
    }
  })
  event.detail.resume({ skipSnapshotRendering: true })
})

Letting people provide their own PageRenderer is a potential solution. … Turbo.setRenderer(CustomPageRenderer)

I think I'm generally against this approach, as the entire PageRenderer becomes public API, which then puts the burden of maintaining all of the unconventional things that could be done with it, onto the maintainers. Should PageRenderer ever need to change, it'd be breaking one :/

internets · 2021-07-16T15:37:47Z

I'm not a morphdom expert, but I imagine there may be cases where you have some state that you would like to keep even though the server is returning a new body that contains some default state. I don't know if morphdom has its own way to express that.

I think this is possible with the onBeforeElUpdated hook:

That looks straightforward to me. I like the idea. Far better than making PageRenderer public API :)

For our use case extra weight of morphdom isn't an issue, I'll give it a go with your branch and see if I can get script activation working as well.

Thanks!

internets · 2021-07-16T18:37:40Z

Not tested extensively, but what you had there more or less worked, and adding Turbo-compatible script activation was easy as well. In case anybody is curious:

function copyElementAttributes(destinationElement, sourceElement) {
  for (const { name, value } of [ ...sourceElement.attributes ]) {
    destinationElement.setAttribute(name, value)
  }
}

function createScriptElement(element) {
  if (element.getAttribute("data-turbo-eval") == "false") {
    return element
  } else {
    const createdScriptElement = document.createElement("script")
    const cspNonce = document.head.querySelector('meta[name="csp-nonce"]')?.getAttribute("content")
    if (cspNonce) {
      createdScriptElement.nonce = cspNonce
    }
    createdScriptElement.textContent = element.textContent
    createdScriptElement.async = false
    copyElementAttributes(createdScriptElement, element)
    return createdScriptElement
  }
}

addEventListener('turbo:before-render', async function (event) {
  event.preventDefault()
  morphdom(document.body, event.detail.newBody, {
    onNodeAdded: function (node) {
      if (node.nodeName === 'SCRIPT') {
        const activatedScriptElement = createScriptElement(node)
        node.replaceWith(activatedScriptElement)
      }
    },
    onBeforeElUpdated: function (fromEl, toEl) {
      const permanent = (
        fromEl.dataset.turboPermanent !== undefined &&
        toEl.dataset.turboPermanent !== undefined &&
        fromEl.id === toEl.id
      )
      if (permanent) return false

      if (fromEl.nodeName === "SCRIPT" && toEl.nodeName === "SCRIPT") {
        const activatedScriptElement = createScriptElement(toEl)
        fromEl.replaceWith(activatedScriptElement)

        return false;
      }

      return true
    }
  })
  event.detail.resume({ skipSnapshotRendering: true })
})

While adopting morphdom and copying some turbo functionality can work for us, I can see value in also having an official Turbo solution using @mrrooijen's suggestion of specifying a target for body assignment with something like data-turbo-drive-body. It allows people with iframes that can't be reloaded to use Turbo without adding additional libraries and duplicating Turbo functionality, and despite increasing the API surface of Turbo it is a small change code-wise (once mutating renders are supported).

In other words, I like both skipSnapshotRendering as a way to allow custom rendering and data-turbo-drive-body as a way to work around iframe issues.

mrrooijen · 2021-07-22T08:48:07Z

With PageRenderer now being exportable, the following works for me.

import { PageRenderer } from "@hotwired/turbo"

PageRenderer.prototype.assignNewBody = function() {
  const body = document.querySelector("[data-turbo-drive-body]")
  const el   = this.newElement.querySelector("[data-turbo-drive-body]")

  if (body && el) {
    body.replaceWith(el)
  } else if (document.body && this.newElement instanceof HTMLBodyElement) {
    document.body.replaceWith(this.newElement)
  } else {
    document.documentElement.appendChild(this.newElement)
  }
}

<head>
  <meta content="no-cache" name="turbo-cache-control" />
</head>
<body>
  <div data-turbo-drive-body>
    <%= yield %>
  </div>
</body>

Page restorations appear to work for me because I'm not caching pages. Otherwise, as @internets pointed out, it will break.

internets · 2021-07-22T14:11:33Z

Page restorations appear to work for me because I'm not caching pages. Otherwise, as @internets pointed out, it will break.

If you'd like to continue caching, you can eliminate the pause before cloning the snapshot. Here's the monkey patch:

Turbo.navigator.view.cacheSnapshot = function() {
  if (this.shouldCacheSnapshot) {
    this.delegate.viewWillCacheSnapshot()
    const { snapshot, lastRenderedLocation: location } = this
    // await nextEventLoopTick()
    this.snapshotCache.put(location, snapshot.clone())
  }
}

I'm not 100% sure yet, but I believe the await nextEventLoopTick()is a performance optimization. The test suite passes with that modification, so it should be mostly safe :)

rawberg · 2021-08-28T19:50:19Z

Thanks for exporting this, this works great!

import morphdom from "morphdom";
import { PageRenderer } from "@hotwired/turbo";

PageRenderer.prototype.assignNewBody = function() {
    if (document.body && this.newElement instanceof HTMLBodyElement) {
        morphdom(document.body, this.newElement, {
            onBeforeElUpdated: function(fromEl, toEl) {
                return !fromEl.isEqualNode(toEl);
            }
        });
    } else {
        document.documentElement.appendChild(this.newElement)
    }
}

agrobbin · 2022-07-13T17:43:32Z

For those coming across this issue, who might be looking for a more limited patch that still allows for caching to function as expected, I went about it by actually adding await nextEventLoopTick() to PageRenderer#assignNewBody():

// copied since `util` is not exported
const nextEventLoopTick = () => new Promise((resolve) => {
  setTimeout(() => resolve(), 0);
});

const findBodyElement = (body) => body.querySelector('[data-turbo-drive-body]') || body;

PageRenderer.prototype.assignNewBody = async function assignNewBody() {
  await nextEventLoopTick();

  if (document.body && this.newElement instanceof HTMLBodyElement) {
    const currentBody = findBodyElement(document.body);
    const newBody = findBodyElement(this.newElement);

    currentBody.replaceWith(newBody);
  } else {
    document.documentElement.appendChild(this.newElement);
  }
};

The order of execution without adding await nextEventLoopTick() to PageRenderer#assignNewBody() seems to be:

PageView#cacheSnapshot() before await nextEventLoopTick()
PageRenderer#assignNewBody()
PageView#cacheSnapshot() after await nextEventLoopTick()

By adding await nextEventLoopTick() to PageRenderer#assignNewBody(), (2) and (3) get flipped. This way, PageRenderer#assignNewBody() waits until PageView#cacheSnapshot() has finished, before replacing part of document.body.

I believe the reason this isn't an issue if you replace all of document.body is that the old HTMLBodyElement has not been manipulated, so the out-of-order execution doesn't have any practical implications on what is cloned. Since the HTMLBodyElement is not different, the out-of-order execution has a clear impact.

While most of the time, replacing `<body>` makes perfect sense, when working with 3rd party integrations (i.e. Stripe), there are elements injected just before the closing `</body>` tag that should not be removed between page visits. There is more detail on this issue, particularly with injected `<iframe>` elements in hotwired#305 (comment). Now, if someone wants to customize the element that is replaced by Drive, they can add `data-turbo-drive-body` to an element, and only that element will be replaced between visits.

While most of the time, replacing `<body>` makes perfect sense, when working with 3rd party integrations (i.e. Stripe), there are elements injected just before the closing `</body>` tag that should not be removed between page visits. There is more detail on this issue, particularly with injected `<iframe>` elements in #305 (comment). Now, if someone wants to customize the element that is replaced by Drive, they can add `data-turbo-drive-body` to an element, and only that element will be replaced between visits.

scottnicolson · 2022-10-20T11:01:42Z

For anyone that takes inspiration from #305 (comment) please be aware that the code provided fixes the cache visits issue, but for non-cache visits, if the new body and old body contain data-turbo-permanent elements, they do not get reattached. The solution seems to work fine when then body doesn't contain turbo-permanent elements, so just use with care until a proper solution is available.

manuelpuyol · 2022-10-28T17:02:25Z

@agrobbin @scottnicolson I'd like to point that implementing #305 (comment) causes turbo:load to be fired before we actually replace the element, so this may cause some unwanted behaviors

manuelpuyol · 2022-10-28T17:32:17Z

-    static preservingPermanentElements(delegate, permanentElementMap, callback) {
+    static async preservingPermanentElements(delegate, permanentElementMap, callback) {
         const bardo = new this(delegate, permanentElementMap);
         bardo.enter();
-        callback();
+        await callback();
         bardo.leave();
     }
     enter() {
@@ -1225,8 +1225,8 @@ class Renderer {
             delete this.resolvingFunctions;
         }
     }
-    preservingPermanentElements(callback) {
-        Bardo.preservingPermanentElements(this, this.permanentElementMap, callback);
+    async preservingPermanentElements(callback) {
+        await Bardo.preservingPermanentElements(this, this.permanentElementMap, callback);
     }
     focusFirstAutofocusableElement() {
         const element = this.connectedSnapshot.firstAutofocusableElement;
@@ -2582,7 +2582,7 @@ class PageRenderer extends Renderer {
     }
     async render() {
         if (this.willRender) {
-            this.replaceBody();
+            await this.replaceBody();
         }
     }
     finishRendering() {
@@ -2607,10 +2607,10 @@ class PageRenderer extends Renderer {
         this.copyNewHeadProvisionalElements();
         await newStylesheetElements;
     }
-    replaceBody() {
-        this.preservingPermanentElements(() => {
+    async replaceBody() {
+        await this.preservingPermanentElements(async () => {
             this.activateNewBody();
-            this.assignNewBody();
+            await this.assignNewBody();
         });
     }
     get trackedElementsAreIdentical() {
@@ -2649,8 +2649,8 @@ class PageRenderer extends Renderer {
             inertScriptElement.replaceWith(activatedScriptElement);
         }
     }
-    assignNewBody() {
-        this.renderElement(this.currentElement, this.newElement);
+    async assignNewBody() {
+        await this.renderElement(this.currentElement, this.newElement);
     }

I think these are the minimal changes to make turbo:load correctly fire after the custom render actually replaces the elements

Export PageRenderer

2e1c803

dhh merged commit a84d25d into hotwired:main Jul 12, 2021

mrrooijen deleted the export-PageRenderer branch July 13, 2021 09:49

tbo mentioned this pull request Jul 13, 2021

[RFC] Cancelable rendering #218

Closed

botandrose mentioned this pull request Apr 13, 2022

Expose FrameRenderer so we can extend its behavior, e.g. vDOM. #568

Merged

agrobbin mentioned this pull request Jul 14, 2022

Allow customizing the element that Drive replaces #627

Merged

seanpdoyle mentioned this pull request Jul 16, 2022

Support custom rendering in turbo:before{-frame,}-render events #431

Merged

agrobbin mentioned this pull request Sep 13, 2022

Re-introduce #627 #711

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export PageRenderer #305

Export PageRenderer #305

mrrooijen commented Jul 10, 2021

dhh commented Jul 12, 2021

michelson commented Jul 13, 2021 •

edited

mrrooijen commented Jul 13, 2021 •

edited

dhh commented Jul 13, 2021

mrrooijen commented Jul 13, 2021 •

edited

domchristie commented Jul 13, 2021 •

edited

internets commented Jul 14, 2021

domchristie commented Jul 14, 2021

internets commented Jul 15, 2021

domchristie commented Jul 15, 2021

internets commented Jul 15, 2021

domchristie commented Jul 15, 2021

internets commented Jul 16, 2021

internets commented Jul 16, 2021

mrrooijen commented Jul 22, 2021

internets commented Jul 22, 2021

rawberg commented Aug 28, 2021 •

edited

agrobbin commented Jul 13, 2022 •

edited

scottnicolson commented Oct 20, 2022

manuelpuyol commented Oct 28, 2022

manuelpuyol commented Oct 28, 2022

Export PageRenderer #305

Export PageRenderer #305

Conversation

mrrooijen commented Jul 10, 2021

dhh commented Jul 12, 2021

michelson commented Jul 13, 2021 • edited

mrrooijen commented Jul 13, 2021 • edited

dhh commented Jul 13, 2021

mrrooijen commented Jul 13, 2021 • edited

domchristie commented Jul 13, 2021 • edited

internets commented Jul 14, 2021

domchristie commented Jul 14, 2021

internets commented Jul 15, 2021

domchristie commented Jul 15, 2021

internets commented Jul 15, 2021

domchristie commented Jul 15, 2021

internets commented Jul 16, 2021

internets commented Jul 16, 2021

mrrooijen commented Jul 22, 2021

internets commented Jul 22, 2021

rawberg commented Aug 28, 2021 • edited

agrobbin commented Jul 13, 2022 • edited

scottnicolson commented Oct 20, 2022

manuelpuyol commented Oct 28, 2022

manuelpuyol commented Oct 28, 2022

michelson commented Jul 13, 2021 •

edited

mrrooijen commented Jul 13, 2021 •

edited

mrrooijen commented Jul 13, 2021 •

edited

domchristie commented Jul 13, 2021 •

edited

rawberg commented Aug 28, 2021 •

edited

agrobbin commented Jul 13, 2022 •

edited