[Asset] Add support for preloading with links and HTTP/2 push #21478

Closed
wants to merge 16 commits into
from
@dunglas
Member
dunglas commented Jan 31, 2017 edited
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR todo

Allows compatible clients to preload mandatory assets like scripts, stylesheets or images according to the "preload" working draft of the W3C.
Thanks to this PR, Symfony will automatically adds Link HTTP headers with a preload relation for mandatory assets. If an intermediate proxy supports HTTP/2 push, it will convert preload headers. For instance Cloudflare supports this feature.

It dramatically increases pages speed and make the web greener because only one TCP connection is used to fetch all mandatory assets (decrease servers and devices loads, improve battery lives).

Usage:

Updated version:

<html>
    <body>
    Hello
    <script src="{{ preload(asset('/scripts/foo.js'), 'script') }}"></script>
    </body>
</html>

First proposal:

<html>
    <body>
    Hello
    <script src="{{ preloaded_asset('/scripts/foo.js', 'script') }}"></script>
    </body>
</html>
  • Add tests
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jan 31, 2017
@nicolas-grekas nicolas-grekas changed the title from [Asset][WIP] Add support for preloading with links and HTTP/2 push to [Asset] Add support for preloading with links and HTTP/2 push Jan 31, 2017
@dunglas
Member
dunglas commented Jan 31, 2017 edited

Test and nopush support added.

Status: needs review

@pkruithof
Contributor

Very nice! We were about to implement this ourselves.

Why use a different function for this? Would an attribute not be more logical?

