Replace global fetch patch with Turbo-specific behavior #1025

seanpdoyle · 2023-10-08T21:42:16Z

As an alternative to patching fetch globally, this commit introduces a
new FetchRequestObserver class, and makes the Session instance a
delegate.

As prep-work, this commit also moves the recentRequests out of the
global @hotwired/turbo module an into an instance property on
Session.

The FetchRequestObserver instance attaches an event listener for
turbo:before-fetch-request (in this case, on the <html> element).
Instead of generating the request ID in the delegate, this commit also
extends the FetchRequest class to generate the uuid() and set it to
the new FetchRequest.id property.

The Session.willFetch delegate method will be invoked before each
request, and will read the X-Turbo-Request-Id header and write it to
the instance's recentRequests set.

This commit 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. Co-Authored-By: Jorge Manrubia <jorge@hey.com>

jorgemanrubia

Hey @seanpdoyle,

Thanks for working on this. What I had in mind here was two things:

Define an internal fetch that decorates window.fetch with the new behavior, without overriding it. Turbo will internally use this internal fetch. And applications will be able to import it too:

import { fetch } from "@hotwired/turbo-rails"

Expose it also via Turbo.window, so that libraries that doesn't build against Turbo – such as requestjs-rails – can still do something like:

const fetch = window.Turbo?.fetch || window.fetch

(1) will be the preferred form of using the new fetch from apps. (2) is intended a fallback mechanism.

I think your refactor is a nice improvement over the original PR, but it's kind of misaligned with (1). I think we need a custom fetch to use internally and export to clients, instead of making the current requests observable.

What do you think?

seanpdoyle · 2023-10-23T14:45:59Z

@jorgemanrubia I think I am misunderstanding the use case for applications to utilize a Turbo-specific fetch (1. above).

Why would an application need to make a request with an X-Turbo-Request-Id header? Even if they were able to import and utilize the Turbo-extended fetch, how would they application pass the HTTP response back to Turbo in a way that is compatible with Turbo Drive?

If they're submitting a request outside of Turbo.visit or HTMLFormElement.requestSubmit(), what risks would an internal fetch guard against?

As an alternative to patching `fetch` globally, this commit introduces a new `FetchRequestObserver` class, and makes the `Session` instance a delegate. As prep-work, this commit also moves the `recentRequests` out of the global `@hotwired/turbo` module an into an instance property on `Session`. The `FetchRequestObserver` instance attaches an event listener for `turbo:before-fetch-request` (in this case, on the `<html>` element). Instead of generating the request ID in the delegate, this commit also extends the `FetchRequest` class to generate the `uuid()` and set it to the new `FetchRequest.id` property. The `Session.willFetch` delegate method will be invoked before each request, and will read the `X-Turbo-Request-Id` header and write it to the instance's `recentRequests` set.

jorgemanrubia · 2023-10-23T16:14:00Z

@seanpdoyle because that way the application makes sure that the request id is registered by Turbo internally. So that, if a "refresh" stream action is broadcasted, and if it carries information about the "request id that originated it", Turbo can determine if it was self-originated and, in that case, ignore it.

Practical case: you have a Drag & Drop system that relies on window.fetch from a stimulus controller to update stuff. You have configured the server the involved models to broadcast "page refreshes" on changes. If you drop an element, you don't want to get a broadcasted "page refresh" from the same request that you originated.

In the general case, you are interested in all the broadcasted "page refreshes" except the ones that you originate. The vast majority of your page refreshes will come via Turbo actions, but not necessarily all of them, since using window.fetch is still relatively common in certain circumstances. That's the problem we're trying to solve here.

seanpdoyle · 2023-10-23T16:49:44Z

you don't want to get a broadcasted "page refresh" from the same request that you originated.

Ahhh, so the header is less about fetch's integration with HTTP, and more about guarding against background broadcasts over Web Sockets. That's something that was missing from my perspective on the problem.

With that in mind, is there a difference in outcomes between Turbo support for something like:

import { fetch } from "@hotwired/turbo"

fetch("/my-path", { ... })

versus something like:

import { withTurboHeaders } from "@hotwired/turbo"

window.fetch("/my-path", withTurboHeaders({ ... }))

// or maybe
import { FetchRequest } from "@hotwired/turbo"

window.fetch("/my-path", FetchRequest.prepare({ ... }))

Both scenarios require awareness that Turbo does something special with handling fetch.

