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

[WebLink] improve web_link #20212

Merged
merged 1 commit into from
Sep 19, 2024
Merged

[WebLink] improve web_link #20212

merged 1 commit into from
Sep 19, 2024

Conversation

garak
Copy link
Contributor

@garak garak commented Sep 9, 2024

The current doc for generating the proper lines in the head section is quite misleading, acting as if it could be enough to use a single line to include a style/script, while two lines each are needed. Moreover, despite mentioning the need for the as attribute, if fails to add it in the examples.
The proposed changes should fix both problems.

@xabbuh
Copy link
Member

xabbuh commented Sep 16, 2024

Isn't the previous code enough because the application sends the required information as additional HTTP headers (see https://symfony.com/doc/current/web_link.html#how-does-it-work)?

@garak
Copy link
Contributor Author

garak commented Sep 16, 2024

Isn't the previous code enough because the application sends the required information as additional HTTP headers (see https://symfony.com/doc/current/web_link.html#how-does-it-work)?

It's enough for the headers, not for the proper HTML tags in the <head>

@javiereguiluz
Copy link
Member

On symfony.com we have something like this on templates:

{% block importmap %}
    {{ importmap(['app', 'search', 'home']) }}
{% endblock %}

And that generates these HTML tags:

<link rel="stylesheet" href="/assets/vendor/bootstrap/dist/css/bootstrap.min-1712f0378f8675ca7cd423d6262fcccf.css">
<link rel="stylesheet" href="/assets/styles/app-c5e5f7501da9d82bf95c197610450518.css">
<link rel="stylesheet" href="/assets/styles/search-8ccfaff80d2880d747e9efe1089469d2.css">
<!-- ... -->

That alone is enough to enable the preloading of those assets:

image

But, we use Cloudflare. In the HTTP Response headers, we can see those early hints headers:

image


So, maybe let's ask @dunglas who knows a lot about these things. Is the change proposed in this PR really needed when you don't use Cloudflare or some similar server? Thanks!

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

The change looks good to me. It fixes the fallback using a HTML link elements, when it's not possible/supported to use HTTP headers.

@carsonbot carsonbot changed the title improve web_link [WebLink] improve web_link Sep 19, 2024
@javiereguiluz javiereguiluz modified the milestone: 6.4 Sep 19, 2024
@javiereguiluz javiereguiluz changed the base branch from 7.1 to 6.4 September 19, 2024 14:39
@javiereguiluz javiereguiluz merged commit 6ff9491 into symfony:6.4 Sep 19, 2024
3 checks passed
@javiereguiluz
Copy link
Member

Merged! Massimiliano thanks for fixing this and Kévin thanks for reviewing it.

@javiereguiluz
Copy link
Member

Also, about my previous comment here (#20212 (comment)) I forgot that the importmap() function automatically adds the Link HTTP header for you when the WebLink component is installed. That's why you don't have to add it explicitly.

@garak garak deleted the fix-web-link branch September 19, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants