Skip to content

[Turbo] Add Helper/TurboStream::append() et al. methods #2196

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
Oct 2, 2024

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Sep 23, 2024

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

For thoughts, this might help, WDYT?

return new TurboResponse(TurboStream::append($target, $html));

(Note that I'm also wondering about having a twig component to render streams from the twig side, stay tuned)

@carsonbot carsonbot added Feature New Feature Turbo Status: Needs Review Needs to be reviewed labels Sep 23, 2024
@nicolas-grekas nicolas-grekas force-pushed the turbo-helper branch 2 times, most recently from 1fdcd34 to 1e0a0cc Compare September 23, 2024 10:43
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

I'm not a Turbo user, but I think it can be useful yes.

Just, can you add some documentation for this new feature please? And also fix failing checks (IMO we can ignore the error found by PHPStan).

Thanks! :)

@nicolas-grekas
Copy link
Member Author

Updated

@rvmourik
Copy link

Great addition imho, but what about multiple streams in a single response? Or is that the Twig solution you are talking about @nicolas-grekas?

return new TurboResponse(TurboStream::append($target, $html));

Regarding DX it is not optimal to have this I guess:

return new TurboResponse(TurboStream::append($target, $html) . TurboStream::replace($target2, $html2));

Is this something to consider?

@smnandre
Copy link
Member

Thanks a lot @nicolas-grekas! More comments tomorrow :)

@nicolas-grekas
Copy link
Member Author

@rvmourik concatenation FTW indeed. I don't see a better option. In the end, any alternative would just be a fancy way to do concatenation.

@rvmourik
Copy link

@rvmourik concatenation FTW indeed. I don't see a better option. In the end, any alternative would just be a fancy way to do concatenation.

@nicolas-grekas thanks for the quick reply.

I know the TurboResponse is extending the base Response class but if we take a look at the JsonResponse there is also an option to pass an array or a mixed value, when it is mixed in the end is converts it to an ArrayObject. Could the TurboResponse do something similar, always convert the passed stream(s) to be a collection of streams and processing them before passing it to the base Response?

Maybe I am overthinking it? 😇

@javiereguiluz
Copy link
Member

javiereguiluz commented Sep 24, 2024

If we decide to add some helpers to make using Turbo more user-friendly, what if we all DX and provide a fluent interface to not deal with any internal details?

return TurboResponse()
    ->append($targetId, $html)
    ->prepend($targetId, $html)
    ->replace($targetId, $html, $method = 'morph')
    ->update($targetId, $html, $method = 'morph')
    ->remove($targetId, $html)
    ->before($targetId, $html)
    ->after($targetId, $html)
    // useful if the action (append, prepend, etc.) is a PHP variable:
    ->stream($action, $targetId, $html)
;

@rvmourik your example now would look like this:

// before
return new TurboResponse(TurboStream::append($target, $html) . TurboStream::replace($target2, $html2));

// after
return new TurboResponse()
    ->append($target, $html)
    ->append($target2, $html2)
;

There's a final consideration, which is not covered either in the current proposal of this PR. In addition to target (the targetID), you can use targets (a CSS selector) to apply the stream to multiple targets (See https://turbo.hotwired.dev/handbook/streams#actions-with-multiple-targets).

Adding an optional targets param would look ugly in my opinion:

return TurboResponse()
    ->append(string|null $targetId, string|null $targets, string $html)
    ->prepend(string|null $targetId, string|null $targets, string $html)
    // ...

We could add a *All() series of methods:

return TurboResponse()
    ->append(string $targetId, string $html)
    ->appendAll(string $targets, string $html)

    ->prepend(string $targetId, string $html)
    ->prependAll(string $targets, string $html)
    // ...

What do you think?

@rvmourik
Copy link

If we decide to add some helpers to make using Turbo more user-friendly, what if we all DX and provide a fluent interface to not deal with any internal details?

return TurboResponse()
    ->append($targetId, $html)
    ->prepend($targetId, $html)
    ->replace($targetId, $html, $method = 'morph')
    ->update($targetId, $html, $method = 'morph')
    ->remove($targetId, $html)
    ->before($targetId, $html)
    ->after($targetId, $html)
    // useful if the action (append, prepend, etc.) is a PHP variable:
    ->stream($action, $targetId, $html)
;

@rvmourik your example now would look like this:

// before
return new TurboResponse(TurboStream::append($target, $html) . TurboStream::replace($target2, $html2));

// after
return new TurboResponse()
    ->append($target, $html)
    ->append($target2, $html2)
;

There's a final consideration, which is not covered either in the current proposal of this PR. In addition to target (the targetID), you can use targets (a CSS selector) to apply the stream to multiple targets (See https://turbo.hotwired.dev/handbook/streams#actions-with-multiple-targets).

Adding an optional targets param would look ugly in my opinion:

return TurboResponse()
    ->append(string|null $targetId, string|null $targets, $html)
    ->prepend(string|null $targetId, string|null $targets, $html)
    // ...

We could add a *All() series of methods:

return TurboResponse()
    ->append(string $targetId, $html)
    ->appendAll(string $targets, $html)

    ->prepend(string $targetId, string $html)
    ->prependAll(string $targets, $html)
    // ...

What do you think?

I think this is a good suggestion, maybe in a future PR the same can be done with adding a helper for a regular turbo frame. I find my self often doing one of the following:

a) creating a twig file only with the frame and HTML in it
b) manually building a response like in the helper in this PR en load the twig file between the tag in the response.

Thanks all.

@nicolas-grekas
Copy link
Member Author

what if we all DX and provide a fluent interface

Works for me, I added the fluent methods to the TurboStreamResponse class.
Note that I don't know yet if that should be the recommended way to generate turbo stream: the drawback of this approach is that it splits targets from actual HTML, so that it can be easy to forget to update a target after updating a template. Putting the stream tags in the template that they target would improve this by providing locality.

In addition to target (the targetID), you can use targets (a CSS selector) to apply the stream to multiple targets

You might have missed this in the attached patch: this PR uses only targets, with an "s". That means you have to use #foo to target id foo. To me, this is the most empowering and DX-friendly way to express the target. Using target (no "s") provides zero benefits.

private static function wrap(string $action, string $target, string $html, string $attr = ''): string
{
return \sprintf(<<<EOHTML
<turbo-stream action="%s" targets="%s"%s>
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the "s" to targets, and the docblock comments: we support only CSS selectors, not pure IDs (CSS selectors are more capable anyway).

@nicolas-grekas
Copy link
Member Author

PR ready IMHO (failure unrelated IIUC)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Oct 2, 2024
@nicolas-grekas
Copy link
Member Author

All green :)

@Kocal
Copy link
Member

Kocal commented Oct 2, 2024

Thank you @nicolas-grekas.

@Kocal Kocal merged commit bca5384 into symfony:2.x Oct 2, 2024
63 checks passed
Kocal added a commit that referenced this pull request Oct 2, 2024
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Add `<twig:Turbo:Stream:*>` components

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

This PR provides Twig components for turbo-streams:

```html
<twig:Turbo:Stream:Append target="#some-id">
    <div>Append this</div>
</twig:Turbo:Stream:Append>
```

Sibling PR to #2196
See #2196 (comment) for some background.

Commits
-------

bfdbb46 CI love
356dbc2 Add `<twig:Turbo:Stream:*>` components
@nicolas-grekas nicolas-grekas deleted the turbo-helper branch October 2, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer Turbo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants