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

[NEXT-811] [appDir] next/image with priority doesn't generate <link rel="preload" ...> tag. #43134

Closed
1 task done
kvnang opened this issue Nov 19, 2022 · 17 comments · Fixed by #52425
Closed
1 task done
Assignees
Labels
area: app App directory (appDir: true) Image (next/image) Related to Next.js Image Optimization. locked Metadata Related to Next.js' Metadata API.

Comments

@kvnang
Copy link
Contributor

kvnang commented Nov 19, 2022

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
Platform: darwin
Arch: x64
Version: Darwin Kernel Version 22.1.0: Sun Oct 9 20:14:54 PDT 2022; root:xnu-8792.41.9~2/RELEASE_X86_64
Binaries:
Node: 18.10.0
npm: 8.19.2
Yarn: 1.22.19
pnpm: 7.13.0
Relevant packages:
next: 13.0.5-canary.2
eslint-config-next: 13.0.4
react: 18.2.0
react-dom: 18.2.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

When using pages directory, the <Image /> component from next/image with priority prop will generate a <link rel="preload" as="image" ... /> on the <head />. But this is not the case when using app directory.

I believe this is because, currently, the <link> tag is injected using next/head on a client component (reference). This doesn't work given the server component setup.

Expected Behavior

<link rel="preload" as="image" ... /> will also be generated for priority <Image /> when using app directory.

Link to reproduction - Issues with a link to complete (but minimal) reproduction code will be addressed faster

https://github.com/kvnang/next-image-preload

To Reproduce

git clone https://github.com/kvnang/next-image-preload
cd next-image-preload
npm run build && npm run start

Under /app/page.tsx, the <Image /> component with src vercel.svg has a priority prop set to true, and thus should produce <link rel="preload" /> tag. But it doesn't in this case.

NEXT-811

@kvnang kvnang added the bug Issue was opened via the bug report template. label Nov 19, 2022
@kvnang
Copy link
Contributor Author

kvnang commented Nov 20, 2022

Possible solutions I have been exploring:

  • Using ReactDOM's float preload() (like in <Script /> component), but it doesn't support certain attributes like imgsrcset, for example.
  • Making a server component version of <Image /> component, and just inlining <link> tag, which will be picked up by the <head>. But this seems quite destructive ...

@transitive-bullshit
Copy link

I'm seeing this as well, though I'm also seeing this with the new next/image when reverting to use pages/.

@balazsorban44 balazsorban44 added area: app App directory (appDir: true) Image (next/image) Related to Next.js Image Optimization. Metadata Related to Next.js' Metadata API. labels Nov 21, 2022
@balazsorban44
Copy link
Member

Thanks, this is currently expected, as head handling is still a work in progress: https://beta.nextjs.org/docs/app-directory-roadmap#supported-and-planned-features, which next/image relies on to push a preload tag to the DOM.

