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

feat(templates): add Link header for Early Hints, HTTP2 Server Push etc. #3200

Closed
wants to merge 1 commit into from

Conversation

Plopix
Copy link

@Plopix Plopix commented May 15, 2022

Hello,

Context

When using HTTP/2 we can optimize the first loading with HTTP/2 Server Push Link header.
I had to implement it for a project and I wonder if it might cool to have it in Remix directly.

Probably not be the best place to put this code but at least this PR open the topic, and make it simple to test ;)
Should we have that in Remix by default? or with a configuration?

What do you think?

Testing with HTTPS and HTTP2 locally

  • install Caddy Server
  • install mkcert and generate certificates

Then here a simple Caddyfile

my.custom.domain.com {
    tls ./provisioning/dev/certs/domains.pem ./provisioning/dev/certs/key.pem

    push
    
    @websockets {
        header Connection *Upgrade*
        header Upgrade websocket
    }
    # (3019 is custom, change for you)
    reverse_proxy @websockets localhost:3019
    
    # (3018 is custom, change for you)
    reverse_proxy 127.0.0.1:3018
}

Results

Screen Shot 2022-05-15 at 12 44 52 PM

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 15, 2022

Hi @Plopix,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 15, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@machour
Copy link
Collaborator

machour commented May 16, 2022

Hi @Plopix and thank you for opening this PR!
Can you target the dev branch instead as you're changing code?

@machour machour added the needs-response We need a response from the original author about this issue/PR label May 16, 2022
@Plopix Plopix changed the base branch from main to dev May 16, 2022 15:14
@Plopix
Copy link
Author

Plopix commented May 16, 2022

Hello @machour, it's done!

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 16, 2022
@MichaelDeBoey MichaelDeBoey changed the title Add HTTP/2 Server Push Link header feat(templates): add HTTP/2 Server Push link header May 16, 2022
@sergiodxa
Copy link
Member

I like this idea, but I have a question, in daffy.org we import multiple copies of the same image in different sizes to support different pixel densities and for desktop and mobile, I then preload them using a link tag which supports imagesrcset and media for media queries.

Will this change send Link header to preload all of my images? Even the ones I don’t really need? Because right now an iPhone user for example only loader one of those images (mobile an 3x size) and not the rest, but Link header doesn’t support srcSet and media queries to conditionally load images.

@Plopix
Copy link
Author

Plopix commented May 17, 2022

thanks @MichaelDeBoey much cleaner ;-) changed and pushed!

@sergiodxa the first piece of code will add the list of modules and imports (basically just the chunks).

see my screenshot

The second line answers "a bit more" your question:

responseHeaders.set("Link", (responseHeaders.has("Link") ? responseHeaders.get("Link") + "," : '') + http2PushLinksHeaders.map((link: string) => `<${link}>; rel=preload; as=script`).join(","));

If a Link header already exists then we add to it.
In my screenshot, the tailwind CSS is using push because in my root.tsx I have:

export const headers: HeadersFunction = () => {
    return {
        Link: `<${tailwindStyles}>; rel=preload; as=style`,
    };
};

So to really answer your question the Link Tag is not related actually.

The preload value of the element's rel attribute lets you declare fetch requests in the HTML's , specifying resources that your page will need very soon, which you want to start loading early in the page lifecycle, before browsers' main rendering machinery kicks in. This ensures they are available earlier and are less likely to block the page's render, improving performance.

Here we the Link header is a HTTP/2 feature. That pushes the resources directly into the response, there is no roundtrip like with preload

Conclusion: it should not impact you ;-)

@Plopix Plopix requested a review from MichaelDeBoey May 17, 2022 00:24
@MichaelDeBoey MichaelDeBoey requested review from ryanflorence, mjackson, mcansh, chaance and MichaelDeBoey and removed request for MichaelDeBoey May 17, 2022 10:12
@Plopix
Copy link
Author

Plopix commented May 18, 2022

