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

[WIP] Next.js Image Component #18122

Closed
21 of 24 tasks
timneutkens opened this issue Oct 22, 2020 · 22 comments
Closed
21 of 24 tasks

[WIP] Next.js Image Component #18122

timneutkens opened this issue Oct 22, 2020 · 22 comments
Assignees

Comments

@timneutkens
Copy link
Member

timneutkens commented Oct 22, 2020

Keeping track of the feedback so far:

@todortotev
Copy link
Contributor

I could lend a hand with these if needed.
Throw error when width and height are missing and unsized is not set
Add err.sh links for all errors

@kevva
Copy link
Contributor

kevva commented Oct 23, 2020

  • Support AMP

@timneutkens
Copy link
Member Author

  • Support AMP

Given the current <img> usage requires replacement with amp-img it's not a goal of the RFC to support AMP initially. Should be relatively straight-forward to add support for it later though given we'll enforce width and height which AMP does as well.

@ItsWendell
Copy link

Great work on this, I've been following the RFC, canary developments and the above pull requests. In the RFC it was mentioned that there should be support for multiple CDNs at the same time.

We're modelling the way we save user-generated images now and notice that just saving the absolute url might not be enough, since then we can't make proper use of the next/image component. Now we're thinking of saving the root url, relative path, dimensions and source / provider (e.g. imgix / cloudinary).

According to the docs, loaders should be defined like this:

module.exports = {
  images: {
    loader: 'imgix',
    path: 'https://example.com/myaccount/',
  },
}

Now what if we are initially using cloudinary, later want to move to imgix or our own solution?

The RFC mentioned the following:

Support for files from multiple sources
It’s not uncommon for an application to feature images served from multiple image hosts. The Image component will support this through an optional host attribute that lets the user specify from one of multiple hosts declared in the next.config.js file. The component will use both the host substring and the loader registered to go along with that host in constructing the URL.

If no host attribute is supplied, the default host and loader will be used

I don't see any (work in process) PR's open for this functionality and it's not clear on how to implement this yet, is someone working on this and is this planned for the coming release?

kodiakhq bot pushed a commit that referenced this issue Oct 24, 2020
Previously, vector images like svg were being converted to webp and resized.

However, vector images already handle any size so we can bypass the same we do for animated images.

Related to #18122
kodiakhq bot pushed a commit that referenced this issue Oct 24, 2020
For many users refactoring from `<img>` to `<Image>`, we can often support their properties as-is.
The exception was width/height.

This PR allows the `<Image>` component to accept strings for `width` and `height` just like `<img>`.

Related to #18122
@timneutkens
Copy link
Member Author

I don't see any (work in process) PR's open for this functionality and it's not clear on how to implement this yet, is someone working on this and is this planned for the coming release?

Support for it was removed for the initial release given that it adds significant complexity in both implementation and configuration. It also caused more code than necessary to be shipped to the browser. We'll address it later if there's enough need for it. In general it's uncommon to be hosting on both imgix and cloudinary at the same time.

kodiakhq bot pushed a commit to vercel/vercel that referenced this issue Oct 24, 2020
We currently pass through `images` whenever its defined, but this is enabling Image Optimization in the Proxy for every Next.js project.

Instead, we should check to see if the default loader is used (the same use for `next dev`) as a signal to enable this feature in the deployment.

Related to vercel/next.js#18122
kodiakhq bot pushed a commit that referenced this issue Oct 24, 2020
kodiakhq bot pushed a commit that referenced this issue Oct 25, 2020
We currently always accept requests to the new `/_next/image` endpoint, even when it should not be used.

Instead, we should check to see if the default loader is used as a signal to enable this API.

Other loaders (such as cloudinary) will not go through the Next.js API so there is no need to expose this, instead we 404.

- Analogous to vercel/vercel#5321
- Related to #18122
@dohomi
Copy link

dohomi commented Oct 26, 2020

I am excited to see the new Image component. I would like to know if there will be an example how to deal with an Image which is part of a 2 column flexrow element (lets say on a grid with 50% or 30% width) on tablet/desktop. But on a mobile its 100% width. Is there any example in place how that should be done?