{priority ? (
// Note how we omit the `href` attribute, as it would only be relevant
// for browsers that do not support `imagesrcset`, and in those cases
// it would likely cause the incorrect image to be preloaded.
//
// https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset
<Head>

@transitive-bullshit, I could not reproduce it with pages/, are you sure you are using priority? If so, could you link to a reproduction, as that might actually be considered a bug.

@balazsorban44 balazsorban44 removed the bug Issue was opened via the bug report template. label Nov 21, 2022
@sckimynwa
Copy link

sckimynwa commented Dec 26, 2022

Same Issue Here.
I'm using vercels official next 13 demo which uses next 13.0.7-canary.4 as a dependency

in this demo, I created new page and add Client Component to Use next/image

image

import Test from './child.client';

export default async function Page() {
  return (
    <div className="space-y-8">
      <div className="space-y-4">
        <h1 className="text-xl font-medium text-gray-400/80">
          Next Image Test
        </h1>
        <Test />
      </div>
    </div>
  );
}

// child.client.tsx
import Image from 'next/image';

const Test = () => {
  return (
    <div className="space-y-2">
      <div className="flex space-x-2">
        <Image
          src="/prince-akachi-LWkFHEGpleE-unsplash.jpg"
          className="rounded-full"
          width={300}
          height={300}
          alt="User"
        />
        <Image
          priority
          src="https://images.pexels.com/photos/45201/kitty-cat-kitten-pet-45201.jpeg?cs=srgb&dl=pexels-pixabay-45201.jpg&fm=jpg"
          className="rounded-full"
          width={300}
          height={300}
          alt="User"
        />
      </div>
    </div>
  );
};

export default Test;

in the second image, with priority props, next should generate link tag with preload as image, but nothing happend in the document's head tag. I think injecting the preload tag is not working in thie version 🤔

image

@michaelyang
Copy link

michaelyang commented Jan 16, 2023

Same issue here, not seeing the preload being generated. The priority prop seems to be only be removing the lazy loading portion, even though the doc says it should also preload the image.

@steve-marmalade
Copy link

@balazsorban44 , regarding:

Thanks, this is currently expected, as head handling is still a work in progress: https://beta.nextjs.org/docs/app-directory-roadmap#supported-and-planned-features, which next/image relies on to push a preload tag to the DOM.

Has anything changed in this regard with all of the effort around head.js / metadata that went into 13.2 ?

@styfle styfle changed the title [appDir] next/image with priority doesn't generate <link rel="preload" ...> tag. [NEXT-811] [appDir] next/image with priority doesn't generate <link rel="preload" ...> tag. Mar 10, 2023
@styfle styfle self-assigned this Mar 10, 2023
@steve-marmalade
Copy link

The latest Beta docs now include a section on Resource hints, and it looks like support for next/image should be added automatically for client components:

These methods are currently only supported in Client components.
Next.js in-built features such as next/font, next/image and next/script automatically handle relevant resource hints.

However, when I updated to 13.3 I am not seeing these tags in the <head> of the document.

Should I create a separate repro for this? Or should we be setting these manually as shown in e.g. this comment?

@styfle
Copy link
Member

styfle commented Apr 14, 2023

This issue is that browsers like Safari that don't support imagesrcset will fallback to href.

// Note how we omit the `href` attribute, as it would only be relevant
// for browsers that do not support `imagesrcset`, and in those cases
// it would likely cause the incorrect image to be preloaded.
//
// https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset

The new ReactDOM.preload() requires a href and that could cause Safari to be slower by preloading the wrong image, so we need a better solution.

We'll likely need to fix this in React upstream first.

@leo-cheron
Copy link

App dir is now Stable but this issues remains on next 13.4.1. It drastically cripples LCP performances :(

@ghost
Copy link

ghost commented May 12, 2023

Also an issue here, lcp image has fetchpriority but no preload tag is generated.

@Jarpiino
Copy link

still apparent in 13.4.5 :(

@markomitranic
Copy link

markomitranic commented Jun 21, 2023

This issue is that browsers like Safari that don't support imagesrcset will fallback to href.

// Note how we omit the `href` attribute, as it would only be relevant
// for browsers that do not support `imagesrcset`, and in those cases
// it would likely cause the incorrect image to be preloaded.
//
// https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset

The new ReactDOM.preload() requires a href and that could cause Safari to be slower by preloading the wrong image, so we need a better solution.

We'll likely need to fix this in React upstream first.

Would be nice to just simply not support srcsets until its fixed - instead of omitting the whole feature. But thats not a next feature, its a react one...

Until then, i live with the errors, and use:

    ReactDOM.preload(url, { as: "image" as "script" });

@gauravsaini964
Copy link

Any ETA on when this fix is landing in canary? I see huge drop in LCP performance ~1 second.

@Jarpiino
Copy link

Have you tried to statically import your images?
import sample_image from "public/images/sample_image.jpg"
And then applying the priority=true?

@jep-a
Copy link

jep-a commented Jul 13, 2023

Looks like if you need it now, you can update to 13.4.9 and use getImgProps from #51205 and use that to get the generated srcset and sizes from your image and feed it into ReactDOM.preload

Just wrote this without testing, but this is a simplified version of what I did in real code

import Image, { unstable_getImgProps as getImgProps } from 'next/image';
import { preload } from 'react-dom';

const ImagePreload = ({imgProps}) => {
  const preloadImgProps = getImgProps(imgProps);
  
  preload(preloadImgProps.props.src, {
    as: 'image',
    imageSrcSet: preloadImgProps.props.srcSet,
    imageSizes: imgProps.sizes
  });
  
  return (
    <Image {...imgProps} />
  );
};

But looks like its almost fixed anyways with this PR #52425

@kodiakhq kodiakhq bot closed this as completed in #52425 Jul 14, 2023
kodiakhq bot pushed a commit that referenced this issue Jul 14, 2023
@gauravsaini964
Copy link

Please check #52995 .

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) Image (next/image) Related to Next.js Image Optimization. locked Metadata Related to Next.js' Metadata API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.