Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[AssetMapper] Adding nopush to Link header #52143

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

Closed
wants to merge 2 commits into from

Conversation

weaverryan
Copy link
Member

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets None
License MIT

See conversation here - #51829 (comment)

I am not 100% sure this is needed - I'd love an HTTP/2 expert (perhaps dunglas?) to weigh in on this. Notes:

A) The purpose of the Link header is to hint to the browser as early as possible that a CSS file is needed. However, a web server MAY see this and perform a server push. That is NOT desired, as it would push the asset on every request, even after it is cached.

B) So, adding nopush makes sense. But I'm not sure which web servers, in practice, are actually server-pushing at this time.

Note: HTTP/2 server push is removed from Chrome - https://developer.chrome.com/blog/removing-push/ - which means they send a SETTINGS_ENABLE_PUSH=0 setting when establishing the connection to ask the server to NOT push.

So I think this is a good change... unless it's totally not needed because web servers aren't server-pushing already. And I don't know if that's the case.

Cheers!

@stof
Copy link
Member

stof commented Oct 18, 2023

If nopush also disables the 103 early hints, that would be a bad move IMO.

@stof
Copy link
Member

stof commented Oct 18, 2023

note that as Chrome indicates that it does not want pushed resources, servers won't push them those resources.

@weaverryan
Copy link
Member Author

note that as Chrome indicates that it does not want pushed resources, servers won't push them those resources.

The question is: for people using Firefox, Safari, are those also indicating that they don't want pushed resources? I don't know the answer. And which web servers are pushing resources? Is this a real problem? Or in practice, is nothing being pushed already?

If nopush also disables the 103 early hints, that would be a bad move IMO.

I think you're correct that we would NOT want to include nopush in the Link headers of a 103 early-hint response. Currently, AssetMapper does not attempt to send a 103. So having the nopush on the main response shouldn't affect if a user DID try to send a 103. In the future, we could consider having AssetMapper send a 103. And in that case, indeed, we would want to use nopush.

@stof
Copy link
Member

stof commented Oct 18, 2023

AssetMapper is not the one sending the response at all when you render an import map. So early hints might still be used.

@weaverryan
Copy link
Member Author

AssetMapper is not the one sending the response at all when you render an import map. So early hints might still be used

Hmm. Here is the only flow I can imagine:

A) User manually adds Link header and sends the 103. This does not include nopush
B) Request continues. When the importmap is rendered, AssetMapper adds a Link header for the same asset with nopush

I'm not entirely sure what happens in this case. The best I can tell, this would be ok - but it is a bit odd. However, closing this PR would be a bigger problem, assuming that server-push is actually something that's happening in practice with non-Chrome browsers.

@OskarStark OskarStark requested a review from dunglas October 18, 2023 14:47
@dunglas
Copy link
Member

dunglas commented Oct 18, 2023

The push attribute of the preload link type seems to have been removed from the specification: https://html.spec.whatwg.org/multipage/links.html#link-type-preload

As @stof pointed out, Chromium-based browsers no longer support Server Push, and although SP is still supported by Firefox and Safari, it can still bring some performance benefits in some cases (if the resource is already cached, the browser will cancel the download, and servers will only trigger the push if conditions are ideal).

AFAIK, all web servers supporting Server Push have a configuration option to disable it necessary.

TL;DR: I wouldn't bother adding this attribute, as it's no longer part of any standard anyway.

Regarding 103 Early Hints, I tried to use them in the original implementation of AssetMapper, unfortunately it's current not possible to use Early Hints to preload files used in importmap: #50476

However, now that the component supports CSS files, it should be possible to use Early Hints to preload CSS files.

@weaverryan
Copy link
Member Author

Thank you for that detailed clarification! I'll close this

However, now that the component supports CSS files, it should be possible to use Early Hints to preload CSS files

Cool :). It won't be for 6.4, but we should think about this for the future.

@weaverryan weaverryan closed this Oct 19, 2023
@weaverryan weaverryan deleted the asset-mapper-link-nopush branch October 19, 2023 11:04
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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.