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

Add resizing #35

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add resizing #35

wants to merge 15 commits into from

Conversation

sebb0
Copy link

@sebb0 sebb0 commented Feb 8, 2024

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation
  • Other

Description

This pull request adds the ability to resize scalar assets. If no configuration for resizing is passed, no resizing will be performed. If ResizeOptions from sharp are passed as configuration, resizing will be performed and the configuration will be passed to sharp directly.

Resizing is only written to the file if the resulting file size would be lower than the original file size. I think this is a point which could be discussed.

Checks

  • PR adheres to the code style of this project
  • Related issues linked using fixes #number N/A
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Lint and build have passed locally by running pnpm lint && pnpm build
  • Code changes have been verified in local
  • Documentation added/updated if necessary

Additional Context

@sebb0
Copy link
Author

sebb0 commented May 27, 2024

I don't want to sound rude, but is there any feedback here?

@menasheh
Copy link

Does it work? Not sure if the maintainer is active

@sebb0
Copy link
Author

sebb0 commented Jun 17, 2024

Yes it works.

The maintainer last commited 3 weeks ago.

@FatehAK
Copy link
Owner

FatehAK commented Jun 17, 2024

Looks good to me @sebb0. I'll publish a release shortly. Thanks for the patience!

@sebb0
Copy link
Author

sebb0 commented Jun 17, 2024

@FatehAK thank you!

@FatehAK
Copy link
Owner

FatehAK commented Jun 17, 2024

Resizing is only written to the file if the resulting file size would be lower than the original file size. I think this is a point which could be discussed.

For the above case, do you reckon we will face issues if say user is resizing the image larger than the original @sebb0 ?

@sebb0
Copy link
Author

sebb0 commented Jun 17, 2024

Resizing is only written to the file if the resulting file size would be lower than the original file size. I think this is a point which could be discussed.

For the above case, do you reckon we will face issues if say user is resizing the image larger than the original @sebb0 ?

In my opinion, the purpose of this plugin is to optimise image assets, mainly to save bandwidth.

I would simply want to use my high resolution images in a project and reduce them to a certain image size (plus all the other optimisations in this plugin) to save bandwidth. I don't care about the image size if the storage size of the image is smaller before passing through the plugin.
I have implemented this according to my needs.

If you think that users of this plugin want to enlarge image sizes and also want to be able to rely on this enlargement happening, then we probably need to tweak this.

@FatehAK
Copy link
Owner

FatehAK commented Jun 17, 2024

I have a quick question regarding this matter. If you pass the resize options as { width: 600, height: 700 } and you have images of varying dimensions, wouldn't all images be resized to exactly 600x700 pixels? Is that the expected behaviour you want?

Also, I developed this plugin with the primary goal of optimizing images rather than transforming them. Mixing these together makes things super messy. There should be other plugins available that handle image transformation as a preliminary step before this optimizer plugin runs as part of Vite's build pipeline.

@sebb0

@FatehAK
Copy link
Owner

FatehAK commented Jun 26, 2024

@sebb0 awaiting your response for the above query before I go ahead with the merge. Thanks again!

@sebb0
Copy link
Author

sebb0 commented Jun 28, 2024

The whole thing happened so long ago that I had to take some time to familiarise myself with the subject again.

I've thought it all over again and don't really see any problems. Your original concerns regarding the enlargement of assets can be dispelled with the [options.withoutEnlargement] parameter from the resize function of sharpen.js.

There are also suitable parameters for preserving the aspect ratios of the images. If I understood you correctly, that was another of your concerns. The default behaviour is to keep the aspect ratio.

Unfortunately, I cannot dispel your concern about mixing two functionalities that you believe should not be mixed. But in the end it's your project and your decision. 😀

@caleb531
Copy link

caleb531 commented Jul 1, 2024

Just chiming in here—I would just like to mention that the publishing of this resize functionality would enable me to switch from vite-imagetools and use this plugin for all image optimization in my project.

Right now, I'm using vite-imagetools for resizing JPEGs, and vite-plugin-image-optimizer for optimizing SVGs. But I do think it would be great to have both handled by the same plugin (and without any benchmarks to prove it, could potentially minimize overhead in building my Vite project).

And regarding the separation of concerns matter: I'm of the mindset that resizing is a form of image optimization (maybe not other types of image transformations, but I think resizing definitely counts). Because the end goal of optimization is to cut down of file size, which image resizing expressly does. Moreover, I think the practicality of having this (also given that it's a relatively small amount of code) would sufficiently outweigh any drawbacks regarding principle.

My only question would be: does this PR allow for image resizing on a per-import basis? Because different images in my project need to be resized to different sizes. Vite-imagetools handles this through query params on the imported path, e.g.:

import myImage from './src/images/my-image.jpg?w=320';
import myImageRetina from './src/images/my-image.jpg?w=640';

@sebb0
Copy link
Author

sebb0 commented Jul 2, 2024

My only question would be: does this PR allow for image resizing on a per-import basis?

No it was not in my scope. But personally, I like this idea.

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

Successfully merging this pull request may close these issues.

4 participants