[RFC] Cancelable rendering #218
Conversation
| await this.renderSnapshot(renderer) | ||
| const event = this.delegate.viewWillRenderSnapshot(snapshot, isPreview) | ||
| if (!event.defaultPrevented) { | ||
| await this.renderSnapshot(renderer) |
There was a problem hiding this comment.
Snapshot rendering involves more than setting innerHTML (e.g. data-turbo-permanent element transfer).
I wonder if the framework could provide some form of "renderer" interface as part of the event.detail payload, then allow the event listeners to provide their own (via event.detail.render = ...) instead of preventing the default.
There was a problem hiding this comment.
You are right. I didn't consider that.
I was exploring another route as well. It is possible to overload an Element's replaceChild function. Either by monkey-patching it:
document.documentElement.replaceChild = <T extends Node>(newChild: Node, oldChild: T) {
const clone = oldChild.cloneNode(true) as T;
morphdom(oldChild, newChild);
return clone;
};Or by extending the elements with web components. Its usage could look like this:
<body is="morphable">Turbo Frames could be extended in a similar fashion (class MorphableTurboFrame extends TurboFrame { ...). The good thing about this solution would be that it requires basically no change to turbo drive. Only the cloning of the body before caching it would be needed for mutating renderers. I'm still a bit on the fence what the cleanest solution would be.
There was a problem hiding this comment.
Well I guess it is up to @sstephenson to decide how the API should look like. So some input about the general direction would be much appreciated.
|
Do you need anything beyond what #290 offers? |
|
@dhh #290 doesn't quite work for my use case. I just want to replace the render logic and leave the rest untouched. I think this case was already discussed here. I might be able to monkey patch the There is also an additional issue that hasn't been addressed in the other PRs. The snapshotting mechanism doesn't account for mutating renderers with references to the old DOM. If the DOM is being partially reused between renders (e.g. in DOM Diffing), then some mutations are applied to the snapshot as well. The default renderer simply replaces the old DOM and therefore doesn't have this issue. The fix is rather simple. I could create a separate PR for it. The rest of this PR is outdated and I would propose that we find an alternative solution. The discussions in #290 and #305 look promising. |
|
@tbo Please do investigate a proper supported solution for custom rendering 👍 |
Thanks for tracking that issue down, @tbo! I would love to see the fix for the snapshotting mechanism w/ mutating renderers in a separate PR. That (combined with the exported PageRenderer) would give us a workable solution for preventing iframe refresh. |
|
@internets I'll create a separate PR for the fix over the weekend. |
| @@ -13,7 +13,8 @@ export class PageSnapshot extends Snapshot<HTMLBodyElement> { | |||
| } | |||
|
|
|||
| static fromDocument({ head, body }: Document) { | |||
| return new this(body as HTMLBodyElement, new HeadSnapshot(head)) | |||
| // We need to clone the body because a custom renderer might mutate it. | |||
| return new this((body.cloneNode(true)) as HTMLBodyElement, new HeadSnapshot(head)) | |||
There was a problem hiding this comment.
Unfortunately I think this breaks scrolling. I think because:
Line 26 in 49f8213
There was a problem hiding this comment.
I just glanced over the implementation, but getElementForAnchor queries the snapshot DOM and shouldn't rely on any fixed reference within.
I'll build a small prototype and make sure that this case is covered.
There was a problem hiding this comment.
FWIW I have been working on a proof-of-concept for custom rendering on this branch: main...domchristie:custom_rendering
Demo here: https://glitch.com/edit/#!/magnificent-four-friend?path=turbo.js%3A701%3A70
If you put a debugger on line 702 of turbo.js, and inspect element, element.isConnected is false
There was a problem hiding this comment.
There are definitely test failures when cloning the body in Snapshot.fromDocument, not just with anchor scrolling but also with permanent elements, preserving video playback, and possibly others. A lot of render operations depends on the snapshot element being connected.
Turbo already clones the snapshot before putting it into the cache, but it does it after a delay. That delay is the reason why our mutated snapshot makes it into the snapshotCache. If we eliminate the delay, our mutating render works like we want it to. I'm not 100% sure of the reason behind the delay, but I believe it to be a performance optimization.
#101 describes an issue with the old solution of waiting a micro task tick (still too long for mutating renders), and the original implementation (waiting a micro task) can be seen in a7d02f0#diff-9519a55c38756c9b6bee1838521a79ce71017312f257330c52f19e469083155aR32-R39 with no documentation on the reason for waiting.
Out of curiosity I did build the minimal reproduction referenced in #101 in glitch with no delay before cloning here https://glitch.com/edit/#!/half-northern-thunder?path=index.html%3A7%3A30 and I am unable to reproduce the problem outlined. So at least this wouldn't cause a regression on that :)
Here's what the method would look like:
cacheSnapshot() {
if (this.shouldCacheSnapshot) {
this.delegate.viewWillCacheSnapshot()
const { snapshot, lastRenderedLocation: location } = this
this.snapshotCache.put(location, snapshot.clone())
}
}All tests pass with that change. Thoughts?
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
The problem
---
The rendering process during a page-wide navigation is opaque, and
cannot be extended. Proposals have been made to use third-party
rendering tools like [morphdom][], or other animation libraries.
Outside of the HTML manipulation, Turbo is also responsible for loading
script, transposing permanent elements, etc.
How might these tools integrate with Turbo in a way that's compliant
with permanent elements.
The solution
---
When publishing a `turbo:before-render` event, dispatch it with a
`render()` function property in addition to the `resume()`.
This way, consumer applications can override rendering:
```html
import morphdom from "morphdom"
addEventListener("turbo:before-render", ({ detail }) => {
detail.render = (currentElement, newElement) => {
morphdom(currentElement, newElement)
}
// or, more tersely
detail.render = morphdom
})
```
Potentially Closes [hotwired#197][]
Potentially Closes [hotwired#378][]
Potentially Closes [hotwired#218][]
[morphdom]: https://github.com/patrick-steele-idem/morphdom
[hotwired#218]: hotwired#218
[hotwired#378]: hotwired#378
[hotwired#197]: hotwired#197
This PR allows to cancel the default rendering operation in any
turbo:before-renderhandler. Its intended usage is to allow the application of custom rendering logic (e.g. DOM diffing). It is compatible with full page and frame based rendering. Example usage:This should close #197.
I would add the necessary tests if a maintainer agrees with the general approach.