Skip to content

Commit

Permalink
Infer renderElement during Renderer construction
Browse files Browse the repository at this point in the history
Follow-up to [hotwired#1192][]
Closes [hotwired#1303][]

Remove the argument from all `Renderer` subclass constructor call sites.
In its place, define the default value for the `Renderer.renderElement`
property based on the `static renderElement(currentElement, newElement)`
defined by the inheriting class.

By automatically determining which `renderElement` implementation based
on class, this commit enables a simpler Frame morphing strategy. Instead
of attaching one-time event listeners, this commit maintains a private
`#shouldMorphFrame` property that is set, then un-set during the
rendering lifecycle of a frame.

Similarly, this commit implements the
`MorphingFrameRenderer.preservingPermanentElements` method in the same
style as the `MorphingPageRenderer.preservingPermanentElements`. Since
_instances_ of `MorphingFrameRenderer` are now being used instead of the
static `MorphingFrameRenderer.renderElement`, we're able to utilize an
instance method instead of passing around an additional anonymous
function (which is the implementation that [hotwired#1303][] proposed).

[hotwired#1192]: hotwired#1192
[hotwired#1303]: hotwired#1303

Co-authored-by: Micah Geisel <micah@botandrose.com>
  • Loading branch information
seanpdoyle and botandrose-machine committed Sep 12, 2024
1 parent ca5899d commit 03549a9
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 12 deletions.
4 changes: 2 additions & 2 deletions src/core/drive/page_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class PageView extends View {
const shouldMorphPage = this.isPageRefresh(visit) && this.snapshot.shouldMorphPage
const rendererClass = shouldMorphPage ? MorphingPageRenderer : PageRenderer

const renderer = new rendererClass(this.snapshot, snapshot, rendererClass.renderElement, isPreview, willRender)
const renderer = new rendererClass(this.snapshot, snapshot, isPreview, willRender)

if (!renderer.shouldRender) {
this.forceReloaded = true
Expand All @@ -32,7 +32,7 @@ export class PageView extends View {

renderError(snapshot, visit) {
visit?.changeHistory()
const renderer = new ErrorRenderer(this.snapshot, snapshot, ErrorRenderer.renderElement, false)
const renderer = new ErrorRenderer(this.snapshot, snapshot, false)
return this.render(renderer)
}

Expand Down
15 changes: 7 additions & 8 deletions src/core/frames/frame_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export class FrameController {
#connected = false
#hasBeenLoaded = false
#ignoredAttributes = new Set()
#shouldMorphFrame = false
action = null

constructor(element) {
Expand Down Expand Up @@ -90,13 +91,10 @@ export class FrameController {
}

sourceURLReloaded() {
if (this.element.shouldReloadWithMorph) {
this.element.addEventListener("turbo:before-frame-render", ({ detail }) => {
detail.render = MorphingFrameRenderer.renderElement
}, { once: true })
}
const { refresh, src } = this.element

this.#shouldMorphFrame = src && refresh === "morph"

const { src } = this.element
this.element.removeAttribute("complete")
this.element.src = null
this.element.src = src
Expand Down Expand Up @@ -139,6 +137,7 @@ export class FrameController {
}
}
} finally {
this.#shouldMorphFrame = false
this.fetchResponseLoaded = () => Promise.resolve()
}
}
Expand Down Expand Up @@ -304,11 +303,11 @@ export class FrameController {

async #loadFrameResponse(fetchResponse, document) {
const newFrameElement = await this.extractForeignFrameElement(document.body)
const rendererClass = this.#shouldMorphFrame ? MorphingFrameRenderer : FrameRenderer

if (newFrameElement) {
const snapshot = new Snapshot(newFrameElement)
const renderer = new FrameRenderer(this, this.view.snapshot, snapshot, FrameRenderer.renderElement, false, false)

const renderer = new rendererClass(this, this.view.snapshot, snapshot, false, false)
if (this.view.renderPromise) await this.view.renderPromise
this.changeHistory()

Expand Down
4 changes: 4 additions & 0 deletions src/core/frames/morphing_frame_renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,8 @@ export class MorphingFrameRenderer extends FrameRenderer {

morphChildren(currentElement, newElement)
}

async preservingPermanentElements(callback) {
return await callback()
}
}
8 changes: 6 additions & 2 deletions src/core/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@ import { Bardo } from "./bardo"
export class Renderer {
#activeElement = null

constructor(currentSnapshot, newSnapshot, renderElement, isPreview, willRender = true) {
static renderElement(currentElement, newElement) {
// Abstract method
}

constructor(currentSnapshot, newSnapshot, isPreview, willRender = true) {
this.currentSnapshot = currentSnapshot
this.newSnapshot = newSnapshot
this.isPreview = isPreview
this.willRender = willRender
this.renderElement = renderElement
this.renderElement = this.constructor.renderElement
this.promise = new Promise((resolve, reject) => (this.resolvingFunctions = { resolve, reject }))
}

Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/frames.html
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ <h2>Frames: #frame</h2>
<a id="link-frame-with-search-params" href="/src/tests/fixtures/frames/frame.html?key=value">Navigate #frame with ?key=value</a>
<a id="link-nested-frame-action-advance" href="/src/tests/fixtures/frames/frame.html" data-turbo-action="advance">Navigate #frame from within with a[data-turbo-action="advance"]</a>
<a id="link-top" href="/src/tests/fixtures/one.html" data-turbo-frame="_top">Visit one.html</a>
<input id="permanent-input" data-turbo-permanent>
<form action="/src/tests/fixtures/one.html" data-turbo-frame="_top">
<button id="form-submit-top">Visit one.html</button>
</form>
Expand Down
12 changes: 12 additions & 0 deletions src/tests/functional/frame_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,18 @@ test("calling reload on a frame[refresh=morph] morphs the contents", async ({ pa
expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()
})

test("calling reload on a frame[refresh=morph] preserves [data-turbo-permanent] elements", async ({ page }) => {
await page.click("#add-src-to-frame")
await page.click("#add-refresh-morph-to-frame")
const input = await page.locator("#permanent-input")

await input.fill("Preserve me")
await page.evaluate(() => document.getElementById("frame").reload())

await expect(input).toBeFocused()
await expect(input).toHaveValue("Preserve me")
})

test("following a link in rapid succession cancels the previous request", async ({ page }) => {
await page.click("#outside-frame-form")
await page.click("#outer-frame-link")
Expand Down

0 comments on commit 03549a9

Please sign in to comment.