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

Frame Visits: Cache Snapshot later in process #488

Merged
merged 1 commit into from Jul 17, 2022

Conversation

seanpdoyle
Copy link
Contributor

Follow-up to hotwired/turbo#441
Depends on hotwired/turbo#487
Closes #472


When caching Snapshots during a Visit, elements are not cached until
after the turbo:before-cache event fires. This affords client
applications with an opportunity to disconnect and deconstruct and
JavaScript state, and provides an opportunity to encode that state into
the HTML so that it can survive the caching process.

The timing of the construction of the SnapshotSubstitution instance
occurs too early in the frame rendering process: the <turbo-frame>
descendants have not been disconnected. The handling of the
<turbo-frame> caching is already an exception from the norm.

Unfortunately, the current implementation is caching too early in the
process. If the Snapshot were cached too late in the process with the
rest of the page (as described in hotwired/turbo#441), the [src]
attribute and descendant content would have already changed, so any
previous state would be lost.

This commit strikes a balance between the two extremes by introducing
the FrameRendererDelegate interface and the frameContentsExtracted()
hook. During <turbo-frame> rendering, the FrameRenderer instance
selects a Range of nodes and removes them by calling
Range.deleteContents. The deleteContents() method removes the
Nodes and discards them.

This commit replaces the deleteContents() call with one to
Range.extractContents, so that the Nodes are retained as a
DocumentFragment instance. While handling the callback, the
FrameController retains that instance by setting an internal
previousContents property.

Later on in the Frame rendering-to-Visit-promotion process, the
FrameController implements the visitCachedSnapshot() hook to read
from the previousContents property and substitute the frame's contents
with the previousContents, replacing the need for the
SnapshotSubstitution class.


if (frame && this.previousContents) {
frame.innerHTML = ""
frame.append(this.previousContents)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this implementation would call ParentNode.replaceChildren, but unfortunately, typescript@4.1.x doesn't support that API. It's introduced in typescript@4.4, which might be worth upgrading to in a separate PR.

@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Dec 1, 2021

Since this depends on #487, it'd be best to skip the first commit when reviewing the changes in this branch.

@seanpdoyle seanpdoyle force-pushed the frame-visit-snapshot-timing branch 5 times, most recently from 7403004 to d8be477 Compare December 1, 2021 21:24
@dhh
Copy link
Member

dhh commented Jul 16, 2022

Can you update this? Let's get this in.

@dhh dhh added this to the 7.2.0 milestone Jul 16, 2022
Follow-up to [hotwired#441][]
Depends on [hotwired#487][]
Closes hotwired#472

---

When caching Snapshots during a `Visit`, elements are not cached until
after the `turbo:before-cache` event fires. This affords client
applications with an opportunity to disconnect and deconstruct and
JavaScript state, and provides an opportunity to encode that state into
the HTML so that it can survive the caching process.

The timing of the construction of the `SnapshotSubstitution` instance
occurs too early in the frame rendering process: the `<turbo-frame>`
descendants have not been disconnected. The handling of the
`<turbo-frame>` caching is already an exception from the norm.

Unfortunately, the current implementation is caching _too early_ in the
process. If the Snapshot were cached too late in the process with the
rest of the page (as described in [hotwired#441][]), the `[src]`
attribute and descendant content would have already changed, so any
previous state would be lost.

This commit strikes a balance between the two extremes by introducing
the `FrameRendererDelegate` interface and the `frameContentsExtracted()`
hook. During `<turbo-frame>` rendering, the `FrameRenderer` instance
selects a [Range][] of nodes and removes them by calling
[Range.deleteContents][]. The `deleteContents()` method removes the
Nodes and discards them.

This commit replaces the `deleteContents()` call with one to
[Range.extractContents][], so that the Nodes are retained as a
[DocumentFragment][] instance. While handling the callback, the
`FrameController` retains that instance by setting an internal
`previousContents` property.

Later on in the Frame rendering-to-Visit-promotion process, the
`FrameController` implements the `visitCachedSnapshot()` hook to read
from the `previousContents` property and substitute the frame's contents
with the `previousContents`, replacing the need for the
`SnapshotSubstitution` class.

[hotwired#441]: hotwired#441
[hotwired#487]: hotwired#487
[Range]: https://developer.mozilla.org/en-US/docs/Web/API/Range
[Range.deleteContents]: https://developer.mozilla.org/en-US/docs/Web/API/range/deleteContents
[Range.extractContents]: https://developer.mozilla.org/en-US/docs/Web/API/Range/extractContents
[DocumentFragment]: https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment
@dhh dhh merged commit 1811189 into hotwired:main Jul 17, 2022
@seanpdoyle seanpdoyle deleted the frame-visit-snapshot-timing branch July 17, 2022 23:35
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.

Turbo Frame with promoted navigation caching too early
2 participants