-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[T-6] Image transformer #4881
[T-6] Image transformer #4881
Conversation
This pull request has been linked to and will mark 1 task as "Done" when merged:
|
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. Three.js x4 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... |
@@ -32,6 +32,7 @@ | |||
"script:*.vue": ["@parcel/transformer-vue"], | |||
"style:*.vue": ["@parcel/transformer-vue"], | |||
"custom:*.vue": ["@parcel/transformer-vue"], | |||
"url:*.{png,jpg,jpeg,webp}": ["@parcel/transformer-image", "..."], |
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.
hmm so now you couldn't use it from CSS/HTML without the url:
prefix either... :/
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.
that's probably why I originally did it without the url:
prefix...
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.
so should I change it back?
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.
Let's get it in first and figure out the url thing later. Seems like we're going to need to treat url:
specially...
: undefined; | ||
let format = asset.query.as ? asset.query.as.toLowerCase().trim() : null; | ||
|
||
let imagePipeline = sharp(inputBuffer); |
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 none of the options are set, should we skip running sharp altogether? Maybe faster?
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.
But the output might be non minified
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.
Hmm it's not an optimizer... I guess we could check that option too. I was more thinking in dev it might be slow/unnecessary.
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.
Yeah... I can fix that in a follow-up PR if you want, sharp isn't always smaller anyway, I've had a couple cases in which it significantly enlarged output images... (on personal/work projects)
↪️ Pull Request
Allow resizing of images using query parameters and sharp
Closes #3737
Closes #3736
Closes #3477
Closes T-6
💻 Examples
🚨 Test instructions
Run kitchen sink example
✔️ PR Todo