-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow wildcarded domains for image optimization #27345
Closed
FDiskas
wants to merge
15
commits into
vercel:canary
from
FDiskas:feature/allow-wildcard-domainas-for-image-optimization
Closed
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
433b51b
Allow wildcarded domains for image optimization
FDiskas 6782cec
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas a1c960a
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 055e08b
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 726e5ca
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 03b329c
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 4aed40a
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 63c572b
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 8417dbd
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 8ea2b1d
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas c1c4678
Merge branch 'canary' of git://github.com/vercel/next.js into feature…
FDiskas a373763
Merge branch 'canary' of git://github.com/vercel/next.js into feature…
FDiskas 625e2cf
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 11d26f7
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas 50940c6
Merge branch 'canary' into feature/allow-wildcard-domainas-for-image-…
FDiskas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great questions which is why its better to have our own implementation, so we can test and document the usage.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 casesThere was a problem hiding this comment.
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 simpleurl.hostname.endsWith(suffix)
. Then we can add support for single*
in a separate PR.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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