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

Page refreshes #1019

Merged
merged 17 commits into from
Nov 14, 2023
Merged

Page refreshes #1019

merged 17 commits into from
Nov 14, 2023

Conversation

afcapel
Copy link
Collaborator

@afcapel afcapel commented Oct 5, 2023

This PR introduces the concept of page refresh. A page refresh happens when Turbo renders the current page again. We will offer two new options to control behavior when a page refresh happens:

  • The method used to update the page: with a new option to use morphing (Turbo currently replaces the body).

  • The scroll strategy: with a new option to keep it (Turbo currently resets scroll to the top-left).

The combination of morphing and scroll-keeping results in smoother updates that keep the screen state. For example, this will keep both horizontal and vertical scroll, the focus, the text selection, CSS transition states, etc.

We will also introduce a new turbo stream action that, when broadcasted, will request a page refresh. This will offer a simplified alternative to fine-grained broadcasted turbo-stream actions.

body-replacement.mov
morphing.mov

You can learn more about the change here.

By @afcapel and @jorgemanrubia.

API

Page refresh configuration

Turbo will detect page refreshes automatically when it renders the current location again. The user configures how those page refreshes are handled.

Configurable refresh method

<meta name="turbo-refresh-method" content="replace|morph">
  • replace: Current behavior: it replaces the body.
  • morph: It morphs the existing body to become the new body.

The default is replace. We may switch to morph eventually.

Configurable scroll behavior:

<meta name="turbo-refresh-scroll" content="reset|preserve">
  • reset: Current behavior: it resets the scroll to the top-left.
  • preserve: It keeps the scroll in place.

The default is reset. We may switch to preserve eventually.

Support via helpers

See companion PR in turbo-rails.

turbo_refreshes_with scroll method: :morph, scroll: :preserve

Exclude sections from morphing

There are scenarios where you might want to define sections that you want to ignore while morphing. The most common scenario is a popover or similar UI elements, which state you want to preserve when the page refreshes. We reuse the existing data-turbo-permanent attribute to flag such elements.

<div data-turbo-permanent>...</div>

Broadcast changes

Turbo stream action to signal a refresh

This introduces a new turbo stream action called refresh that will trigger a page refresh:

<turbo-stream action="refresh"></turbo-stream>

Broadcast changes that require a page refresh

There is a new method in active record's models to broadcast a page refresh signal when the record changes. This is implemented internally by broadcasting the turbo stream action referred to above.

module Recording::Broadcasting
  extend ActiveSupport::Concern

  included do
    broadcasts_refreshes_to :bucket
  end
end

In view templates, you would subscribe to these updates using the regular turbo_stream_from helper:

turbo_stream_from bucket

The system will debounce automatically sequences of updates since it doesn't make sense to trigger multiple signals for multiple updates in a row.

You can learn more about the Rails helpers in the companion PR in turbo-rails hotwired/turbo-rails#499

Implementation notes

Idiomorph

We started with morphdom as the DOM-tree morphing library but we eventually changed to idiomorph. The reason is that we found morphdom to be very picky about ids when matching nodes, when idiomorph is way more relaxed. In practice, we found that with morphdom you had to keep adding ids everywhere if you wanted morphing to work, while idiomorph worked out of the box with pretty complex HTML structures.

Mechanism against bounced signals

The system includes a mechanism to ignore refresh actions that originated in a web request that originated in the same browser session. To enable this, these refresh actions can carry a request-id attribute to track the request that originated them. Turbo will keep a list of recent requests and, when there is a match, the stream action is discarded.

This is implemented by patching fetch to inject a random request id in a custom header X-Turbo-Request-Id. We originally intended to use the Rails' standard X-Request-Id in the response, but we found a blocking problem: it's not possible with Javascript to read the response headers in a redirect response, you can just read the headers in the target destination response.

The server side where the broadcasts originate is responsible for using that header to flag broadcasted page refreshes with it (==see turbo-rails PR with the reference implementation == ).

Pending

  • Document in guides.
  • Complete code documentation.

