Skip to content

[Turbo] Simplify stream negotiation #217

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

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Jan 11, 2022

Q A
Bug fix? no
New feature? yes
Tickets n/a
License MIT

This patch replaces the ad-hoc content negotiation system for Turbo Streams used by the bundle. HttpFoundation's native content negotiation features are now used instead. This greatly simplifies the code, which is security-sensitive, and improves DX (content negotiation is now consistent with how it is handled in the rest of the framework).

Replaces #214. Needs #215 and symfony/symfony#44980.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I really like that. Here are some comments.

@chalasr chalasr force-pushed the feat/simplify-stream-negotitation branch from 1636d22 to b6de1e3 Compare January 12, 2022 18:02
@chalasr
Copy link
Member

chalasr commented Jan 12, 2022

Thanks Kévin.

@chalasr chalasr merged commit ae21db7 into symfony:2.x Jan 12, 2022
Copy link
Contributor

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

Great work. Thank you 👍

$response = $this->render('task/success.html.twig', ['task' => $task]);
}

return $response->setVary('Accept');
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

public const STREAM_FORMAT = 'turbo_stream';
public const STREAM_MEDIA_TYPE = 'text/vnd.turbo-stream.html';

public function boot(): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know about this method. Nice to know something like this exists.

@dunglas dunglas deleted the feat/simplify-stream-negotitation branch January 18, 2022 00:38
@alexander-schranz
Copy link
Contributor

@weaverryan anything blocking here for a release?

@nicolas-grekas
Copy link
Member

/cc @florentdestremau this relates to your talk at the meetup yesterday: TurboStreamResponse is gone, see updated doc in the PR.

@florentdestremau
Copy link
Contributor

Indeed, but you still need to specify the request format ? I feel like you still need to "think" about changing something in your code when you switch from a turbo-frame to a turbo-stream. Or did I misread ?

@nicolas-grekas
Copy link
Member

The call to $request->setRequestFormat(TurboBundle::STREAM_FORMAT); will be propagated to the response so that the right content-type is returned.

@florentdestremau
Copy link
Contributor

I'm not sure I understand the improvement here 🤔 . If you can either return a turbo-frame (e.g. Form Invalid → 400, or Form display → 200) or a turbo-stream (e.g. Form Submitted Successfully → 302 + 200 or 200) then you can't always set the request format, so you don't really "gain" code length vs extending the TurboStreamRequest object in your $this->render() from what I see. I can imagine it helps when you are returning in 100% of the cases a turbo-stream to avoid repeating?
What I would like is for Symfony to auto-detect when a response is a turbo-stream to update the headers. Is this what the PR is doing ?

@florentdestremau
Copy link
Contributor

Other point: I'm thankful for you @nicolas-grekas pointing this PR out, because it causes a BC-break in our code: the object TurboStreamResponse is removed in a minor version so I feel like it should have been deprecated instead, no ? For now we downgraded to 2.0.1 until we take the time to update our codebase with 2.1

@alexander-schranz
Copy link
Contributor

alexander-schranz commented May 11, 2022

The object TurboStreamResponse is removed in a minor version so I feel like it should have been deprecated instead, no ? For now we downgraded to 2.0.1 until we take the time to update our codebase with 2.1

The Symfony/UX component is still marked as experimental and so can contain bc breaks in minor release. When using a experimental components you should go with e.g.: 2.0.* in your composer.json to avoid accidentally update to a version you are maybe not supporting.

@nicolas-grekas
Copy link
Member

At least from an internal pov, this removes a listener, which is a net win. About userland, that's similar yes, although more straightforward now I believe.

@florentdestremau
Copy link
Contributor

@alexander-schranz Yeah I kinda suspected that, but I can't find an explicit message mentioning this, would be interesting to make sur it is 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants