Skip to content
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

[Turbo] Pass EventSource options to turbo_stream_listen #2447

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

Fan2Shrek
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Issues Fix #1860 I think
License MIT

This allows to pass options to the EventSource, for example
{{ turbo_stream_listen('a_topic', 'default', { withCredentials: true }) }}
Or create specific auth cookie
{{ turbo_stream_listen('a_topic', 'default', { subscribe: '*' }) }}
This functionality is similar to how mercure() works

@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Dec 12, 2024
Copy link

github-actions bot commented Dec 12, 2024

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
Turbo
turbo_stream_controller.d.ts 562 B / 280 B 651 B+16% 📈 / 303 B+8% 📈
turbo_stream_controller.js 1.27 kB / 535 B 1.35 kB+6% 📈 / 565 B+6% 📈

@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch 3 times, most recently from 867707d to 06ac699 Compare December 12, 2024 22:35
@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Dec 13, 2024
@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from 06ac699 to abcc723 Compare December 13, 2024 18:02
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Dec 13, 2024
@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from abcc723 to e95cbab Compare December 13, 2024 18:14
@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch 3 times, most recently from f92e643 to e56b74c Compare December 14, 2024 22:53
@Fan2Shrek
Copy link
Contributor Author

Hey @smnandre,
Thanks for taking the time to explain this to me ! 😊

However, the last commit seems a bit off to me. Could you confirm if that's what you had in mind ?
Also, since the cookie logic was moved to the Mercure TurboStreamListenRenderer, is the Twig runtime still relevant ?

@smnandre
Copy link
Member

Hi seb-jean Guervyl gremo DRaichev and anyone who want to :)

Well.... It’s time to lay all this out clearly! 😊

Could you share your ideal Developer Experience (DX) suggestions?
As much in PHP code than in Twig // Give examples of code usage
Once we have a solid understanding of what you’re aiming for, we can figure out the best way to make it happen together.

Please clarify where and how you’d use this feature:
For example, in a page template, a component, a full-page view, multiple times, or other contexts. This will help us better target the needs and define responsibilities.

Looking forward to your insights!

@carsonbot carsonbot removed the Status: Needs Review Needs to be reviewed label Feb 22, 2025
@smnandre smnandre changed the title Add options to turbo_stream_listen [Turbo] Pass EventSource options to turbo_stream_listen Feb 22, 2025
@smnandre smnandre added Status: Reviewing Review is ongoing, refining with author Turbo and removed Status: Needs Work Additional work is needed labels Feb 22, 2025
@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from e56b74c to 4d9f2a9 Compare February 22, 2025 22:27
@smnandre
Copy link
Member

TwigComponent errors are unrelated

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

@Fan2Shrek it took too much time, i'm sorry for this. Thank you very much for this work.

@seb-jean & @tibobaldwin thank you for the recent pokes :)

@carsonbot carsonbot added the Status: Reviewed Has been reviewed by a maintainer label Feb 22, 2025
@Fan2Shrek
Copy link
Contributor Author

Hey @smnandre, thanks for the help! Unfortunately, there's still a problem with the TurboStreamListenRenderer. It doesn't create the cookie—I think it's because the class dependencies aren't resolved, but I have no clue why.

@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from 4d9f2a9 to 961ef08 Compare February 22, 2025 23:15
@Fan2Shrek
Copy link
Contributor Author

Fan2Shrek commented Feb 22, 2025

I found that TurboStreamListenRenderer is configured by the Mercure bundle here. Should we open a PR there as well?

Using Mercure to create the cookies causes the tests to fail.

@smnandre
Copy link
Member

I promise you I did not remember this... but now it happens it feels a bit ironic 😆

Well, let's move on :)

You can try this: #2447 (comment)

The only think you cannot is "configure" a service from another bundle/bridge/etc.

So if you find a way to inject the service directly it's perfect (just use ignoreOnInvalid to not pollute our code if there were a problem one day)

If you don't, I'll have a look later during the night 🦇 or tomorrow

@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from 583aab6 to 5b429a7 Compare February 22, 2025 23:46
@Fan2Shrek
Copy link
Contributor Author

Fan2Shrek commented Feb 22, 2025

I adapted the code to use the Twig Mercure extension.
For the injection part, I'm not good enough to find a proper solution. 😅

@smnandre
Copy link
Member

Well well well ... 👼 (1/1)

Capture d’écran 2025-02-23 à 01 21 57

#2460

Well well well .... 👼 (2/2)

     *
     * @return string The URL of the hub with the appropriate "topic" query parameters (if any)
     */
    public function mercure($topics = null, array $options = []): string

https://github.com/symfony/mercure[•••]src/Twig/MercureExtension.php

@smnandre
Copy link
Member

For the injection part, I'm not good enough to find a proper solution. 😅

Does it work ? If not, what is missing here ? How do you test?

@Fan2Shrek
Copy link
Contributor Author

I tested it on a small app with Mercure already set up. The only problem I’m facing is how to inject TwigEnvironment to get MercureExtension. Since TurboStreamRenderer is configured by the MercureBundle, it doesn't inject the new dependencies.

I’m not sure what the 'proper' way to solve this problem is.

{
private StimulusHelper $stimulusHelper;

public function __construct(
private HubInterface $hub,
StimulusHelper|StimulusTwigExtension $stimulus,
private IdAccessor $idAccessor,
private Environment $twig,
Copy link
Member

Choose a reason for hiding this comment

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

did you update the config for this argument ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I? The config is handled by MercureBundle.

Copy link
Member

Choose a reason for hiding this comment

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

How so ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Well... back to step1.

Honestly it makes not much sense to me MercureBundle is the one configuring classes on this bundle :|

Could you start a conversation on the MercureBundle repository and see what's the best way to handle this ?

@Fan2Shrek Fan2Shrek force-pushed the add-option-to-turbo-stream branch from ed131f3 to a59fd33 Compare February 28, 2025 23:03
@Fan2Shrek
Copy link
Contributor Author

Hey @smnandre, this finally looks good to me :)
Let me know if there are any details to adjust .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Feature New Feature Status: Reviewed Has been reviewed by a maintainer Status: Reviewing Review is ongoing, refining with author Turbo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Turbo] Add mercure() to turbo_stream_listen()
5 participants