jorgemanrubia · 2023-10-23T16:59:21Z

@seanpdoyle not in outcomes but in ergonomics from the caller side, imo. Also, the first form's encapsulation is higher, hiding away the fact that internally it's manipulating the request headers.

adrienpoly · 2023-10-23T19:49:39Z

Hello

Thanks for all the work going on, it looks really promising.

I wanted to share a few thoughts:

import { fetch } from "@hotwired/turbo"

I personally think having a custom fetch option coming from Turbo will add lots of confusion at the end.

I have been really enjoying the @rails/request.js library so far. The API is nice with lots of small goodies. Out of all the fetch libraries that I have been using, I think it has the best API.

in #1019 (comment) @marcelolx proposed some help to make Rails/request.js Turbo compatible. My favorite solution would be to be able to use this package for all my fetch-related actions.

Therefore project NOT using @rails/request.js, I would second @seanpdoyle suggestion for something like that.

import { withTurboHeaders } from "@hotwired/turbo"

window.fetch("/my-path", withTurboHeaders({ ... }))

This makes it clear that it is not the preferred option. The preferred option would be documented by using request.js.

And for Rails/request.js let's make sure this works out of the box from the standard fetch library provided by Rails.

jorgemanrubia · 2023-10-23T21:06:41Z

Hey @adrienpoly, +1 to make @rails/request.js the preferred option for manual fetch operations. We can make sure we document that clearly in the docs also, and we can improve the compatibility with Turbo in the process.

I don't agree with making the not-preferred option less ergonomic though.

adrienpoly · 2023-10-23T21:44:54Z

The issue I see with exposing this new custom fetch from Turbo as a public API is that it will make it harder down the road to unify Turbo and Request.js. To me, it should remain a private API inside Turbo or come from request.js

When I saw Request.js for the first time, I saw it as an extraction of Turbo internal fetch and thought the plan was to ultimately replace Turbo internal fetch with request.js. I am sure there are quite a few differences in both implementations but last time I looked there was also a lot of duplication in both code bases.

IMHO, the most comprehensive API would be

import { get, put, post, delete  } from "@hotwired/turbo"

and those get, put, post, and delete would be coming from request.js

seanpdoyle · 2023-10-27T16:51:01Z

Closing due to comments above: #1025 (review).

afcapel and others added 3 commits October 4, 2023 22:37

Introduce refresh attribute for frame element

3f45254

Use dispatch util function in MorphRenderer

a653d12

seanpdoyle mentioned this pull request Oct 8, 2023

Page refreshes #1019

Merged

2 tasks

jorgemanrubia force-pushed the morph-refreshes branch 3 times, most recently from 7ac1faf to d1935bd Compare October 23, 2023 09:24

jorgemanrubia reviewed Oct 23, 2023

View reviewed changes

seanpdoyle force-pushed the morph-refreshes-without-patching-global-fetch branch from 328c93a to 6c318e2 Compare October 23, 2023 15:13

jorgemanrubia force-pushed the morph-refreshes branch 2 times, most recently from 9125652 to 00b3015 Compare October 27, 2023 07:13

seanpdoyle closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace global fetch patch with Turbo-specific behavior #1025

Replace global fetch patch with Turbo-specific behavior #1025

seanpdoyle commented Oct 8, 2023

jorgemanrubia left a comment

seanpdoyle commented Oct 23, 2023

jorgemanrubia commented Oct 23, 2023

seanpdoyle commented Oct 23, 2023 •

edited

Loading

jorgemanrubia commented Oct 23, 2023

adrienpoly commented Oct 23, 2023

jorgemanrubia commented Oct 23, 2023

adrienpoly commented Oct 23, 2023

seanpdoyle commented Oct 27, 2023

Replace global fetch patch with Turbo-specific behavior #1025

Replace global fetch patch with Turbo-specific behavior #1025

Conversation

seanpdoyle commented Oct 8, 2023

jorgemanrubia left a comment

Choose a reason for hiding this comment

seanpdoyle commented Oct 23, 2023

jorgemanrubia commented Oct 23, 2023

seanpdoyle commented Oct 23, 2023 • edited Loading

jorgemanrubia commented Oct 23, 2023

adrienpoly commented Oct 23, 2023

jorgemanrubia commented Oct 23, 2023

adrienpoly commented Oct 23, 2023

seanpdoyle commented Oct 27, 2023

seanpdoyle commented Oct 23, 2023 •

edited

Loading