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.

Sign up for GitHub

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

Jump to bottom

Bug: link order been messed up after upgrade to next version #26705

Closed
lili21 opened this issue Apr 23, 2023 · 11 comments
Closed

Bug: link order been messed up after upgrade to next version #26705

lili21 opened this issue Apr 23, 2023 · 11 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@lili21
Copy link

lili21 commented Apr 23, 2023

the link order inside head element has been messed up after upgrade to next version

React version: 18.3.0-next-c8369527e-20230420

Steps To Reproduce

  1. clone the code example
  2. follow the readme

Link to code example:

https://github.com/lili21/remix-react_next

The current behavior

the link order is

image

The expected behavior

the link order should be

image
@lili21 lili21 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 23, 2023
@lili21
Copy link
Author

lili21 commented Apr 23, 2023

the link order is important cause we want to preload some critical image to decrease the LCP time

@lili21
Copy link
Author

lili21 commented Apr 23, 2023

downgrade the version to 18.2.0, and the order is expected.

@SamarthBagga
Copy link

Hi @lili21 , i am new to open source could i get a chance to work on this issue?

@gaearon
Copy link
Collaborator

gaearon commented May 16, 2023

When you say "messed up" do you mean some tags or missing? Or can you be more specific?

@lili21 lili21 closed this as completed May 17, 2023
@lili21
Copy link
Author

lili21 commented Jun 21, 2023

the order of links is not expected.

@lili21 lili21 reopened this Jun 21, 2023
@lili21
Copy link
Author

lili21 commented Jun 21, 2023

those <link rel=modulepreload /> tags have been moved into head, which should be in body.

@lili21
Copy link
Author

lili21 commented Jun 25, 2023

it's hurting our performance. you can check the similar problem with vitejs/vite#5120.

after upgrade to react@canary version, we see the same effect in production, the FCP increase about 1s. because all the <link rel=modulepreload /> have been moved into head.

@rossipedia
Copy link

@gaearon <link rel="modulepreload" /> tags are being moved to before <link rel="stylesheet" /> tags. Since stylesheets are blocking, if you have a decent amount of modulepreloads then you end up delaying the initial render by however long it takes the browser to free up a network connection to fetch the stylesheet due to all the in-flight preloads.

I think modulepreloads should ideally be moved to after the last stylesheet link tag, which would let the browser fetch the blocking CSS as fast as possible before attempting to preload any JS modules, which can occur asynchronously and won't block the initial paint.

@jrestall
Copy link

Ideally React fixes this by automatically prioritizing stylesheets over modulepreloads and a workaround isn't needed.

However to move the stylesheets above the modulepreloads in the meantime, you can add a precedence: 'high' attribute on stylesheet links from the Remix links function. I don't think this is documented but I learnt it from the React unit tests.

e.g.

  export const links: LinksFunction = () => [
    { rel: 'stylesheet', href: appStylesHref, precedence: 'high' },
  ];

Will result in the stylesheet link being placed at the top of the head element, above the modulepreloads.

  <head>
      <meta charSet="utf-8"/>
      <meta name="viewport" content="width=device-width,initial-scale=1"/>
      <link rel="stylesheet" href="/build/_assets/app-B5PUCUEH.css" data-precedence="high"/>
      <title>New Remix App</title>
      <meta name="description" content="Welcome to Remix!"/>
      <link rel="modulepreload" href="/build/entry.client-4MXBZIKC.js"/>
      <link rel="modulepreload" href="/build/_shared/chunk-PNG5AS42.js"/>
      <link rel="modulepreload" href="/build/root-XKG6C447.js"/>
      <link rel="modulepreload" href="/build/routes/_index-T377ZUPD.js"/>
  </head>

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2024

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2024
@github-actions GitHub Actions
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@rossipedia @gaearon @lili21 @jrestall @SamarthBagga