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

Added webp preset types and test for same. Issue 3747 #3748

Merged
merged 3 commits into from Aug 4, 2023

Conversation

pilotso11
Copy link
Contributor

For issue #3747.

The webp output options type is missing the preset values in the types. They are present in the js library.
This PR adds:

  • A new enum (PresetEnum)
  • The preset on WebpOptions
  • The enum as sharp.preset on sharp.
  • Tests for both string and enum based use of webp({preset: ...})

This follows the same implementation as other options, such as resize.fit.

npm test-types and npm test-lint both run cleanly.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR, I've left one question inline.

lib/index.d.ts Outdated Show resolved Hide resolved
@lovell
Copy link
Owner

lovell commented Aug 4, 2023

The enum as sharp.preset on sharp.

Did you intend to include this as part of your PR?

@pilotso11
Copy link
Contributor Author

| The enum as sharp.preset on sharp.
I had added this to be consistent with the way other enum's seem to have been exposed. It does allow the following two code styles:

sharp('input.tiff').webp({ preset: 'photo' }).toFile('out.webp');
sharp('input.tiff').webp({ preset: sharp.webpPreset.photo }).toFile('out.webp');

But having taken a closer look, the other exposed enums are there specifically because they are then exposed and overridden in an interface later. So this doesn't really seem necessary. I've removed it and pushed the code again.

@lovell lovell merged commit ffefbd2 into lovell:main Aug 4, 2023
17 checks passed
@lovell
Copy link
Owner

lovell commented Aug 4, 2023

Thank you Seth!

lovell added a commit that referenced this pull request Aug 14, 2023
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.

None yet

2 participants