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

Allow wildcarded domains for image optimization #27345

Conversation

FDiskas
Copy link

@FDiskas FDiskas commented Jul 20, 2021

Add ability to use wildcard in configuration of allowed list of domains.

Bug

Related PR:

fixes #18412 #23902 #18632 #27925

  • Related issues:
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

Requests:

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes

@@ -3,6 +3,7 @@ import { createHash } from 'crypto'
import { createReadStream, promises } from 'fs'
import { getOrientation, Orientation } from 'get-orientation'
import { IncomingMessage, ServerResponse } from 'http'
import micromatch from 'micromatch'
Copy link
Member

Choose a reason for hiding this comment

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

We also need to validate on the client in packages/next/client/image.tsx

I think #18730 is a better implementation because it doesn't rely on a 3rd party package that would need to be bundled in the client.

Copy link
Author

@FDiskas FDiskas Jul 22, 2021

Choose a reason for hiding this comment

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

I think that special modules does his job better then that pr. The module has a lot of tests and it works. Pr has unknown status. What if I add *.staging.*.com or **.google.com to the config list? Will it work?

Copy link
Author

Choose a reason for hiding this comment

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

In any case we need that feature a lot. That image resizing is super awesome. And there is so many cdn's with random sub domains.

Copy link
Member

Choose a reason for hiding this comment

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

What if I add .staging..com or **.google.com to the config list?

These are great questions which is why its better to have our own implementation, so we can test and document the usage.

Copy link
Author

Choose a reason for hiding this comment

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

Should I continue on this PR - add some more tests and so on?

Copy link
Member

Choose a reason for hiding this comment

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

If so, we would need to drop micromatch.

I'm skeptical if we even need to differentiate ** vs * but willing to hear out any use cases

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, maybe we do need ** because wildcard certs have a single * so we should match that behavior and then ** could mean any number of subdomains.

We could even introduce this in separate phases so that only ** is supported at first which can be implemented with a simple url.hostname.endsWith(suffix). Then we can add support for single * in a separate PR.

Copy link

Choose a reason for hiding this comment

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

@FDiskas @styfle is the decision around support for ** vs * the blocker on merging this PR?

Copy link
Member

Choose a reason for hiding this comment

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

The blocker is micromatch. It shouldn't use an external dependency since we need to document behavior around ** vs *. See #27925

AbdulrhmnGhanem added a commit to kitspace/kitspace-v2 that referenced this pull request Oct 13, 2021
- Add `gitea.${KITSPACE_DOMAIN}, and `processor.${KITSAPCE_DOMAIN} for
  nextjs image optimization allowed domains.
	- There is no wildcards support yet, see vercel/next.js#27345
- Remove ISR environment variables as they are no longer used.
ianconsolata added a commit to mysilio-co/garden that referenced this pull request Oct 20, 2021
@413n
Copy link

413n commented Oct 21, 2021

Any update?

@FDiskas
Copy link
Author

FDiskas commented Oct 21, 2021

If it's still relevant I can take a look. There was similar PR without introduced dependency.
#18730

…/allow-wildcard-domainas-for-image-optimization
@FDiskas FDiskas requested a review from molebox as a code owner November 4, 2021 10:54
…/allow-wildcard-domainas-for-image-optimization
@FDiskas
Copy link
Author

FDiskas commented Nov 4, 2021

I see that somebody already added micromatch as a dev dependency. So that means we are good to go?
@styfle any thoughts?

@FDiskas FDiskas requested a review from styfle November 4, 2021 11:22
@fizzterious

This comment has been minimized.

@jericopulvera

This comment has been minimized.

@samstr

This comment has been minimized.

@joelamltc
Copy link

Any updates on this PR? This feature is really important that if we are loading external CDN images.

@FDiskas
Copy link
Author

FDiskas commented Dec 23, 2021

I don't know what should I do to make it happen.

AbdulrhmnGhanem added a commit to kitspace/kitspace-v2 that referenced this pull request Dec 25, 2021
- Add `gitea.${KITSPACE_DOMAIN}, and `processor.${KITSAPCE_DOMAIN} for
  nextjs image optimization allowed domains.
	- There is no wildcards support yet, see vercel/next.js#27345
- Remove ISR environment variables as they are no longer used.
@RebootGG
Copy link

RebootGG commented Feb 1, 2022

Could we please have an update from the Vercel team ?

The app we are developing displays images from dynamic external hosts. Unless we have a solution, I might be forced to use a regular <img> tag.

@devin-aegis-studio
Copy link

Bump... what's going on with this?

@FDiskas
Copy link
Author

FDiskas commented Feb 11, 2022

As far as I understand this will not be merged untill it will introduce a new dependency. I should update it and make it work with custom glob match.

@borispoehland
Copy link

Just here to say how urgent this is needed. Thanks ❤️

@bmstefanski
Copy link

Just here to say how urgent this is needed. Thanks ❤️

I recently posted a workaround here, #18429 (comment) . It has some cons, especially in big scale projects, but for most apps, it should do the job

cc'ing @borispoehland, @joelamltc, @samstr, @jericopulvera, @fizzterious, @afnar

@Rishab1207
Copy link

any further updates on this?

@styfle
Copy link
Member

styfle commented Apr 5, 2022

Yes, take a look at the proposal here #27925

@FDiskas
Copy link
Author

FDiskas commented Apr 14, 2022

I have a concept that could actually work

let match = 'https://**.google.*';

match = match.replace(/[$()+.?[\\\]^{|}]/g, '\\$&');
match = match.replace(/\*\*/g, '([^/]+/<asterisk>)+');
match = match.replace(/\*/g, '([^./]<asterisk>)');
match = match.replace(/<asterisk>/g, '*');

console.log('https://www.google.com'.match(match)); // true
console.log('https://buzz.www.google.com'.match(match)); // true
console.log('https://foo.buzz.www.google.com'.match(match)); // true
console.log('https://google.com'.match(match)); // false

@kodiakhq kodiakhq bot closed this in #36245 May 5, 2022
kodiakhq bot pushed a commit that referenced this pull request May 5, 2022
…#36245)

## Description 
This PR implements a new configuration object in `next.config.js` called `experimental.images.remotePatterns`.

This will eventually deprecate `images.domains` because it covers the same use cases and more by allowing wildcard pattern matching on `hostname` and `pathname` and also allows restricting `protocol` and `port`.

## Feature

- [x] Implements an existing feature request.
- [x] Related issues linked
- [x] Unit tests added
- [x] Integration tests added
- [x] Documentation added
- [x] Telemetry added. In case of a feature if it's used or not.
- [x] Errors have helpful link attached, see `contributing.md`

## Related 

- Fixes #27925 
- Closes #18429 
- Closes #18632
- Closes #18730
- Closes #27345
@styfle
Copy link
Member

styfle commented May 6, 2022

This feature is available on the canary channel npm i next@canary using the experimental remotePatterns config.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image optimisation external resource not working