{{ asset('/scripts/foo.js', 'script', {preload: true}) }}
+ */
+class HttpFoundationPreloadManager implements PreloadManagerInterface
+{
+ private $resources = array();
@lyrixx
lyrixx Feb 1, 2017 Member

Can you get rides of this state? Because right now it's a memory leak IIUC.

@dunglas
dunglas Feb 1, 2017 Member

No it's not possible because this state gather all assets to add to the link.
However I can add a new method to clear it that will be called in the listener to avoid the memory leak.

@stof
stof Feb 1, 2017 Member

we indeed need to clear the state. Otherwise, using the same kernel to handle multiple requests would leak resources from the previous request. Your implementation breaks the isolation of request handling

@javiereguiluz
Member

I have the same question as @pkruithof: why adding a new preloaded_asset() function instead of adding config options (globally for asset package config and locally for the asset() function).

@dunglas
Member
dunglas commented Feb 1, 2017 edited

@pkruithof @javiereguiluz it was my 1st though, however the current signature is: { asset('/path', 'packageName') }.
Both { asset('/path', null, true, 'script', false) } and { asset('/path', null, {'preload': true, 'nopush': true, 'as': 'script') } look bad.
Changing parameters order isn't possible for BC. It's why I've introduced this new tag.

@javiereguiluz
Member

@dunglas thanks for the explanation. Another question: should we name this function asset_preload() instead?

First, it would match the naming followed by other functions, where the first word is "the common thing" (e.g. render_*(), form_*(), asset_*(), etc.)

Second, it would look better when using composition:

{{ preloaded_asset(asset('/scripts/foo.js'), 'script') }}
{{ asset_preload(asset('/scripts/foo.js'), 'script') }}
+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+class PreloadListener
@stof
stof Feb 1, 2017 Member

should be an event subscriber IMO, like our other listeners, to embed the knownledge about the event being listened to

src/Symfony/Component/Asset/Package.php
+ }
+
+ $url = $this->getUrl($path);
+ $this->preloadManager->addResource($url, $as, $nopush);
@stof
stof Feb 1, 2017 Member

should the package really be the part being aware of the PreloadManager ? I don't think so. IMO, it should be done at a higher level in the stack. The preloading part is totally independent from the asset url building anyway

+ */
+class HttpFoundationPreloadManager implements PreloadManagerInterface
+{
+ private $resources = array();
@stof
stof Feb 1, 2017 Member

we indeed need to clear the state. Otherwise, using the same kernel to handle multiple requests would leak resources from the previous request. Your implementation breaks the isolation of request handling

+ *
+ * @param Response $response
+ */
+ public function setLinkHeader(Response $response)
@stof
stof Feb 1, 2017 Member

a more reusable API would be to have a method returning the header instead, without dependency on the Response. The event listener would then handle the binding to HttpFoundation by setting the header (allowing the same manager to be reused by people using a PSR-7 stack instead, without reimplementing all the logic in a different class just because of this method).
Currently, the most important part of the business logic is in this method, which is not part of the interface

@dunglas
Member
dunglas commented Feb 1, 2017

@stof (thanks!!), @lyrixx and @javiereguiluz comments took into account:

  • The preload feature is now 100% independent of the packages and has a Twig function of its own.
  • Dealing with the HttpFoundation's Response is only done in the listener (the manager can now be reused with any HTTP message implementation)
  • Memory leak fixed

The new syntax:

<html>
    <body>
    Hello
    <script src="{{ preload(asset('/scripts/foo.js'), 'script') }}"></script>
    </body>
</html>
@@ -765,6 +767,15 @@ private function registerAssetsConfiguration(array $config, ContainerBuilder $co
$defaultVersion = $this->createVersion($container, $config['version'], $config['version_format'], '_default');
}
+ if (class_exists(PreloadManager::class)) {
@stof
stof Feb 1, 2017 Member

missing class existence resource (or you could just conflict with asset < 3.3, and have all this in the XML file directly)

@dunglas
dunglas Feb 1, 2017 Member

I don't get what you mean. But if it's ok to bump the dependency, I'll move this definition to the XML, it's cleaner.

- return $package
+ $package
@stof
stof Feb 1, 2017 Member

Why editing this code ?

@dunglas
dunglas Feb 1, 2017 edited Member

I've done that in a previous commit and it's no useless but I find the new version cleaner/simpler. I can revert it if you prefer the old one.

@stof
stof Feb 2, 2017 Member

changing this without a need for it makes it harder to merge branches together for no reason

@@ -375,6 +376,16 @@ public function testAssetsDefaultVersionStrategyAsService()
$this->assertEquals('assets.custom_version_strategy', (string) $defaultPackage->getArgument(1));
}
+ public function testAssetHasPreloadListener()
+ {
+ if (!class_exists(PreloadListener::class)) {
@stof
stof Feb 1, 2017 Member

composer ensures that this is always existing

@dunglas
dunglas Feb 1, 2017 edited Member

Not if a version for symfony/asset lesser than 3.3 has been installed, right?

@stof
stof Feb 2, 2017 Member

it cannot, due to the require-dev constraint

+
+ public function onKernelResponse(FilterResponseEvent $event)
+ {
+ if ($value = $this->preloadManager->getLinkValue()) {
@stof
stof Feb 1, 2017 Member

this logic must be done only for the master request.
An asset referenced in a subrequest must be preloaded by the master request, as it is the one being sent to the user, and your code would also use all previous master preloaded asset for the subrequest, thus breaking preloading.

+ foreach ($this->resources as $uri => $options) {
+ $part = "<$uri>; rel=preload";
+ if ('' !== $options['as']) {
+ $part .= "; as={$options['as']}";
@stof
stof Feb 1, 2017 Member

We generally use sprintf rather than interpolation (even more when it is not simple interpolation)

@dunglas
dunglas Feb 1, 2017 Member

I've done it this way because of https://blog.blackfire.io/php-7-performance-improvements-encapsed-strings-optimization.html but after a second look, it looks like a micro-optimisation. I'll switch to sprintf.

+ *
+ * @param array $resources
+ */
+ public function setResources(array $resources);
@stof
stof Feb 1, 2017 Member

do we have any use case for replacing all resources, except for clearing ? If no, we could simplify the code by allowing only clearing

+ *
+ * @return string|null
+ */
+ public function getLinkValue();
@stof
stof Feb 1, 2017 Member

I think getLinkHeader (or buildLinkHeader) might be a better name

+ }
+
+ if (!isset($options['as']) || !is_string($options['as'])) {
+ throw new InvalidArgumentException('The "as" option is mandatory and must be a string.');
@javiereguiluz
javiereguiluz Feb 1, 2017 Member

I have a question about this behavior because I'm not aware of the related specification. If as is mandatory, would it make sense to make it optional and default its value to the appropriate value according to the extension of the asset? For example, if the asset is foo.js then use script as as automatically.

@dunglas
dunglas Feb 1, 2017 edited Member

@javiereguiluz according to the spec it's not mandatory.

I was thinking about a guesser, but without inspecting the content of the file it's difficult to do it reliably.

@dunglas
Member
dunglas commented Feb 1, 2017

@stof done. I've also remove the PreloadManager::getResources() because it was leaking an internal state and was not used.

@dunglas
Member
dunglas commented Feb 1, 2017 edited

By the way, the preload system is now 100% decoupled of the rest of the Asset component. It may be moved to its own component and its own Twig extension but I'm not sure it's worth it (it's only a couple of files).

- public function __construct(Packages $packages)
+ public function __construct(Packages $packages, PreloadManagerInterface $preloadManager = null)
@fabpot
fabpot Feb 1, 2017 Member

The default PreloadManagerInterface implementation is simple and does not have dependencies, so I suppose it does not make sense to have a proper runtime.

@dunglas
dunglas Feb 3, 2017 Member

Sorry I don't get what you mean.

+{
+ private $preloadManager;
+
+ public function __construct(PreloadManager $preloadManager)
@stof
stof Feb 2, 2017 Member

the typehint must use the interface

+ }
+
+ if ($value = $this->preloadManager->buildLinkValue()) {
+ $event->getResponse()->headers->set('Link', $value);
@stof
stof Feb 2, 2017 Member

you must pass false as third argument. You don't want to replace the existing Link headers which might exist for other purposes

+ $this->assertInstanceOf(EventSubscriberInterface::class, $subscriber);
+ $this->assertEquals('</foo>; rel=preload', $response->headers->get('Link'));
+ $this->assertNull($manager->buildLinkValue());
+ }
@stof
stof Feb 2, 2017 Member

please add a test where the response already has another Link header previously

@dunglas
Member
dunglas commented Feb 3, 2017

Should be all good now.

Status: needs review

@xabbuh
Member
xabbuh commented Feb 6, 2017

General question: Do we want to merge the PR as long as the spec document is in draft state?

@dunglas
Member
dunglas commented Feb 6, 2017 edited

@xabbuh it's already broadly used in the wild: Chrome and Opera support this feature, Firefox and Webkit are implementing it, Edge is considering implementing it (https://developer.microsoft.com/en-us/microsoft-edge/platform/status/preload/) and - more interestingly - CloudFlare already supports the transparent conversion of preload links to HTTP/2 pushes so there are immediate benefits for any client supporting HTTP/2 (80% of all modern browsers) if the website uses CloudFlare.

Maybe can we mark it as @experimental in the (unlikely) eventuality that the spec change but I think we should merge it as soon as possible to allow our users to benefit of the already existing performance boost.

@dunglas
Member
dunglas commented Feb 7, 2017

@xabbuh btw it's an hot topic, the @GoogleChrome team just released a Webpack plugin that looks very similar: https://github.com/googlechrome/preload-webpack-plugin (but our solution is better because CloudFlare supports only HTTP headers, not HTML links yet).

They also support prefetch while we just support preload for now. What do you think of adding a new prefetch Twig tag?

+ public function testGetAndPreloadAssetUrl()
+ {
+ if (!class_exists(PreloadManager::class)) {
+ $this->markTestSkipped('Requires asset 3.3 or superior.');
@fabpot
fabpot Feb 7, 2017 Member

Requires Asset 3.3+

+ public function buildLinkValue()
+ {
+ if (!$this->resources) {
+ return null;
+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+interface PreloadManagerInterface
@fabpot
fabpot Feb 7, 2017 Member

Why do we need this interface for?

@dunglas
dunglas Feb 7, 2017 edited Member

This extension point allows to replace the manager by a smarter one (for instance if you want to create one giving access to the list of resources added or to change them programmatically).
IMO it doesn't hurt and allow to easily replace the global app manager to add custom behaviors.

@stof
stof Feb 7, 2017 Member

it could also allow something to add a profiler panel reporting the preloaded resources for instance, by using a decorator implementation

@@ -19,10 +19,12 @@
"php": ">=5.5.9"
},
"suggest": {
- "symfony/http-foundation": ""
+ "symfony/http-foundation": "",
+ "symfony/http-kernel": ""
@fabpot
fabpot Feb 7, 2017 Member

I would remove this.

dunglas added some commits Jan 31, 2017
@dunglas dunglas [Asset] Add suport for preloading with links and HTTP/2 push 64bd530
@dunglas dunglas Add tests and nopush support f920ca4
@dunglas dunglas CS 1de19cc
@dunglas dunglas Fix tests c5ca4bc
@dunglas dunglas Preload is now a standalone Twig function 639d7d1
@dunglas dunglas Make the preload manager independant of HTTP Foundation 9c11a0f
@dunglas dunglas Fix CS d39b4f1
@dunglas dunglas Fix listener 581c700
@dunglas dunglas Fix @stof's comments 693107c
@dunglas dunglas Fix CS 2d8d750
@dunglas dunglas Don't replace existing link 0b7fe14
@dunglas dunglas Fix tests dffb7b6
@dunglas dunglas Some more fixes 18412e5
@dunglas dunglas Remove HttpKernel from suggest
78d7b6a
@dunglas dunglas Remove unseless null
05134b6
@dunglas dunglas Update skip message
a7f77b4
+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+class PreloadManager implements PreloadManagerInterface
@xabbuh
xabbuh Feb 7, 2017 Member

could be final

@nicolas-grekas
nicolas-grekas Feb 11, 2017 Member

would forbid decoration for no reason imho - see comments on the interface: building a profiler panel could require decorating this class

@nicolas-grekas
nicolas-grekas Feb 11, 2017 Member

I meant decoration by inheritance of course.
My root issue is that final here would be only forbidding use cases while providing no benefit for us.
"final" should only used on method/classes that are not bound by any contract - like data objects.
When an interface covers the said methods, the contract is already enforced and inheritance should not be prevented.

@unkind
unkind Feb 12, 2017 Contributor

My root issue is that final here would be only forbidding use cases while providing no benefit for us.

There are 2 benefits:

  • changing the methods is less risky, see for example: #11708, it was rejected just because of potential BC breaking;
  • discourage "decoration by inheritance" when you have an interface with the same methods: in most cases there is no sane reason to do that, it is probably design mistake.

When an interface covers the said methods, the contract is already enforced and inheritance should not be prevented.

I don't see connection between contract enforcing and "inheritance should not be prevented". I'd say the opposite. See similar opinion, for example: http://ocramius.github.io/blog/when-to-declare-classes-final/ /cc @Ocramius

@fabpot
Member
fabpot commented Feb 19, 2017

Thank you @dunglas.

@fabpot fabpot closed this Feb 19, 2017
@fabpot fabpot added a commit that referenced this pull request Feb 19, 2017
@fabpot fabpot feature #21478 [Asset] Add support for preloading with links and HTTP…
…/2 push (dunglas)

This PR was squashed before being merged into the 3.3-dev branch (closes #21478).

Discussion
----------

[Asset] Add support for preloading with links and HTTP/2 push

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | todo

Allows compatible clients to preload mandatory assets like scripts, stylesheets or images according to [the "preload" working draft of the W3C](https://www.w3.org/TR/preload/).
Thanks to this PR, Symfony will automatically adds `Link` HTTP headers with a `preload` relation for mandatory assets.  If an intermediate proxy supports HTTP/2 push, it will convert preload headers. For instance [Cloudflare supports this feature](https://blog.cloudflare.com/using-http-2-server-push-with-php/).

It dramatically increases pages speed and make the web greener because only one TCP connection is used to fetch all mandatory assets (decrease servers and devices loads, improve battery lives).

Usage:

Updated version:

```html
<html>
    <body>
    Hello
    <script src="{{ preload(asset('/scripts/foo.js'), 'script') }}"></script>
    </body>
</html>
```

~~First proposal:~~

```html
<html>
    <body>
    Hello
    <script src="{{ preloaded_asset('/scripts/foo.js', 'script') }}"></script>
    </body>
</html>
```

- [x] Add tests

Commits
-------

7bab217 [Asset] Add support for preloading with links and HTTP/2 push
6d77cdf
@fabpot
Member
fabpot commented Feb 19, 2017

@dunglas Can you submit a PR on the docs?

@xabbuh xabbuh referenced this pull request in symfony/symfony-docs Feb 20, 2017
Open

Preloading support with links and HTTP/2 push #7515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment