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

fix: Fix empty toDataURL() in TypeScript #672

Merged
merged 6 commits into from
Oct 31, 2022

Conversation

indianakernick
Copy link
Contributor

@indianakernick indianakernick commented Oct 31, 2022

The recent change to the toDataURL function made the type parameter required. It was previously optional. This is a breaking change and most likely an innocent mistake.

Additionally, the type of the encoderOptions parameter for the second overload was corrected. The type of type could be string and its value could be 'image/svg+xml'. So ToSVGOptions should be accepted by the encoderOptions parameter. This is not technically a breaking change but it's something I noticed while I was in the area.

@indianakernick indianakernick changed the title Fix regression introduced by release 4.1.0 Fix breaking change to toDataURL type signature introduced by release 4.1.0 Oct 31, 2022
@UziTech
Copy link
Collaborator

UziTech commented Oct 31, 2022

The type of type could be string and its value could be 'image/svg+xml'. So ToSVGOptions should be accepted by the encoderOptions parameter.

The whole point of the overload is so the encoderOptions parameter can be ToSvgOptions only when the type is 'image/svg+xml'

Copy link
Collaborator

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

Can you add a test for toDataURL without a type parameter so this doesn't break again?

@indianakernick
Copy link
Contributor Author

indianakernick commented Oct 31, 2022

Can you add a test for toDataURL without a type parameter so this doesn't break again?

Added.

The whole point of the overload is so the encoderOptions parameter can be ToSvgOptions only when the type is 'image/svg+xml'

Yeah. I understand the intent. When type is 'image/svg+xml', encoderOptions must be ToSvgOptions. When it is not 'image/svg+xml', it must be number. It's a bit difficult to express that second part in TypeScript currently. There's no way to say "any string except for 'image/svg+xml'". Because of this, the overload set doesn't really work. For example, this compiles: toDataURL('image/svg+xml', 1). In this case, the second overload is chosen.

As it is defined with and without the changes in this PR, toDataURL doesn't fully enforce correct usage at compile-time. However, the first overload still expresses intent. It still acts as a form of documentation. When I type toDataURL('image/, the IDE suggests that I use 'image/svg+xml' and pass ToSvgOptions as the second argument. There are a few possibilities for improving this.

  1. Throw an exception instead of ignoring encorderOptions when the runtime type check fails. This is a breaking change but it breaks code that was already broken so it sort of isn't a breaking change.
  2. Remove the SVG overload from toDataURL and instead create a dedicated function that calls toSVG and converts it to a data URL. This is a breaking change.
  3. Instead of using string, use a union of all image mime types. So type ImageMimeType = 'image/png' | 'image/svg+xml' .... Then the first overload could use 'image/svg+xml' and the second overload could be Exclude<ImageMimeType, 'image/svg+xml'>. This is also a breaking change. Also, maintaining a complete list of image mime types would be a bit of a pain.

Unless you feel like releasing version 5.0.0 with some breaking changes, it seems like the available options are limited.

@UziTech
Copy link
Collaborator

UziTech commented Oct 31, 2022

I don't care if they put a number for SVG. It will be handled the same as setting a number for PNG It does nothing. But I don't want ToSVGOption except for SVG.

@UziTech UziTech changed the title Fix breaking change to toDataURL type signature introduced by release 4.1.0 fix: Fix empty toDataURL() in TypeScript Oct 31, 2022
@UziTech UziTech merged commit 0ce27f0 into szimek:master Oct 31, 2022
@UziTech
Copy link
Collaborator

UziTech commented Oct 31, 2022

Thanks for bringing this to my attention! 💯

github-actions bot pushed a commit that referenced this pull request Oct 31, 2022
## [4.1.1](v4.1.0...v4.1.1) (2022-10-31)

### Bug Fixes

* Fix empty `toDataURL()` in TypeScript ([#672](#672)) ([0ce27f0](0ce27f0))
@github-actions
Copy link

🎉 This PR is included in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants