hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.69k stars 423 forks source link

Turbo Stream: use a proper media type instead of an attribute #24

Closed dunglas closed 3 years ago

dunglas commented 3 years ago

Hi, thanks for this great library!

Currently, the media type expected for turbo streams fragments is text/html; turbo-stream. This creates some problems:

  1. This attribute is not registered (https://www.iana.org/assignments/media-types/text/html) and is ignored by most parsers.
  2. Consequently, it can be very difficult to detect if a request is intended to be used by Turbo or not. For instance, it's currently impossible to do it with the content negotiation tools built in Symfony.

Instead of an attribute, a vendor media type such as application/vnd.hotwire.turbo.stream could be used, as suggested by RFC 6838

thetutlage commented 3 years ago

100% on this. The content negotiation parser of AdonisJS will fail too. I have seen most of the projects like JSONAPI also used a custom media type

panda-madness commented 3 years ago

You could also append a custom header to a turbo request, and expect a custom header in the response. Something like X-Turbo-Request, X-Turbo-Response

delitescere commented 3 years ago

@dunglas Parameters are optional. While the only one mentioned in the text/html spec is charset, parsers should ignore any other parameters as if they weren't present, see https://tools.ietf.org/html/rfc2046#section-1:

implementations must also ignore any parameters whose names they do not recognize.

The media type should remain text/html (parameter or no), as that is what the data stream is. Perhaps raise an issue with the parsers that aren't compliant?

@panda-madness While a custom response header may be the best indicator, the X- prefix for custom HTTP headers is no longer a practice (see https://tools.ietf.org/html/rfc6648). Hotwire-Turbo-Stream: true may be a better alternative?

dunglas commented 3 years ago

Parameters are optional. While the only one mentioned in the text/html spec is charset, parsers should ignore any other parameters as if they weren't present

@delitescere that's exactly the problem! As this parameter isn't registered for the HTML media type, parsers ignore it and it becomes complicated to detect if Turbo Stream flavored HTML or regular HTML must be served.

Using a vendored media type fixes that (and it's not uncommon for a media type to extend another one, for instance OpenXML files have their own media types but are basically zipped XML files). Using the standard HTML media type and a custom header in addition (as done for Turbo Frames) does the trick too.

dhh commented 3 years ago

Current thinking is we'll go with something ala RSS/ATOM and use application/turbo-stream+html. But yes, needs to change.

dunglas commented 3 years ago

@dhh I thought about that but unfortunately it's also invalid. xml (used by Atom) and json (used by JSON-LD) for instance are valid registered suffixes, but html isn't one.

delitescere commented 3 years ago

A great idea but until html is registered as a suffix (and then parser implementations catch up) that’ll have similar problems.

@dhh Sadly text/turbo-stream+html won't be read as HTML by browsers until it's registered (they currently read it as if it's plain text). But then that goes for text/turbo-stream+xml too. Worse, application/turbo-stream+xml is treated as a downloaded file. So, if you wanted the content of these responses to be handled in a manner allowing progressive enhancement, the Content-Type is not the way to go. If you don't expect it to be (i.e. the body is only useful for the turbo-streams JavaScript component to handle) then it doesn't matter and text/turbo-stream+html +xml or a full vnd would work equally well.

I'm not sure why it can't just be plain text/html though? Is it to optimize the turbo-stream JavaScript checking it if has to bother parsing the response or not? That is, it only parses and looks for <turbo-stream> elements if the right Content-Type header is present?

If that's the case, any other header (e.g. Hotwire-Turbo-Stream: true) would do, would also allow normal text/html processing of the body, and doesn't get in the way of anything else.

dhh commented 3 years ago

Ah. Maybe we will just go with xml then. Or vnd. Not interested in using a header.

delitescere commented 3 years ago

@dhh nonetheless, Content-Type is a header. The suggestion was just to use a different (custom) header, like Hotwire-Turbo-Stream: true and leave Content-Type: text/html undisturbed.

I'm interested in the rationale that makes a custom HTTP header undesirable?

dhh commented 3 years ago

Using a header would mean we can't use Rails' content negotiation by default. You'll see all the examples everywhere use:

respond_to do |format|
  format.turbo_stream
  format.html
  format.json
end

This relies on content negotiation, which uses the Accept header, which should have a matching content type for the return value. No interest in special casing that. So scope here is to find a workable content-type we can use.

delitescere commented 3 years ago

@dhh My understanding so far:

  1. Turbo generates the following request header Accept: text/html; turbo-stream, text/html, application/xhtml+xml for all requests (whether a turbo-stream response is expected or not), and
  2. this request header is indeed useful for content negotiation by the server framework, but even so
  3. the Content-Type response header returned for both your format.turbo_stream and format.html handlers could still be the unadorned text/html (because the presence of <turbo-stream> tags doesn't make the response not HTML, even though such elements should only be present when the turbo-stream indicator exists in the Accept request header)

I'm still puzzled as to why there's nothing in the originating document that indicates how the expected response will be handled as a turbo-stream, which seems starkly different to a turbo-frame.

Do you think the following example would make things easier all round? The Accept request header only needs to be augmented for the specific requests expecting a turbo-stream response (in this case the form submission), and the response doesn't need any response header, special content type, or other indication of turbo-streamness (other than the turbo-stream tags in the response body) because the JS handler already knows at that point that a turbo-stream is expected. And, if turbo's JS isn't loaded, shit still works as normal (assuming the server does use a format.html content handler and thus doesn't return content with a <turbo-stream> element)

<turbo-frame id="greeting_frame">
    <a href="/greeting/?person=Josh">I expect this to be replaced in-situ because it's a turbo-frame</a>
</turbo-frame>

<form action="/pinger" method="post" data-turbo-stream>
    <button type="submit">Ping 1.1.1.1</button>
</form>
<div>
    <p>Ping times</p>
    <ol id="pings">I expect this to be replaced, appended, deleted, etc in-situ by the turbo-stream request indicated in the document (in the form above)</ol>
</div>
dhh commented 3 years ago

@delitescere For content negotiation to work with proper fallback in Rails (and presumably any other framework that does content negotiation based on the content type), we have to have a dedicated format that is only rendered when its matched with the correct order in the Accept header. That's what the example I posted above does. That would not work if there was no Accept: text/html; turbo-stream, text/html header.

So we're going to use a content type to make this work, but we'll find a content type that works broadly and is in compliance πŸ‘

delitescere commented 3 years ago

Totally agree on the request Accept values. Just pointing out that the response Content-Type can remain text/html whether or not it is a response to a turbo-stream request and contains turbo-stream tags (providing the JS that is parsing it has the context to look for turbo-stream tags)

dhh commented 3 years ago

You don't know in advance what kind of response you're going to get. A form submission could result in a redirect, in an inline rendering of an error page, or in a turbo stream update. So need the content type to tell us what's what.

jefrog1844 commented 3 years ago

Hi, thanks for this great library!

Currently, the media type expected for turbo streams fragments is text/html; turbo-stream. This creates some problems:

1. This attribute is not registered (https://www.iana.org/assignments/media-types/text/html) and is ignored by most parsers.

2. Consequently, it can be very difficult to detect if a request is intended to be used by Turbo or not. For instance, it's currently impossible to do it with the [content negotiation tools built in Symfony](https://symfony.com/blog/new-in-symfony-4-2-acceptable-request-formats).

Instead of an attribute, a vendor media type such as application/vnd.hotwire.turbo.stream could be used, as suggested by RFC 6838

Agreed this looks like a very promising library that I'm excited about integrating into my application.

However, I'm experiencing the same issue.

Setting up Turbo to use with Quarkus(rest-easy)/Qute templates. I'm not a mime type expert, but rest-easy returns a 400 error and I believe it is because the mime type has a semi-colon between text/html and turbo-stream in the content-type as shown below which is part of the stream_observers.ts file

prepareFetchRequest = ((event: CustomEvent) => { const fetchOptions: FetchRequestOptions = event.detail?.fetchOptions if (fetchOptions) { const { headers } = fetchOptions headers.Accept = [ "text/html; turbo-stream", headers.Accept ].join(", ") } })

When I change the semi colon to a comma, rest-easy then returns a 500 error.

Once turbo-stream portion is removed from the header everything works as expected.

dhh commented 3 years ago

We'll be changing the mime type in beta 3 to application/vnd.turbo.stream.html πŸ‘

jefrog1844 commented 3 years ago

Thank you very much. This looks to be an awesome product that will integrate nicely with Quarkus/Qute.

Jeff

On Tue, Jan 5, 2021 at 11:34 AM David Heinemeier Hansson < notifications@github.com> wrote:

We'll be changing the mime type in beta 3 to application/vnd.turbo.stream.html πŸ‘

β€” You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hotwired/turbo/issues/24#issuecomment-754783881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM6FHO6DDKJCREOZKC3DM33SYNERXANCNFSM4VG3NXRQ .

dunglas commented 3 years ago

@dhh I can work on a PR if you want.

dhh commented 3 years ago

Please do!

santib commented 3 years ago

Sorry if it's a silly question, but something that confused me right from the beginning was that every Turbo request had text/html; turbo-stream format (even if they were triggered by Turbo Drive or Turbo Frame). Now that we are renaming the media type, wouldn't it make sense to change it to application/vnd.turbo.html(or text/vnd.turbo.html) instead of application/vnd.turbo.stream.html(or text/vnd.turbo.stream.html)

jefrog1844 commented 3 years ago

I was wondering the same thing as I observed the issue when using all turbo frame and not turbo stream. Seems like the mime type should be used only for the turbo stream.

On Jan 9, 2021, at 10:50 AM, Santiago Bartesaghi notifications@github.com wrote:

Sorry if it's a silly question, but something that confused me right from the beginning was that every Turbo request had text/html; turbo-stream format (even if they were triggered by Turbo Drive or Turbo Frame). Now that we are renaming the media type, wouldn't it makes sense to change it to application/vnd.turbo.html instead of application/vnd.turbo.stream.html

β€” You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hotwired/turbo/issues/24#issuecomment-757333961, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM6FHO6FZYGD2SLDQFH7S4DSZCCNRANCNFSM4VG3NXRQ.