Introduce Symfony UX Turbo #61

tgalopin · 2021-02-19T20:08:09Z

Q	A
Bug fix?	no
New feature?	yes
Tickets	-
License	MIT

This PR introduces a new package to Symfony UX: UX Turbo, integrating the Hotwire Turbo library with Symfony. It is based on an initial prototype made by Kevin Dunglas (@dunglas) as a Mercure project.

Hotwire is an aggregation of several technologies developed by the Ruby on Rails ecosystem and the Bascamp company to build interactive applications with as less JavaScript code as possible. Stimulus, as the foundation of Symfony UX, is already well integrated into Symfony. This PR goes to the next step: integrating Turbo.

You can learn more about the Hotwire intiative on https://twitter.com/dhh/status/1341420143239450624. You can also read the README in this PR to better understand the usage in a Symfony application.

TLDR: Turbo is a generic way to build reactive applications (inline form edition, DOM changes without reload, real-time display of chat messages, etc.) without the need to develop any JavaScript (or at least as little as possible). This has two big advantages: it reduces dramatically the time necessary to build a reactive app and it eliminates in many places the need for a client-side state which could be a source of issues.

This PR is loosely based on the equivalent gem in Ruby that integrates Turbo in RoR. It has however a few key differences:

it leverages Symfony Mercure by default for Streams instead of Websockets (as PHP isn't a long running process)
it build upon other tools in Symfony like the stimulus_controller helper introduced a few days ago in Webpack Encore

src/Turbo/Twig/FrameTwigExtension.php

src/Turbo/Twig/StreamTwigExtension.php

src/Turbo/composer.json

nicolas-grekas · 2021-02-20T10:29:37Z

src/Turbo/DependencyInjection/TurboExtension.php

+/**
+ * @author Titouan Galopin <[email protected]>
+ *
+ * @internal


I don't think we need all those @internal annotations, do we?
What about @experimental also? Some have it, not all.

I may be a bit conservative regarding these annotations indeed :) . Other UX packages have these annotations as well, not sure what our policy should be on them.

I'm often adding @internal when the class should not be used in both framework and standalone usage. E.g. when someone is using this package outside the Symfony framework, they probably end up needing to initialize MercureStreamAdapter. So I wouldn't add @internal there.

As for @experimental, I'm often not adding it to @internal classes (as there is no BC promise in those anyway). Not sure if these are "Symfony policies" or only my personal policy regarding them :)

I agree with @wouterj here, we should remove @internal imho.
Maybe we can make the class final also?

tgalopin · 2021-02-20T14:32:55Z

@Kocal I'm not familiar with eslint, any idea why it fails on this package but not others? I may be missing required config but I don't see it

Kocal · 2021-02-20T14:51:41Z

Hi @tgalopin, I think it's because of this https://github.com/tgalopin/ux/blob/ux-turbo/package.json#L34

I don't think this glob pattern will match src/Turbo/Resources/assets/test/adapters/mercure-controller.test.js since mercure-controller.test.js is located under adapters folder.

Can you try using "src/*/Resources/assets/test/**/*.js" instead?

tgalopin · 2021-02-20T14:55:30Z

Aah I was looking in the package, I didn't think of this file. Let me try, thanks!

nicolas-grekas · 2021-02-20T20:51:45Z

Missing LICENSE file, see #62 :)

.github/workflows/test.yaml

src/Turbo/DependencyInjection/Configuration.php

stof · 2021-02-22T11:15:36Z

src/Turbo/README.md

+    <a href="/messages">Back</a>
+
+<!-- End the frame -->
+{{ turbo_frame_end() }}


shouldn't this be a tag rather than a couple of _start and _end functions ?

AFAIK, tags do things and functions display things, thus this notation. I do agree tags would probably look nicer though. Not fixed about this.

Tags would help in IDE autocompletion (same as for 'apply' and 'endapply'), so it's less error-prone I guess.

Do we have any examples of tags that output something?
forms use functions for the same reason.

One that is very close to this use-case is NelmioSecurityBundle's CSP tags:

{% cspscript %} <script> window.api_key = '{{ api_key }}'; </script> {% endcspscript %}

(this modifies the <script> tag to add the correct CSP digest)

Twig's {% autoescape %} is also outputting (the escaped content). {% spaceless %} used to do this (but has been moved to filter + apply, not sure if there is a particular reason for that). The {% apply %} tag itself is outputting by definition (as it processes the contents using a Twig filter).

That's a different use case though, as it modifies the content of the tag (a bit like autoescape would). It doesn't display anything by itself.

I think I would be in favor of keeping in functions. It matches the way the rest of Symfony works (esp. forms) and it doesn't have that big of an impact on DX IMO. IDE can usually autocomplete functions as well as tags.

Well... what you are creating here is a particular type of block, isn't it? You always expect a start and an end so to me tags feel much more appropriate.

Suggested change

{{ turbo_frame_end() }}

{% turboframe "name" %}

Your stuff.

{% endturboframe %}

Well, these 2 functions are about rendering some HTML opening and closing tags. they are indeed not a kind of twig block

What happens if I forget some turbo_frame_end() function calls?

the HTML tag is not closed, and the browser may auto-close it in a different place than where you expect it to be closed.

tgalopin · 2021-02-26T17:10:10Z

Fixed some reviews, checking PHP 8 support

nicolas-grekas

We might need a pack to install mercure-bundle + ux-turbo at the same time, with a working recipe

nicolas-grekas · 2021-02-26T18:55:35Z

src/Turbo/DependencyInjection/TurboExtension.php

+        $config = $this->processConfiguration($configuration, $configs);
+
+        // Register Turbo Streams config
+        if (!empty($config['streams']['adapter'])) {


we try to avoid using empty as much as possible

nicolas-grekas · 2021-02-26T18:55:58Z

src/Turbo/DependencyInjection/TurboExtension.php

+        }
+
+        // Add Twig extension
+        if (class_exists(Environment::class)) {


could use an early return statement

nicolas-grekas · 2021-02-26T18:58:48Z

src/Turbo/DependencyInjection/Configuration.php

+
+        $rootNode
+            ->children()
+                ->arrayNode('streams')


maybe we could remove this nesting level and rename adapter to stream_adapter? (same for options)

nicolas-grekas · 2021-02-26T19:00:19Z

src/Turbo/DependencyInjection/Configuration.php

+        $rootNode
+            ->children()
+                ->arrayNode('streams')
+                    ->info('Enable this section to use Turbo Streams')


there is nothing to enable in this config :)
maybe this should be removed? I don't see similar comments elsewhere.

nicolas-grekas · 2021-02-26T19:01:41Z

src/Turbo/DependencyInjection/TurboExtension.php

+/**
+ * @author Titouan Galopin <[email protected]>
+ *
+ * @internal


I agree with @wouterj here, we should remove @internal imho.
Maybe we can make the class final also?

nicolas-grekas · 2021-02-26T19:24:01Z

src/Turbo/Twig/TurboStreamExtension.php

+            }
+        }
+
+        return trim($html).'></div>';


return substr_replace($html, '></div>', -1);

nicolas-grekas · 2021-02-26T19:27:51Z

src/Turbo/Twig/TurboFrameExtension.php

+            }
+        }
+
+        return trim($html).'>';


return substr_replace($html, '>', -1);

nicolas-grekas · 2021-02-26T19:32:48Z

src/Turbo/Twig/TurboStreamExtension.php

+    public function getFunctions(): array
+    {
+        return [
+            new TwigFunction('turbo_stream_from', [$this, 'streamFrom'], ['needs_environment' => true, 'is_safe' => ['html']]),


turbo_stream_listen?

nicolas-grekas · 2021-02-26T19:41:01Z

src/Turbo/Twig/TurboFrameExtension.php

+    {
+        return [
+            new TwigFunction('turbo_frame_start', [$this, 'startFrame'], ['needs_environment' => true, 'is_safe' => ['html']]),
+            new TwigFunction('turbo_frame_end', [$this, 'endFrame'], ['is_safe' => ['html']]),


maybe we don't need these helpers at all and we should tell ppl to write HTML directly?

nicolas-grekas · 2021-02-26T19:54:34Z

src/Turbo/README.md

+        // Let's imagine $message is the message received from the request (form, query param, ...)
+
+        $publisher(new Update('chat', '
+            <turbo-stream action="append" target="messages">


it could be great to have a service to help generate these tags via some helper service,

the service should be able to wrap HTML content, text content or a twig template. We could take inspiration from e.g. TemplatedEmail.

Yep definitely agree with @nicolas-grekas here, feels bizarre to ask developers to write this while they write turbo_frame_start() in Twig.

nicolas-grekas · 2021-02-26T20:29:02Z

src/Turbo/Twig/TurboStreamExtension.php

+            'symfony--ux-turbo--core' => [],
+        ]).' ';
+
+        foreach ($attrs as $name => $value) {


maybe we don't need attrs at all here btw?

nicolas-grekas · 2021-02-26T20:36:25Z

src/Turbo/Twig/TurboStreamExtension.php

+    {
+        $options = array_merge($this->options, ['id' => $id]);
+
+        $html = '<div ';


I'm wondering: should this generate an html tag, or a list of attributes?

sroze

That looks fun!

sroze · 2021-03-04T14:56:43Z

src/Turbo/README.md

+        // Let's imagine $message is the message received from the request (form, query param, ...)
+
+        $publisher(new Update('chat', '
+            <turbo-stream action="append" target="messages">


Yep definitely agree with @nicolas-grekas here, feels bizarre to ask developers to write this while they write turbo_frame_start() in Twig.

sroze · 2021-03-04T14:59:29Z

src/Turbo/README.md

+    <a href="/messages">Back</a>
+
+<!-- End the frame -->
+{{ turbo_frame_end() }}


Well... what you are creating here is a particular type of block, isn't it? You always expect a start and an end so to me tags feel much more appropriate.

Suggested change

{{ turbo_frame_end() }}

{% turboframe "name" %}

Your stuff.

{% endturboframe %}

sroze · 2021-03-04T15:01:16Z

src/Turbo/README.md

+
+{% block body %}
+    {# This line configures Turbo to listen to the "chat" topic on Mercure #}
+    {{ turbo_stream_from('chat') }}


Though I appreciate you technically want it to live within a block in here, semantically it would make greater sense to register it within your Twig file like that:

{% listen_turbostream "chat" %}

beberlei · 2021-03-21T13:41:55Z

I think this should be named symfony/ux-turbo-streams, because it has nothing to do with Turbo support in general, which works without this PR by just installing the npm/yarn package.

tgalopin · 2021-03-22T08:50:37Z

Closing in favor of the other PR

tgalopin force-pushed the ux-turbo branch 2 times, most recently from 1fbc6b9 to f7c6e63 Compare February 19, 2021 20:12

nicolas-grekas reviewed Feb 20, 2021

View reviewed changes

stof reviewed Feb 22, 2021

View reviewed changes

tgalopin added 4 commits February 26, 2021 17:59

Introduce Symfony UX Turbo

71b4520

Fix disconnect issue

c7ac1ec

Update annotations

123432d

Fix eslint config

884463f

tgalopin force-pushed the ux-turbo branch from 6276fe4 to 3742071 Compare February 26, 2021 17:09

Fix reviews

35852c7

tgalopin force-pushed the ux-turbo branch from 3742071 to 35852c7 Compare February 26, 2021 17:12

Remove PHP 8 support patch

58df899

nicolas-grekas reviewed Feb 26, 2021

View reviewed changes

dunglas mentioned this pull request Mar 2, 2021

Symfony UX Turbo: content negotiation and Doctrine integration #64

Closed

1 task

sroze reviewed Mar 4, 2021

View reviewed changes

tgalopin closed this Mar 22, 2021

tgalopin deleted the ux-turbo branch March 22, 2021 08:50

-{{ turbo_frame_end() }}
+{% turboframe "name" %}
+Your stuff.
+{% endturboframe %}

Uh oh!

Introduce Symfony UX Turbo #61

Introduce Symfony UX Turbo #61

Uh oh!

Conversation

tgalopin commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgalopin commented Feb 20, 2021

Uh oh!

Kocal commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgalopin commented Feb 20, 2021

Uh oh!

nicolas-grekas commented Feb 20, 2021 • edited by chalasr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgalopin commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tgalopin commented Feb 19, 2021 •

edited

Loading

Kocal commented Feb 20, 2021 •

edited

Loading

nicolas-grekas commented Feb 20, 2021 •

edited by chalasr

Loading

wouterj Feb 22, 2021 •

edited

Loading

tgalopin commented Feb 26, 2021 •

edited

Loading