I think we should remove the <link rel="modulepreload" href="/build/_shared/chunk-XXXX.js" /> from the rendered page.
I don't really know the internal of Remix but that must be there: https://github.com/remix-run/remix/blob/main/packages/remix-react/components.tsx#L633 ping @MichaelDeBoey

Checking the header maybe, but there is something.

@Plopix
Copy link
Author

Plopix commented May 25, 2022

btw @mjackson or @ryanflorence do you want to talk about that PR (like 10-15min) after the conf or tomorrow morning?

@sergiodxa
Copy link
Member

I think we should remove the <link rel="modulepreload" href="/build/_shared/chunk-XXXX.js" /> from the rendered page. I don't really know the internal of Remix but that must be there: https://github.com/remix-run/remix/blob/main/packages/remix-react/components.tsx#L633 ping @MichaelDeBoey

Checking the header maybe, but there is something.

I think keeping them should not affect, right? The browser will already have the cached file so it will not request it again, and if for some reason the browser didn't downloaded the files from the header it has another opportunity to do it before actually needing the file.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jan 2, 2023
@sergiodxa
Copy link
Member

Hey! FYI this code no longer works in v1.10.0. I haven't found a solution yet unfortunately.

😉 mcansh/mcan.sh@934a7b1

that being said, maybe this is something we can do OOTB in remix itself? anyone want to open a proposal?

I initially thought Remix could do this OOTB but now I think this should come in the template or be a function (I’m planning to add it to Remix Utils), the reason is because if the developer decides to remove the Scripts component this will push the JS files but nothing will use them.


Another thing is that this same technique can be used to know the linked assets and push them automatically, I was able to do it in my personal site to preload CSS and other assets https://github.com/sergiodxa/sergiodxa.com/blob/bf3ecf401b7e1406044d9480129cb5dd3063d0c0/app/entry.server.tsx#L53

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Mar 9, 2023

v1.14.0 removed the need to have entry files & we removed them from our templates as well.

@Plopix Could you rebase your PR onto latest dev & apply changes to the default entry file instead?

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2023

⚠️ No Changeset found

Latest commit: a02154a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

@Plopix You should update the files in remix-dev/config/defaults instead

@Plopix
Copy link
Author

Plopix commented Mar 9, 2023

ok will do !

@Plopix
Copy link
Author

Plopix commented Mar 9, 2023

I wonder if entry.server.deno and entry.server.cloudflare deserve the same change ?

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Mar 9, 2023

@Plopix I would say they do

You shouldn't change files in templates/remix though, as those will be removed once we merge main into dev (they're already gone on main, see #5455)

@chaance chaance removed their request for review March 9, 2023 15:41
@kiliman
Copy link
Collaborator

kiliman commented Mar 9, 2023

Hmm... my understanding is that HTTP2 Server Push is being deprecated. Chrome is removing support for it in an upcoming release. Apparently, there are not enough implementations on the server side to make it worthwhile to maintain support.

So not sure this is worth adding to the default templates.

https://developer.chrome.com/blog/removing-push/

@Plopix
Copy link
Author

Plopix commented Mar 13, 2023

yes that's true, the new stuff to look for is probably: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/103
Already discussed here: #5378

@MichaelDeBoey
Copy link
Member

Hi @Plopix!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023
@real34
Copy link
Contributor

real34 commented May 11, 2023

Hi everyone!

Depending on @Plopix availability for this, I'm volunteering to help getting it merged if needed.
It looks like most of the work is done, and that would be a great addition to Remix.

Please let me know if help is needed here.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 11, 2023
@Plopix
Copy link
Author

Plopix commented May 12, 2023

I am a bit confused actually here. Where should I do changes finally? At some point in this PR I have been told to do not change anything in templates/ as the files will be removed.
is that still the case ?
entry.client and entry.server seems to be back in main and dev.

@anaibol
Copy link

anaibol commented Jul 14, 2023

THIS IS AMAZING!

@brophdawg11
Copy link
Contributor

I'm going to close this out due to #3200 (comment) and because we have an open proposal that will go through our Open Develoment Process in #5378.

Also because this can be done today in userland in your entry.server.tsx file.

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.

10 participants