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

The config arg should not be passed to user-defined loader function in `next/image #35115

Closed
1 task done
addy-pathania opened this issue Mar 7, 2022 · 5 comments · Fixed by #36013
Closed
1 task done
Assignees
Labels
Image (next/image) Related to Next.js Image Optimization.

Comments

@addy-pathania
Copy link

addy-pathania commented Mar 7, 2022

Verify canary release

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

Provide environment information

Operating System:
Platform: win32
Arch: x64
Version: Windows 10 Pro
Binaries:
Node: 14.17.6
npm: 6.14.15
Yarn: 1.22.17
pnpm: N/A
Relevant packages:
next: 12.1.0
react: 17.0.2
react-dom: 17.0.2

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

The bug for next.config.js not considered for node_modeles was discussed here: https://github.com/vercel/next.js/issues/31065

Fixed in PR:https://github.com/vercel/next.js/pull/33559

After the above fix when trying to build the next app the config argument passed in the custom loader function resolves to the default next variables.

Expected Behavior

The config argument in the loader function should return the variables configured in the next.config.js coming from the node_modules when trying to build the app statically.

To Reproduce

  1. Create the next app
  2. The app should use a next.config.js for the 3rd party packages inside node_modules that import next/image(which uses a custom loader function).
  3. Build the app next build

The config argument in the loader function returns default next.config variables set by vercel.

@addy-pathania addy-pathania added the bug Issue was opened via the bug report template. label Mar 7, 2022
@balazsorban44 balazsorban44 added the Image (next/image) Related to Next.js Image Optimization. label Mar 7, 2022
@ambrauer
Copy link

@balazsorban44 Just to provide some additional context / info...

We are creating a next/image wrapper component (to be published in our SDK npm package) which uses a
custom loader. According to docs/discussions, this approach should support build-time optimization (next build and even next export). However, as noted in the description the config values (in particular, we are using path) passed into the custom loader are not pulling from the app's next.config.js. Instead, they are the default values.

This issue does not occur with next dev.

@styfle
Copy link
Member

styfle commented Apr 1, 2022

Hi @ambrauer and @sc-addypathania

The config parameter of the loader() function is undocumented because its only meant to be used by the default loader.

It was changed from a global variable to a function parameter in #33559 as an implementation detail, not as a public API.

Furthermore, this config is meant for the default Image Optimization API. Since the loader prop bypasses the default Image Optimization API in favor of a custom function, those config options should no longer be relevant because the function can implement the optimization url however it wishes.

It sounds like the bug here is that next dev is accidentally leaking implementation details.

For the purposes of your custom loader, it might be best to ask the user for an explicit prefix prop rather than relying on next.config.js (especially if we ever decide to drop the path config option in a future release).

@styfle
Copy link
Member

styfle commented Apr 2, 2022

Here's a concrete example. Consider this Next.js app:

module.exports = {
  images: {
    loader: 'cloudinary',
    path: 'https://res.cloudinary.com/myaccount/'
  }
}
import Image from 'next/image'
import OtherImage from 'other-image'

<Image src="/me.png" width="100" height="100" />

<OtherImage src="/me.png" width="100" height="100" />

The path only makes sense because the loader is cloudinary and that prefix is for cloudinary. It emits something like:

https://res.cloudinary.com/myaccount/q_auto,c_limit,w_100,q_auto/me.jpg

For the other image imported from a package that wraps next/image, it would likely break if it tried to reuse this path. It might emit something that doesn't work with cloudinary, such as:

https://res.cloudinary.com/myaccount/me.jpg?mw=100

@ambrauer
Copy link

ambrauer commented Apr 5, 2022

Hi @styfle, thanks for clearing this up.

We were struggling with the fact that the URL root/path/prefix is defined in the app, while the custom loader is in our SDK. Piggybacking on the default loaders config was a simple solution for us (and made some sense from a DevEx perspective since "path" served the same purpose for us), but your explanation makes sense.

One suggestion beyond aligning the config treatment between next dev and next build: it might also be helpful to exclude the config from the component loader function parameter type (ImageLoaderProps) to avoid confusion. Folks implementing a custom loader will see this and assume they can use it 😊

We will explore other options for providing the root (such as a prefix prop).

Thanks again!

@styfle styfle changed the title Next image custom loader function(arg: config) resolves back to default on static build. The config arg should not be passed to user-defined loader function in `next/image Apr 5, 2022
@styfle styfle added kind: bug and removed bug Issue was opened via the bug report template. labels Apr 5, 2022
ambrauer added a commit to Sitecore/jss that referenced this issue Apr 5, 2022
…ccording to Vercel: vercel/next.js#35115). Added whitelisting config for default next/image sizes.
@styfle styfle self-assigned this Apr 7, 2022
@kodiakhq kodiakhq bot closed this as completed in #36013 Apr 8, 2022
kodiakhq bot pushed a commit that referenced this issue Apr 8, 2022
…age` (#36013)

The `config` was changed from a global variable to a function parameter of the `loader()` function in PR #33559 as an implementation detail, not as a public API. It was not meant to be used by the end user which is why it was [undocumented](https://nextjs.org/docs/api-reference/next/image#loader).

This config is meant for the default Image Optimization API. Since the `loader` prop bypasses the default Image Optimization API in favor of a custom function, that config is no longer be relevant because the function can implement the optimization url however it desires.

- Fixes #35115
colinhacks pushed a commit to colinhacks/next.js that referenced this issue Apr 14, 2022
…age` (vercel#36013)

The `config` was changed from a global variable to a function parameter of the `loader()` function in PR vercel#33559 as an implementation detail, not as a public API. It was not meant to be used by the end user which is why it was [undocumented](https://nextjs.org/docs/api-reference/next/image#loader).

This config is meant for the default Image Optimization API. Since the `loader` prop bypasses the default Image Optimization API in favor of a custom function, that config is no longer be relevant because the function can implement the optimization url however it desires.

- Fixes vercel#35115
addy-pathania added a commit to Sitecore/jss that referenced this issue Apr 14, 2022
* Implement Nextjs Image nextjs package and styleguide

* WIP: Implement nextjs image in nextjs package and styleguide

* Refactored image loader

* Refactor way in which EE markup is applied
in react image component to be reused in next image component.

* WIP: Start refactoring test for nextjs image
also refactor nextjs-styleguide template to use new image component

* added tests and completed todos

* removed unwanted code and added line to end of file

* addressed review comments

* fixed unit tests

* removed dublicate code

* removed extra comments

* updated yarn.lock

* updated yarn.lock, removed dubplicate interface

* removed changes in yarn.lock

* updated yarn.lock

* updated yarn.lock

* fixed linting error

* updated deviceSizes comment in next.config

* improved comments, added utility to get hostname

* removed path and loader from config

* added relative path to loader function

* added comment for path variable

* updated error message, removed extra interface

* fixed undefined path variable issue

* added test cases for the loader function

* renamed next-setup file

* updated next.config comments, refactored redundant code

* added the fix from vercel regarding access to config in loader function

* removed blurDataUrl, made loader function more customizable, fixed tests

* Delete yarn.lock

* added yarn.lock

* updated yarn.lock

* changed next version in template

* removed redundant comments and files , changed error message

* added test case for user custom loader function

* added another test case fro custom loader

* added test for absolute url, mock loader function

* added test for user sent custom loader

* removed params object, added afterEach

* updated the afterEach call

* refactored mock loader tests

* created two describes for next image tests

* updated tests description

* updated the order of tests and describes in next image tests

* Simplified loader which does not depend on 'config' (not to be used according to Vercel: vercel/next.js#35115). Added whitelisting config for default next/image sizes.

* updated image url, fixed classname bug for EE in next/image

* added unit test

* remove console.log

* resolved merge conflicts in yarn.lock file

* refactored util fucntions, removed redundant code

* updated yarn.lock, refactored utils

* removed url-parse

Co-authored-by: CobyPear <coby.sher@sitecore.com>
Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

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 May 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Image (next/image) Related to Next.js Image Optimization.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants