Frame Visits: Cache Snapshot later in process#488
Merged
dhh merged 1 commit intohotwired:mainfrom Jul 17, 2022
Merged
Conversation
seanpdoyle
commented
Dec 1, 2021
src/core/frames/frame_controller.ts
Outdated
|
|
||
| if (frame && this.previousContents) { | ||
| frame.innerHTML = "" | ||
| frame.append(this.previousContents) |
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
Author
|
Since this depends on #487, it'd be best to skip the first commit when reviewing the changes in this branch. |
7403004 to
d8be477
Compare
Member
|
Can you update this? Let's get this in. |
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
d8be477 to
f7c454c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to hotwired/turbo#441
Depends on hotwired/turbo#487
Closes #472
When caching Snapshots during a
Visit, elements are not cached untilafter the
turbo:before-cacheevent fires. This affords clientapplications 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
SnapshotSubstitutioninstanceoccurs 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
FrameRendererDelegateinterface and theframeContentsExtracted()hook. During
<turbo-frame>rendering, theFrameRendererinstanceselects a Range of nodes and removes them by calling
Range.deleteContents. The
deleteContents()method removes theNodes and discards them.
This commit replaces the
deleteContents()call with one toRange.extractContents, so that the Nodes are retained as a
DocumentFragment instance. While handling the callback, the
FrameControllerretains that instance by setting an internalpreviousContentsproperty.Later on in the Frame rendering-to-Visit-promotion process, the
FrameControllerimplements thevisitCachedSnapshot()hook to readfrom the
previousContentsproperty and substitute the frame's contentswith the
previousContents, replacing the need for theSnapshotSubstitutionclass.