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

Image transformer #4209

Closed
wants to merge 21 commits into from
Closed

Image transformer #4209

wants to merge 21 commits into from

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Feb 23, 2020

↪️ Pull Request

Allow resizing of images using query parameters and sharp

Closes #3737
Closes #3736
Closes #3477

Closes T-6

💻 Examples

import image from 'url:./image.jpg?height=200';
import image from 'url:./image.webp?height=200&width=300';
import image from 'url:./image.png?width=300';

🚨 Test instructions

Run kitchen sink example

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@parcel-benchmark
Copy link

parcel-benchmark commented Feb 23, 2020

Benchmark Results

packages/benchmarks/three ✅

Timings

Description Time Difference
Cold 1.37m +7.00ms
Cached 4.56s -279.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/three/entry.js 2.68mb +0.00b 65.00ms -26.00ms 🚀

packages/benchmarks/kitchen-sink ✅

Timings

Description Time Difference
Cold 7.20s +701.00ms ⚠️
Cached 3.20s -175.00ms 🚀

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.d5807e82.webp 102.94kb -12.74kb 🚀 167.00ms -105.00ms 🚀
dist/modern/parcel.d5807e82.webp 102.94kb -12.74kb 🚀 287.00ms +113.00ms ⚠️
dist/legacy/kitchen-sink.a00e2ace.js 1.21kb +0.00b 937.00ms -123.00ms 🚀
dist/legacy/index.html 709.00b +0.00b 923.00ms -141.00ms 🚀
dist/legacy/styles.27b3555b.css 29.00b +0.00b 1.51s -104.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.d5807e82.webp 102.94kb -12.74kb 🚀 5.00ms -0.00ms
dist/modern/parcel.d5807e82.webp 102.94kb -12.74kb 🚀 3.00ms -0.00ms
dist/modern/kitchen-sink.19a3eecb.js 1.21kb +0.00b 3.00ms -1.00ms 🚀
dist/legacy/index.html 709.00b +0.00b 10.00ms -1.00ms 🚀
dist/modern/index.html 709.00b +0.00b 4.00ms -1.00ms 🚀

packages/benchmarks/react-hn ✅

Timings

Description Time Difference
Cold 9.37s -526.00ms 🚀
Cached 3.50s +272.00ms ⚠️

Cold Bundles

Bundle Size Difference Time Difference
dist/main/index.js 66.50kb +0.00b 1.84s -118.00ms 🚀
dist/module/index.js 37.46kb +0.00b 1.84s -117.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/main/index.js 66.50kb +0.00b 10.00ms +3.00ms ⚠️

packages/benchmarks/ak-editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Click here to view a detailed benchmark overview.

@@ -87,6 +88,10 @@ class BaseAsset {
return this.#asset.value.filePath;
}

get query(): QueryParameters {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should transformers be able to add additional data to the query?

Only example I can think of is html-transformer that would add width and height from srcset to the image asset, but that's probably something with addDependency anyway

@@ -32,6 +35,26 @@ type Module = {|
filePath?: FilePath,
|};

const parseFilePath = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely sure if this is the correct place for this.

I do believe we should pass queryparams to the resolver, in case it should resolve to some remote asset like deno does for example... But maybe we should make the resolverrunner handle this and pass it as an argument to each resolver so they can opt in to caring about it or not?

@DeMoorJasper DeMoorJasper mentioned this pull request Feb 27, 2020
3 tasks
@DeMoorJasper DeMoorJasper changed the title Image resize transformer Image transformer Feb 27, 2020
@DeMoorJasper DeMoorJasper marked this pull request as ready for review February 27, 2020 18:43
let dirContent = await outputFS.readdir(distDir);
console.log(dirContent);

assert.equal(dirContent.length, 8);
Copy link
Member Author

Choose a reason for hiding this comment

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

This should output an image for each format, it just outputs one.
I tried updating the key but it does not seem to work. Not entirely sure how I should fix this. I thought it might be a bundling thing but the assets should be isolated?

@mischnic
Copy link
Member

I'm not entirely sure if this is already the case, but I think query params should generally only be allowed when explicitly specifying a pipeline (for the same reason that we went with a protocol-like syntax for pipelines instead of query params).

@DeMoorJasper
Copy link
Member Author

@mischnic Maybe I misunderstood but I don't think it should be explicitly specified as a pipeline (in parcelrc). That would result in a lot of pipeline config (I think)?

@height
Copy link

height bot commented Mar 14, 2020

This pull request has been linked to and will mark 1 task as "Done" when merged:

  • T-6 Support query params to pass options to transformers (unlink task)

💡Tip: You can link multiple Height tasks to a pull request.

@DeMoorJasper
Copy link
Member Author

Gonna rework this... Too much conflicts at this point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parcel 2: Image Resizing Transformer Parcel 2: WebP Transformer [RFC] Import query params
5 participants