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(isDataURI): Fix MIME Types with underscores not getting matched #2122

Merged
merged 2 commits into from Jan 22, 2023

Conversation

pano9000
Copy link
Contributor

@pano9000 pano9000 commented Dec 6, 2022

Fix isDataURI returning false for perfectly valid strings, more info can be found here: #2121

I've added an additional test case with a MIME type that has an underscore in its name and added the _ (underscore) to the RegExp to match for these cases.

This fixes #2121

(on a side note here: I feel like isDataURI could or maybe even should be rewritten to internally use isMimeType and isBase64 -> that way you would get rid of some code duplication - but that is a completely different topic than this quick bug fix).

Checklist

  • PR contains only changes related; no stray files, etc.
  • [ ] README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (c06fb15) compared to base (531dc7f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2308      2308           
  Branches       578       578           
=========================================
  Hits          2308      2308           
Impacted Files Coverage Δ
src/lib/isDataURI.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@WikiRik
Copy link
Member

WikiRik commented Dec 6, 2022

(on a side note here: I feel like isDataURI could or maybe even should be rewritten to internally use isMimeType and isBase64 -> that way you would get rid of some code duplication - but that is a completely different topic than this quick bug fix).

I was just about to suggest this. I think it would be useful to use isMimeType in this PR. Should not be a difficult fix but makes fixing future bugs a lot easier. isBase64 is a nice benefit but I do not expect that to change anytime soon

@pano9000
Copy link
Contributor Author

pano9000 commented Dec 6, 2022

wouldn't this be more of a "feat" or "refactor" type of PR then, and out of scope of this bugfix PR?

I can take a shot at this, but I would like to have this bugfix in first, if that makes sense?

@WikiRik
Copy link
Member

WikiRik commented Dec 6, 2022

That indeed makes sense, but I'm not so sure if you want to wait that long. Merging PRs does not happen that often in this library (e.g. look at the last release of over a year ago), so it might be better to combine PRs of the same scope. In this case I would maybe even think about closing this PR and fixing this issue in #2120. That way it also makes more sense to use isMimeType for isDataURI in there since you only have to update a single RegExp

@braaar
Copy link
Contributor

braaar commented Dec 7, 2022

I guess I'm a stickler for keeping PRs minimal and discrete, so I would intuitively be skeptical of @WikiRik's approach. It's a shame that it feels necessary, though.

@rubiin rubiin added ready-to-land For PRs that are reviewed and ready to be landed ✅ LGTM labels Jan 21, 2023
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thank!

@profnandaa profnandaa merged commit a571b3e into validatorjs:master Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ LGTM ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isDataURI: is not matching valid MIME types with an underscore
5 participants