@seanpdoyle
Copy link
Contributor

This has the potential to close #37.

@seanpdoyle
Copy link
Contributor

Exclude sections from morphing

There are scenarios where you might want to define sections that you want to ignore while morphing. The most common scenario is a popover or similar UI elements, which state you want to preserve when the page refreshes. We reuse the existing data-turbo-permanent attribute to flag such elements.

Prior to this behavior, the presence of [data-turbo-permanent] required the presence of an [id] attribute. Does this change mean that [data-turbo-permanent] on its own has new morph-only semantics, or will elements still require [id] attributes to be skipped during morphing?

if (node instanceof HTMLElement && node.hasAttribute("data-controller")) {
const originalAttribute = node.getAttribute("data-controller")
node.removeAttribute("data-controller")
await nextAnimationFrame()
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an open issue (#920) about the effects of requestAnimationFrame() being our timing mechanism. Animation frame callbacks might not fire when the user is browsing another tab.

I wonder if there's another timing mechanism that could be used, since morphs are able to be broadcast over WebSockets.

Maybe nextEventLoopTick() or nextMicrotask() are decent candidates?

turbo/src/util.js

Lines 50 to 56 in c207f5b

export function nextEventLoopTick() {
return new Promise((resolve) => setTimeout(() => resolve(), 0))
}
export function nextMicrotask() {
return Promise.resolve()
}

Choose a reason for hiding this comment

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

Is it cohesively correct that Turbo knows about Stimulus controllers and how to trigger their reload?
Or is it better that Stimulus knows about Turbo morph and configures itself when to reload?

Copy link
Member

Choose a reason for hiding this comment

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

@jvillarejo the proposed implementation doesn't reload stimulus controllers automatically in general, only when a node is morphed (e.g: an attribute or its text content changed). We believe this is a good heuristic for stimulus controllers that modify the underlying element when connected.

As a general rule, I agree that that morphing should leave stimulus controllers alone.

Copy link
Contributor

@seanpdoyle seanpdoyle Oct 17, 2023

Choose a reason for hiding this comment

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

is it better that Stimulus knows about Turbo morph and configures itself when to reload?

Could Stimulus detect the presence of Turbo (either the package, or the new <meta> elements) and attach a document-wide turbo:render listener to respond to renders with renderMethod == "morph"? I might be misunderstand the file, but Stimulus defines a defaultSchema export with [data-controller] and friends as the default values in what appears to be a configurable Object.

export const defaultSchema: Schema = {
  controllerAttribute: "data-controller",
  actionAttribute: "data-action",
  targetAttribute: "data-target",
  // ...
}

Comment on lines 39 to 40
window.Turbo.cache.exemptPageFromPreview()
window.Turbo.visit(window.location.href, { action: "replace" })
Copy link
Contributor

@seanpdoyle seanpdoyle Oct 5, 2023

Choose a reason for hiding this comment

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

Is there an advantage to reaching for the window.Turbo.cache instance and window.Turbo.visit instead of importing them from ../../core?

export { navigator, session, cache, PageRenderer, PageSnapshot, FrameRenderer }

turbo/src/core/index.js

Lines 33 to 49 in c207f5b

/**
* Performs an application visit to the given location.
*
* @param location Location to visit (a URL or path)
* @param options Options to apply
* @param options.action Type of history navigation to apply ("restore",
* "replace" or "advance")
* @param options.historyChanged Specifies whether the browser history has
* already been changed for this visit or not
* @param options.referrer Specifies the referrer of this visit such that
* navigations to the same page will not result in a new history entry.
* @param options.snapshotHTML Cached snapshot to render
* @param options.response Response of the specified location
*/
export function visit(location, options) {
session.visit(location, options)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Importing would be nicer, but unfortunately it creates a circular dependency:

(!) Circular dependency
src/core/index.js -> src/core/streams/stream_actions.js -> src/core/index.js
created dist/turbo.es2017-umd.js, dist/turbo.es2017-esm.js in 225ms

Copy link
Contributor

@seanpdoyle seanpdoyle Oct 8, 2023

Choose a reason for hiding this comment

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

Ah, thanks for exploring that @afcapel.

Since Turbo isn't really an instance of a class or namespace, but rather a collection of other exports from the export * as Turbo statement, what do you think about importing the individual components themselves?

I've had some success with this diff locally:

diff --git a/src/core/index.js b/src/core/index.js
index 743380d..34269c5 100644
--- a/src/core/index.js
+++ b/src/core/index.js
@@ -12,8 +12,6 @@ const recentRequests = new LimitedSet(20)
 const { navigator } = session
 export { navigator, session, cache, recentRequests, PageRenderer, PageSnapshot, FrameRenderer }
 
-export { StreamActions } from "./streams/stream_actions"
-
 /**
  * Starts the main session.
  * This initialises any necessary observers such as those to monitor
diff --git a/src/core/streams/stream_actions.js b/src/core/streams/stream_actions.js
index 62801fd..e8e20bd 100644
--- a/src/core/streams/stream_actions.js
+++ b/src/core/streams/stream_actions.js
@@ -1,3 +1,5 @@
+import { cache, recentRequests, visit } from "../"
+
 export const StreamActions = {
   after() {
     this.targetElements.forEach((e) => e.parentElement?.insertBefore(this.templateContent, e.nextSibling))
@@ -34,10 +36,10 @@ export const StreamActions = {
 
   refresh() {
     const requestId = this.getAttribute("request-id")
-    const isRecentRequest = requestId && window.Turbo.recentRequests.has(requestId)
+    const isRecentRequest = requestId && recentRequests.has(requestId)
     if (!isRecentRequest) {
-      window.Turbo.cache.exemptPageFromPreview()
-      window.Turbo.visit(window.location.href, { action: "replace" })
+      cache.exemptPageFromPreview()
+      visit(window.location.href, { action: "replace" })
     }
   }
 }
diff --git a/src/index.js b/src/index.js
index b76e238..336101a 100644
--- a/src/index.js
+++ b/src/index.js
@@ -8,6 +8,7 @@ import * as Turbo from "./core"
 window.Turbo = Turbo
 Turbo.start()
 
+export { StreamActions } from "./core/streams/stream_actions"
 export * from "./core"
 export * from "./elements"
 export * from "./http"
diff --git a/src/tests/unit/export_tests.js b/src/tests/unit/export_tests.js
index be46c81..c09abcf 100644
--- a/src/tests/unit/export_tests.js
+++ b/src/tests/unit/export_tests.js
@@ -1,5 +1,6 @@
 import { assert } from "@open-wc/testing"
 import * as Turbo from "../../index"
+import { StreamActions } from "../../index"
 
 test("test Turbo interface", () => {
   assert.equal(typeof Turbo.start, "function")
@@ -15,4 +16,9 @@ test("test Turbo interface", () => {
   assert.equal(typeof Turbo.cache, "object")
   assert.equal(typeof Turbo.navigator, "object")
   assert.equal(typeof Turbo.session, "object")
+  assert.equal(typeof Turbo.session, "object")
+})
+
+test("test StreamActions interface", () => {
+  assert.equal(typeof StreamActions, "object")
 })

According to the official documentation, applications that need to extend StreamActions should be importing it directly from the module instead of accessing it through window.Turbo.StreamActions.

I've opened #1023 to separate that change from this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened #1026 to explore an alternative to keeping this logic in StreamActions. It leans more heavily on encapsulating the logic within the Session class. It avoids the circular dependency trap described above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overriding the global fetch presents some risky side-effects. Are there ways we could scope this behavior to be Turbo-only? Maybe some changes to FetchRequest delegate classes' prepareRequest callbacks:

prepareRequest(request) {
if (this.acceptsStreamResponse) {
request.acceptResponseType(StreamMessage.contentType)
}
}

// Fetch request delegate
prepareRequest(request) {
if (!request.isSafe) {
const token = getCookieValue(getMetaContent("csrf-param")) || getMetaContent("csrf-token")
if (token) {
request.headers["X-CSRF-Token"] = token
}
}
if (this.requestAcceptsTurboStreamResponse(request)) {
request.acceptResponseType(StreamMessage.contentType)
}
}

// Fetch request delegate
prepareRequest(request) {
request.headers["Turbo-Frame"] = this.id
if (this.currentNavigationElement?.hasAttribute("data-turbo-stream")) {
request.acceptResponseType(StreamMessage.contentType)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean you could just take the existing version of fetch and export it / pass it around.

export async function fetch(url, options = {}) {
  const originalFetch = window.fetch
  const modifiedHeaders = new Headers(options.headers || {})
  const requestUID = uuid()
  window.Turbo.recentRequests.add(requestUID)
  modifiedHeaders.append("X-Turbo-Request-Id", requestUID)

  return originalFetch(url, {
    ...options,
    headers: modifiedHeaders
  })
}

// Somewhere else
import { fetch } from "./fetch.js"

If React taught us anything, it's dont patch global fetch.

Copy link

@natematykiewicz natematykiewicz Oct 10, 2023

Choose a reason for hiding this comment

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

Also, other libraries such as Sentry patch fetch to add instrumentation. If you are overriding window.fetch, because libraries can be loaded async I'm pretty sure you run the risk of one library removing another library's fetch patches (Sentry removing Turbo's or Turbo removing Sentry's). This seems like a read-modify-write race condition of a non-atomic operation.

Choose a reason for hiding this comment

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

If I'm reading this right, I believe this change means Turbo will break Sentry's instrumentation, as fetch is no longer native code.

https://github.com/getsentry/sentry-javascript/blob/a173fa22ae89380114ac4cc8766d9834704aab86/packages/integrations/src/httpclient.ts#L298-L313

Copy link
Member

Choose a reason for hiding this comment

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

The reason for patching fetch globally instead of keeping the modification isolated in the library is that we want the new behavior to kick in when users invoke fetch manually, to prevent bounced page refreshes in those cases.

Said that, I agree that patching it globally sucks. We could certainly expose a turbo/fetch version you can import in your apps if you want to invoke fetchmanually, and use the patched version internally. Still, I'm concerned about places where you don't direct control over how fetch is called (e.g: if using @rails/request.js).

@natematykiewicz that's a great point and something I hadn't considered at all.

I think that going with the importable patched fetch that @KonnorRogers suggests is the way to go. For invocations you don't have control over, you can still override window.fetch in your app if you want that, but sounds like a safer default. I'll take a stab at that.

On a final note, I wouldn't mind reworking the whole thing if you have a better idea about how to detect/prevent page refresh bounces automatically.

Thanks for the feedback here!

Copy link
Contributor

@seanpdoyle seanpdoyle Oct 17, 2023

Choose a reason for hiding this comment

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

when users invoke fetch manually

If a user is invoking their own fetch to an endpoint that responds in a Turbo-enabled way, would it be reasonable to encourage them to use a package like @rails/request.js, which could either be extended to be Turbo aware?

I'm imagining using some heuristic in FetchRequest.headers). Either that, or could Turbo export some kind of prepareRequest or withTurboHeaders helper method?

Maybe I'm misunderstanding the problem, but I'm having trouble imagining the code that would be making fetch requests.

If a fetch were made without the X-Turbo-Request-Id header outside of a Turbo-handled <form> element submission, how would it communicate the native Response back into the Turbo lifecycle? How would it run the risk of circumventing the refresh debouncing mechanism?

The only way that I can imagine is passing the HTML directly to something like Turbo.visit(path, { responseHTML: await response.text() }), which breaches public API and encapsulation in a way that feels dangerous to begin with.

Having said that, I've explored what it'd take to add a global and internal turbo:before-fetch-request listener in 328c93a.

Choose a reason for hiding this comment

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

We could certainly expose a turbo/fetch version you can import in your apps if you want to invoke fetch manually, and use the patched version internally. Still, I'm concerned about places where you don't direct control over how fetch is called (e.g: if using @rails/request.js)

I think depending on how it is implemented in Turbo; we can always support it in @rails/request.js... @rails/request.js is already aware of Turbo, so making it use a patched version of Fetch from Turbo is also possible.

I am not closely following this PR, but I just wanted to share that, if possible, we can add support to that in @rails/request.js. Obviously, other libraries may have the same problem, but I guess nothing will prevent them from supporting it, either.

Comment on lines 79 to 88
#remoteFrames() {
return document.querySelectorAll("turbo-frame[src]")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a Renderer subclass, it has currentSnapshot and newSnapshot properties:

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

They're likely PageSnapshot instances, which are subclasses of Snapshot.

The Snapshot class already defines some querying properties and methods (like firstAutofocusableElement). Have you considered implementing get remoteFrames() { ... } on the PageSnapshot class, then accessing that in this class from this.newSnapshot.remoteFrames?

@swanson
Copy link

swanson commented Oct 5, 2023

I would be curious if this could support refreshing a frame without src. We have several cases of non-lazy loaded frames that we sometimes need to refresh because it's easier than doing a bunch of fine-grained turbostream responses.

We use a custom action at the moment where we set the frame src to the page URL so that the frame would get swapped but we don't need separate routes for each frame.

StreamActions.hard_refresh_frame = function () {
  this.targetElements.forEach(async (element) => {
    element.src = window.location.href
    await element.loaded
    element.removeAttribute('src')
  })
}

Maybe an optional refreshSrc attribute on frame that doesn't lazy-load that could be used as fallback in there is no src?

@seanpdoyle
Copy link
Contributor

@swanson does that seem related to #1004?

@swanson
Copy link

swanson commented Oct 5, 2023

@swanson does that seem related to #1004?

Yes, that is exactly the situation we have.

We have a frame used as a navigation context but it is not lazy-loaded. We have a settings panel that slides in and changes there could have a big impact on the layout of the frame so we just want to reload the whole thing (while being able to keep the settings panel open, etc)

@swanson
Copy link

swanson commented Oct 5, 2023

This is going to be so nice for things like auto-saving forms where, previously, you would lose your cursor focus if you did a full reload!

@morki
Copy link

morki commented Oct 7, 2023

How detection of refresh works? Does it consider different query params? I think it should me configurable like:

<meta name="turbo-refresh-strategy" content="same-path|same-path-and-query|only-turbo-stream">

The naming is only for discussion.

Am i missing something? Thank you for this awesome enhancement!

@@ -335,7 +335,7 @@ export class Visit {
// Scrolling

performScroll() {
if (!this.scrolled && !this.view.forceReloaded) {
if (!this.scrolled && !this.view.forceReloaded && !this.view.snapshot.shouldPreserveScrollPosition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since shouldPreserveScrollPosition doesn't check if the page is a refresh, this restores scroll even when doing a classic visit instead of a refresh. It was unexpected to me when I tried it out, we should only restore scroll for a refresh, shouldn't we ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is intentional. We have two different settings <meta name="turbo-refresh-method"> and <meta name="turbo-refresh-scroll"> because we have identified some use cases in our apps where we want to do one without the other necessarily.

Copy link
Contributor

@seanpdoyle seanpdoyle Oct 17, 2023

Choose a reason for hiding this comment

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

@afcapel is scroll preservation something that's specific to Refresh Morph?

Could scroll preservation be implemented separately, with a more generic <meta> name? It could close #37 directly, without requiring opting-into Morphing.

Maybe something like <meta name="turbo-scroll" value="preserve">? The scroll-related portion of the diff feels small by comparison.

I'd be happy to open a PR to add that behavior separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened #1040 to add <meta name="turbo-scroll" content="preserve"> support. That PR is against main.

I think there's a lot of value in managing scroll preservation outside of a morphing context, so generalizing to turbo-scroll feels like a win.

Copy link
Member

Choose a reason for hiding this comment

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

@seanpdoyle I'd like to keep the scroll configuration under the same "page refresh" umbrella. Our vision here is not adding more flexibility but aiming for seamless smoother page updates (as in re-render the current page again). We believe it's in that scenario where keeping the scroll makes sense, to the point where "preserve" should be the future default for such cases being "reset" the exception. In fact, the only reason we added the option is that we found a case of "page refresh" where you wanted to opt out of scroll preservation due to design considerations.

Just like morphing, scroll preservation for regular navigation could make sense in other scenarios, but we believe those will be quite rare and are out of the scope of what we're addressing here. I'd like to keep both "scroll" and "morph" tied to "page refreshes" for now, as I think it makes for a more cohesive development story.

@Intrepidd
Copy link
Contributor

How detection of refresh works? Does it consider different query params? I think it should me configurable like:

<meta name="turbo-refresh-strategy" content="same-path|same-path-and-query|only-turbo-stream">

The naming is only for discussion.

Am i missing something? Thank you for this awesome enhancement!

AFAICS different query params won't trigger a refresh but a full visit :

https://github.com/hotwired/turbo/pull/1019/files#diff-f1972083be28125eb9c224f3769b8224ed588745e49847fd10213bfbb9ccc5fcR62-R64

I agree it would be nice to be able to perform a refresh when query params differ though. An example use case would be to redirect to the same page with a ?success=true query param that would trigger a success animation after a form submission.

this.#morphElements(this.currentElement, this.newElement)
this.#reloadRemoteFrames()

this.#dispatchEvent("turbo:morph", { currentElement: this.currentElement, newElement: this.newElement })
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the significance of introducing new turbo:morph and turbo:before-frame-morph events as an alternative to extending existing events like turbo:render, turbo:before-frame-render, and turbo:frame-render? Could we change the shape of those events' details to incorporate information about the refresh-method setting being used?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgemanrubia this PR is really shaping up!

I think idea of introducing new events instead of extending existing events is the only remaining concept I'm curious to learn more about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seanpdoyle I think having different event types make it easier to target only the actions you're interested on.

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Oct 8, 2023

What changes are necessary to incorporate idiomorph into the FrameRenderer.loadFrameElement method?

Unless I'm missing something, I don't see changes to support a <turbo-frame refresh="morph"> element navigated by a <a> or <form> being modified by idiomorph.

I've opened #1029 to explore that.

@afcapel
Copy link
Collaborator Author

afcapel commented Oct 8, 2023

Prior to this behavior, the presence of [data-turbo-permanent] required the presence of an [id] attribute. Does this change mean that [data-turbo-permanent] on its own has new morph-only semantics, or will elements still require [id] attributes to be skipped during morphing?

That's correct. Initially we had another name for the data attribute - data-turbo-refresh-preserve -, but we decided to reuse data-turbo-permanent. Semantically it does the same in both use cases.

Copy link
Contributor

@seanpdoyle seanpdoyle Oct 8, 2023

Choose a reason for hiding this comment

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

@afcapel I've opened #1025 (cut off this branch) to explore an alternative to re-writing the window.fetch

seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Oct 9, 2023
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Oct 9, 2023
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
@palkan
Copy link

palkan commented Oct 9, 2023

Hey @afcapel @seanpdoyle!

I'd like to pay your attention to this issue: #951

I haven't tried this branch yet but from what I see in the changes, it seems that the current implementation may be prone to the problem mentioned in the issue (tl;dr snapshot referencing new morphed state of the document).

@palkan
Copy link

palkan commented Oct 9, 2023

@afcapel Could you please share an example of the problem that <turbo-frame refresh="reload"> solves? From the PR description:

For example, if you have loaded new pages of data. You don’t want to lose that data when a page refresh happens. If the user can act on that data, you want to see it refreshed too.

So, we want to reload frames after the page has been morphed. But isn't this already happening automatically for remote frames? We receive the <turbo-frame> HTML without the complete attribute from the server, so morphing would remove it from the current frame node thus triggering the reload (see

this.delegate.completeChanged()
).

A one exclusion I see is frames within permanent elements. With the proposed solution, they would be reloaded disregarding being within permanent containers—which seems confusing to me. And this is not how the current replace strategy works.

/cc @seanpdoyle since it's related to #1029

@jorgemanrubia
Copy link
Member

@palkan

I haven't tried this branch yet but from what I see in the changes, it seems that the current implementation may be prone to the problem mentioned in the issue (tl;dr snapshot referencing new morphed state of the document).

Yes, I found this problem too during our explorations. The reason is that Turbo caches snapshots in a deferred way, both as a performance optimization and to let stimulus controllers an opportunity to disconnect and cache whatever they do there. Essentially, Turbo is relying on a different <body> for the snapshot system to work.

The PR doesn't present that problem because we don't use morphing for regular navigations across different pages, just for cases where you re-render the current page you are in, typically after a redirect. Because of the nature of these page refreshes, caching doesn't really make sense (just like you don't want to cache a stream action response that makes a partial update).

I wonder if the fix by @afcapel from #1014 would fix #951 too

@jorgemanrubia
Copy link
Member

jorgemanrubia commented Oct 10, 2023

@palkan

So, we want to reload frames after the page has been morphed. But isn't this already happening automatically for remote frames? We receive the HTML without the complete attribute from the server, so morphing would remove it from the current frame node thus triggering the reload (see

We don't want those frames to reload normally because that would trigger a replacement of the content so they would lose screen state. We want them to keep screen state and, to all effects, remain untouched if the remote content didn't change.

An example is pagination:

  • You paginate new content in a turbo frame.
  • You delete one of the paginated elements, that triggers a page refresh via a "redirect back" response.
  • You just want to see the deleted element disappear smoothly, with the scroll kept and such.

Checking the code I'm noticing that during the upstreaming we lost something we had in the private gem we created for this, which was excluding turbo-frames flagged with refresh=reload from morphing itself. I think we should do that.

A one exclusion I see is frames within permanent elements. With the proposed solution, they would be reloaded disregarding being within permanent containers—which seems confusing to me. And this is not how the current replace strategy works.

This is a good point. We should exclude those.

Our availability is a bit choppy this week as we are in a company meetup. Thanks for all the feedback everyone.

Update: addressed here f6f62e8.

@palkan
Copy link

palkan commented Oct 10, 2023

@jorgemanrubia Thanks for the quick response!

The PR doesn't present that problem because we don't use morphing for regular navigations across different pages

I see, this is what the isPageRefresh guard for. My first impression was that turbo-refresh-method meta affects everything (even though the "refresh" is used in the name). That's just a matter of documentation.

excluding turbo-frames flagged with refresh=reload from morphing itself

Oh, that explains everything 🙂

@jorgemanrubia
Copy link
Member

@seanpdoyle

Prior to this behavior, the presence of [data-turbo-permanent] required the presence of an [id] attribute. Does this change mean that [data-turbo-permanent] on its own has new morph-only semantics, or will elements still require [id] attributes to be skipped during morphing?

When using morphing, you can use [data-turbo-permanent] without a companion id.

jorgemanrubia and others added 2 commits November 9, 2023 22:15
…errors

Turbo will render 422 responses to allow handling form errors. A common scenario
in Rails is to render those setting the satus like:

```
render "edit", status: :unprocessable_entity
```

This change will consider such operations a "page refresh" and will also consider
the scroll directive.
Respect morphing and scroll preservation settings when handling form errors
There are some cases when we don't want to reload a remote turbo frame on
a page refresh.

This may be because Turbo has added a src attribute to the turbo frame
element, but we don't want to reload the frame from that URL.

Form example, when a form inside a turbo frame is submitted, Turbo adds
a src attribute to the form element. In those cases we don't want to
reload the Turbo frame from the src URL. The src attribute points to the
form submission URL, which may not be loadable with a GET request.

Same thing can happen when a link inside a turbo frame is clicked. Turbo
adds a src attribute to the frame element, but we don't want to reload the
frame from that URL.

If the src attribute of a turbo frame changes, this signals that the server
wants to render something different that what's currently on the DOM, and
Turbo should respect that.

This also matches the progressive enhancement behavior philosophy of Turbo.
The behaviour results in the Turbo frame that the server sends, which
is what would happen anyway if there was no morphing involved, or on a first
page load.
* origin/main:
  Pass session instance to cache constructor (#1063)
@afcapel afcapel merged commit a178184 into main Nov 14, 2023
2 checks passed
@afcapel afcapel deleted the morph-refreshes branch November 14, 2023 15:06
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 14, 2023
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
@jorgemanrubia
Copy link
Member

A heads up that we still need to fix an issue with mobile apps before releasing a beta here. Right now, when you redirect back in a native app, you always get a blank flash because it always uses a new page view (whether you use morphing or not). We'll fix that in their corresponding adapters.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 16, 2023
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 16, 2023
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
chrisgrande added a commit to chrisgrande/django-turbo-response that referenced this pull request Nov 27, 2023
For support for morphing coming in Turbo 8, hotwired/turbo#1019

Duplicates the Rails turbo_refreshes_with template helper.
@jon-sully
Copy link

jon-sully commented Dec 26, 2023

The system includes a mechanism to ignore refresh actions that originated [from a] web request that originated in the same browser session.

This implementation is very clever. So well done! 🙌 "Don't trigger a Page Refresh (broadcasted) when you just did a Page Refresh (since the controller redirected you to the same page)". So nice ✨

chrisgrande added a commit to chrisgrande/django-turbo-response that referenced this pull request Jan 12, 2024
For support for morphing coming in Turbo 8, hotwired/turbo#1019

Duplicates the Rails turbo_refreshes_with template helper.
chrisgrande added a commit to chrisgrande/django-turbo-response that referenced this pull request Jan 12, 2024
For support for morphing coming in Turbo 8, hotwired/turbo#1019

Duplicates the Rails turbo_refreshes_with template helper.
chrisgrande added a commit to chrisgrande/django-turbo-response that referenced this pull request Jan 30, 2024
For support for morphing coming in Turbo 8, hotwired/turbo#1019

Duplicates the Rails turbo_refreshes_with template helper.

Updated docs and helper
chrisgrande added a commit to chrisgrande/django-turbo-response that referenced this pull request Jan 30, 2024
For support for morphing coming in Turbo 8, hotwired/turbo#1019

Duplicates the Rails turbo_refreshes_with template helper.

Updated docs and helper
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Mar 29, 2024
The changes proposed in [hotwired#1019][] require dispatching the new
`Renderer.renderMethod` property as part of the `turbo:render` event.
Similarly, there's an opportunity to include that information in the
`turbo:before-render`, `turbo:before-frame-render`, and
`turbo:frame-render` events.

To simplify those chains of object access, this commit changes the
`View` class delegate's `allowsImmediateRender` and
`viewRenderedSnapshot` methods to accept the `Renderer` instance,
instead of individual properties.

With access to the instance, the delegate's can read properties like
`isPreview` along with the `element` (transitively through the
`newSnapshot` property).

In order to dispatch the `turbo:frame-render` event closer to the moment
in time that the view renders, this commit removes the
`Session.frameRendered` callback, and replaces it with a
`dispatch("turbo:frame-render")` call inside the
`FrameController.viewRenderedSnapshot(renderer)` delegate method.

In order to do so, this commit must work around the fact that
`turbo:frame-render` events include the `FetchResponse` instance
involved in the render. Since that instance isn't available at render
time, this commit wraps the `await this.view.render(renderer)` in a
utility function that injects the `FetchResponse` instance into the
Custom event's `event.detail` object immediately after it's initially
dispatched.

Ideally, this work around will only be temporary, since the
`turbo:frame-load` event also includes `event.detail.fetchResponse`.
There's an opportunity to deprecate that property in
`turbo:frame-render` events in the future.

[hotwired#1019]: hotwired#1019
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.