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

Add origin to internal tags #49

Merged

Conversation

seggewiss
Copy link
Contributor

I noticed that custom attribtues aren't applied to the Vite client script. This causes problems if you need to pass a nonce for example.

Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for cosmic-bubblegum-aa631a canceled.

Name Link
🔨 Latest commit 2582b24
🔍 Latest deploy log https://app.netlify.com/sites/cosmic-bubblegum-aa631a/deploys/67163a97df746a000846a526

@lhapaipai
Copy link
Owner

lhapaipai commented Oct 18, 2024

Hi @seggewiss,

thanks for your suggestion.
I understand your issue and indeed we should find a solution.
However some points in your PR bother me.

Actually, when you define custom attributes, as you could see they are assigned only to the associated entry point.

{{ vite_entry_script_tags('app', { attr: { nonce: 'foobar' }}) }}

render

<script type="module" src="http://127.0.0.1:5174/build/@vite/client" crossorigin></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/app.js" nonce="foobar"></script>

with your PR, the result would be:

<script type="module" src="http://127.0.0.1:5174/build/@vite/client" crossorigin nonce="foobar"></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/app.js" nonce="foobar"></script>

Several points bother me:

  • for your use case, indeed you would like to copy the custom attributes on the 2 tags. this is quite understandable for a nonce but I think that for other attributes, the behavior would not be desired.
  • the vite client is only included once per page, at the first invocation of the twig function vite_entry_script_tags. this means that depending on the order of the calls to the twig function the result would not be the same.
{{ vite_entry_script_tags('app', { attr: { foo: 'bar' }}) }}
{{ vite_entry_script_tags('form', { attr: { hello: 'world' }}) }}

render

<script type="module" src="http://127.0.0.1:5174/build/@vite/client" foo="bar" crossorigin></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/app.js" foo="bar"></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/form.js" hello="world"></script>

but this

{{ vite_entry_script_tags('form', { attr: { hello: 'world' }}) }}
{{ vite_entry_script_tags('app', { attr: { foo: 'bar' }}) }}

render

<script type="module" src="http://127.0.0.1:5174/build/@vite/client" hello="world" crossorigin></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/app.js" foo="bar"></script>
<script crossorigin type="module" src="http://127.0.0.1:5174/build/assets/form.js" hello="world"></script>

the behavior is easily predictible but if the twig functions are in different files with inheritance, I'm afraid we'll end up with a behavior that's hard to predict.

For all these reasons I'm a little reluctant for your proposal.

On the other hand, your issue can be easily solved by adding a listener on RenderAssetTagEvent...

doc Custom Attributes

// src/EventSubscriber/ScriptNonceSubscriber.php
namespace App\EventSubscriber;

use Pentatrion\ViteBundle\Event\RenderAssetTagEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class ScriptNonceSubscriber implements EventSubscriberInterface
{
    public function onRenderAssetTag(RenderAssetTagEvent $event)
    {
        $tag = $event->getTag();
        if ($tag->isInternal()) {
            $tag->setAttribute('nonce', 'your nonce');
        }
    }

    public static function getSubscribedEvents(): array
    {
        return [
            RenderAssetTagEvent::class => 'onRenderAssetTag',
        ];
    }
}

What do you think?
I would be happy to have your point of view...

Good evening

@seggewiss
Copy link
Contributor Author

Morning @lhapaipai 👋

I understand your concerns. Especially as this could potentially break existing integrations.

However our usecase is also not feasible via the RenderAssetTagEvent. I need to be able to add the nonce in one template for a specific Vite entry name. I have no way to check which app is being rendered in the Event. The tag does not carry the information and the Event itself also doesn't.

As I see it we have 2 Options

  1. Either we restrict the attributes to such that don't break even if rendered multiple times due to template inheritance. Implementing a blacklist only allowing attributes like nonce.
  2. Add the entry name to the Event information.

I'm happy to provide both approaches. I would prefer option 1 though 😊

@lhapaipai
Copy link
Owner

Hi @seggewiss,

It is possible to retrieve the entry point in the subscriber,
but only for tags that are not internal... so we cannot retrieve the entry point that launched the Vite client.

class ScriptNonceSubscriber implements EventSubscriberInterface
{
    public function onRenderAssetTag(RenderAssetTagEvent $event)
    {
        $tag = $event->getTag();
        if ($tag->getOrigin() === 'app') {
            
        } elseif ($tag->getOrigin() === '_internal') {

        }
    }

    public static function getSubscribedEvents(): array
    {
        return [
            RenderAssetTagEvent::class => 'onRenderAssetTag',
        ];
    }
}

implementing a blacklist / whitelist can be a good idea but again what bothers me is to define attributes intended for the Vite client that must be global to all entry points in a vite_entry_script_tags function that is specific to only one entry point.

if you invoke vite_entry_script_tags multiple times in the same page, unless you have exactly the same extra attributes for all functions we will end up with unpredictable behaviors.

the Vite client will have the attributes of the first entry point that will invoke it
and in this example the behavior can vary between child-1 and child-2.

child-1.html.twig

{% extends 'parent.html.twig' %}

{% block javascripts %}
{{ vite_entry_link_tags('app', { attr: { foo: 'bar' }}) }}
{% endblock %}

child-2.html.twig

{% extends 'parent.html.twig' %}

{% block javascripts %}{% endblock %}

parent.html.twig

<html>
    <head>
    {% block javascripts %}{% endblock %}
    {{ vite_entry_link_tags('app', { attr: { foo: 'baz' }}) }}
    </head>
</html>

In your case maybe it won't bother you because you will be aware of it but for a user who has not followed this discussion, the side effects could seem unpredictable.

a suggestion :

  • add a boolean property internal to the Tag class.
    when we instantiate an internal Tag, set this boolean to true and instead of assigning _internal to origin, assign the value of the entry point.

we could thus recover the entry point that launched the Vite client.

@seggewiss seggewiss force-pushed the allow-custom-attr-vite-client-script branch from dd78e8f to 2582b24 Compare October 21, 2024 11:27
@seggewiss seggewiss changed the title Allow custom attributes vite client script Add origin to internal tags Oct 21, 2024
@seggewiss
Copy link
Contributor Author

Hey @lhapaipai 👋

I hope this is to your liking. I checked all use cases of the Tag Class and adjusted accordingly.

Hope you have a nice rest of your day 😊

Copy link
Owner

@lhapaipai lhapaipai left a comment

Choose a reason for hiding this comment

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

it's perfect thank you very much

@lhapaipai lhapaipai merged commit 2582b24 into lhapaipai:main Oct 21, 2024
4 checks passed
@lhapaipai
Copy link
Owner

hi @seggewiss,
thanks a lot for your pr !
you can enjoy it in version 7.0.5 ...

@seggewiss
Copy link
Contributor Author

Hey @lhapaipai 👋
Thanks a lot for your guidance.

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.

2 participants