kodiakhq bot pushed a commit that referenced this issue Oct 26, 2020
…ossible image (#18236)

There was a bug when the user defines a width on the Image component, but a larger size image is requested.

This is because the browser uses the `srcset` to decide which image size to request and we had the srcset basically hardcoded.

This PR fixes the behavior so that the `srcset` will never be larger than the `width` defined on the component.

It also fixes a bug where the preload srcset did not match the img srcset.

- Related to #18147 
- Related to #18122
styfle added a commit that referenced this issue Oct 26, 2020
This adds checks to ensure that less than 50 domain and size items are configured and no sizes are less than 0 or greater than 10,000 

x-ref: #18122

Co-authored-by: Steven <[email protected]>
kodiakhq bot pushed a commit that referenced this issue Oct 26, 2020
This separates the `next.config.js` property `images.sizes` into to properties: `images.deviceSizes` and `images.iconSizes`.

The purpose is for images that are not intended to take up the majority of the viewport.


Related to #18122
@ryanwalters
Copy link

Support for it was removed for the initial release given that it adds significant complexity in both implementation and configuration. It also caused more code than necessary to be shipped to the browser. We'll address it later if there's enough need for it. In general it's uncommon to be hosting on both imgix and cloudinary at the same time.

Does this comment include removing support for a single, custom service loader? In our case, we have an in-house image service that doesn't conform to the path patterns for any of the options I saw in the documentation (default, imgix, cloudinary, akamai).

Is there a way to disable the loader and simply pass the path through to the Image component (maybe something like loader: 'none')? Being able to get the performance benefits of the Image component while building our own path would be great.

@codetheweb
Copy link

Apologies if this is the wrong place to ask, but it doesn't seem like the Image component currently supports optimizing images during the build phase for completely static sites. Is this something that has been considered?

@colepeters
Copy link
Contributor

So excited about this component and all the work around it. Thanks to all involved!

🐛 Hope this isn't the wrong place to mention this, but I just noticed when testing the Image component that src attributes starting with a double slash, e.g. //images.ctfassets.net for Contentful images, seem to be treated as local assets instead of hosted assets. I'm getting around this for now by rewriting // to https://, but I suspect this might not be too deep of an edge case for hosted images 😄

@matamatanot
Copy link
Contributor

matamatanot commented Oct 28, 2020

Is there a way to disable the loader and simply pass the path through to the Image component (maybe something like loader: 'none')? Being able to get the performance benefits of the Image component while building our own path would be great.

Here BUILT IN LOADERS are.

//BUILT IN LOADERS
type LoaderProps = CallLoaderProps & { root: string }
function normalizeSrc(src: string) {
return src[0] === '/' ? src.slice(1) : src
}
function imgixLoader({ root, src, width, quality }: LoaderProps): string {
const params = ['auto=format', 'w=' + width]
let paramsString = ''
if (quality) {
params.push('q=' + quality)
}
if (params.length) {
paramsString = '?' + params.join('&')
}
return `${root}${normalizeSrc(src)}${paramsString}`
}
function akamaiLoader({ root, src, width }: LoaderProps): string {
return `${root}${normalizeSrc(src)}?imwidth=${width}`
}
function cloudinaryLoader({ root, src, width, quality }: LoaderProps): string {
const params = ['f_auto', 'w_' + width]
let paramsString = ''
if (quality) {
params.push('q_' + quality)
}
if (params.length) {
paramsString = params.join(',') + '/'
}
return `${root}${paramsString}${normalizeSrc(src)}`
}
function defaultLoader({ root, src, width, quality }: LoaderProps): string {
if (process.env.NODE_ENV !== 'production') {
const missingValues = []
// these should always be provided but make sure they are
if (!src) missingValues.push('src')
if (!width) missingValues.push('width')
if (missingValues.length > 0) {
throw new Error(
`Next Image Optimization requires ${missingValues.join(
', '
)} to be provided. Make sure you pass them as props to the \`next/image\` component. Received: ${JSON.stringify(
{ src, width, quality }
)}`
)
}
if (src && !src.startsWith('/') && configDomains) {
let parsedSrc: URL
try {
parsedSrc = new URL(src)
} catch (err) {
console.error(err)
throw new Error(
`Failed to parse "${src}" in "next/image", if using relative image it must start with a leading slash "/" or be an absolute URL (http:// or https://)`
)
}
if (!configDomains.includes(parsedSrc.hostname)) {
throw new Error(
`Invalid src prop (${src}) on \`next/image\`, hostname "${parsedSrc.hostname}" is not configured under images in your \`next.config.js\`\n` +
`See more info: https://err.sh/nextjs/next-image-unconfigured-host`
)
}
}
}
return `${root}?url=${encodeURIComponent(src)}&w=${width}&q=${quality || 75}`
}

I feel like these functions are simple.

I want to create a custom Loader func which receives LoaderProps, and set it in next.config.js as loader. When will that be implemented or not?

P.S.
The following is my custom loader. Default loader is a little different than mine.

function myLoader({ root, src, width, quality }: LoaderProps): string {
  const paramsString = `width=?${width}&quality=${quality || 75}`
  return `${root}${normalizeSrc(src)}${paramsString}`
}

Edit
@atcastle already had referred to custom loader. 🙏

#16832 (comment)

Support for self-hosted images
Though the Image component is designed with CDNs in mind, it will also work with locally hosted images. The application developer simply needs to add an image compression/resizing step into their build process, and write a custom loader that generates URLs pointing to the output paths used by that build process.

This shouldn’t be too difficult, as there are a variety of popular build tools for resizing images, and a custom loader can be as simple as a template string.

@styfle
Copy link
Member

styfle commented Oct 28, 2020

The // prefix won't work because the Image Optimization API needs to make the upstream request before optimizing and serving the image. We can't rely on the scheme/protocol because many Node.js servers run as HTTP with a HTTPS proxy in front. If we rely on headers, they can be faked.

It's best to use https:// when referencing external images.

@AlexandraKlein
Copy link

AlexandraKlein commented Oct 28, 2020

So excited about this component and all the work around it. Thanks to all involved!

🐛 Hope this isn't the wrong place to mention this, but I just noticed when testing the Image component that src attributes starting with a double slash, e.g. //images.ctfassets.net for Contentful images, seem to be treated as local assets instead of hosted assets. I'm getting around this for now by rewriting // to https://, but I suspect this might not be too deep of an edge case for hosted images 😄

@colepeters Having trouble with this. What does your next.config.js look like?

Edit: forgot to add domains: ['images.ctfassets.net'],. Works now with prepending https: to src.

@colepeters
Copy link
Contributor

@styfle Just to be clear, the // prefix is being used by Contentful (and possibly other CMSes) as the prefix for URLs for image assets. I agree it'd be best to use https when referencing external images, but at the moment there is some user-level intervention required to do this, at least when referencing images served by Contentful. :)

@AlexandraKlein Here's what I've got:

module.exports = {
  images: {
    domains: [
      'images.ctfassets.net'
    ]
  }
}

What I'm doing is prepending https: to my src when using the Image component.

@AlexandraKlein
Copy link

AlexandraKlein commented Oct 28, 2020

With Contentful, when the image src prop changes within the same react component on route change, Image component isn't swapping out old image for new. @colepeters have you seen this?

Takes a browser reload to see new image.

Though adding a key prop triggers rerender populating correct image.

@colepeters
Copy link
Contributor

@AlexandraKlein Sounds like another case of #18369

@mirshko
Copy link

mirshko commented Oct 28, 2020

I'm wondering if there's plans for a Contentful loader?

Contentful has an Image API similar to Imgix or Cloudinary, wondering it makes sense to use that directly?

@laSinteZ
Copy link

laSinteZ commented Oct 30, 2020

I'm wondering if there are plans to add support for custom loaders? Maybe, for example, user could pass a loader callback to <Image>, which receives LoaderProps and returns image url? I think it will be much easier for users to write their own loaders, than to wait for them to be added to Next.js

Anyway, are there plans for Cloudflire Resize loader? I think I could contribute...

@matamatanot
Copy link
Contributor

@laSinteZ
Check #18450 out.

@Timer Timer assigned styfle, timneutkens and ijjk and unassigned styfle Oct 31, 2020
@Timer Timer closed this as completed Oct 31, 2020
@AlexandraKlein
Copy link

AlexandraKlein commented Nov 10, 2020

@styfle Just to be clear, the // prefix is being used by Contentful (and possibly other CMSes) as the prefix for URLs for image assets. I agree it'd be best to use https when referencing external images, but at the moment there is some user-level intervention required to do this, at least when referencing images served by Contentful. :)

@AlexandraKlein Here's what I've got:

module.exports = {
  images: {
    domains: [
      'images.ctfassets.net'
    ]
  }
}

What I'm doing is prepending https: to my src when using the Image component.

This works fine locally for me but urls give me 400 errors in production with "url" parameter is required response.

Source url with 400 error: https://mywebsite.com/_next/image?url=https%3A%2F%2Fimages.ctfassets.net%2F1xkvhwkgcr51%2F6HODBVSFqUTbEKI7ilXx%2Ff9f16ab3552e973b764c922f06db181a%2Fmy-image.png&w=1200&q=75

Source url locally with no error: http://localhost:3000/_next/image?url=https%3A%2F%2Fimages.ctfassets.net%2F1xkvhwkgcr51%2F6HODBVSFqUTbEKI7ilXx%2Ff9f16ab3552e973b764c922f06db181a%2Fmy-image.png&w=1200&q=75

@AlexandraKlein
Copy link

I'm wondering if there's plans for a Contentful loader?

Contentful has an Image API similar to Imgix or Cloudinary, wondering it makes sense to use that directly?

PR in progress: #19117

ofhouse pushed a commit to milliHQ/terraform-aws-next-js that referenced this issue Mar 13, 2021
We currently pass through `images` whenever its defined, but this is enabling Image Optimization in the Proxy for every Next.js project.

Instead, we should check to see if the default loader is used (the same use for `next dev`) as a signal to enable this feature in the deployment.

Related to vercel/next.js#18122
@Jeffrey-FLS
Copy link

I made a component layer above next-image with Image as the naming component, and have set a function with autocompleting asset location if only filename is provided and am currently getting an error of "using relative image it must start with a leading slash "/"" Is there a way to turn this off? The app won't compile without it. Also, reason for the function is to shorten the length of the Image component by removing paths

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests