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

fixes #51 #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fixes #51 #53

wants to merge 1 commit into from

Conversation

yelhouti
Copy link

@yelhouti yelhouti commented May 3, 2021

No description provided.

@striezel
Copy link

How does that fix it? As far as I can see in the commit (92720d9), it only eliminates the extra line

var ext = path.extname(url)

by using path.extname(url) directly instead of ext.

A possible fix should probably look more what @fatihtelis suggested in the comment of the original issue: #51 (comment)

@yelhouti
Copy link
Author

@striezel thanks for asking, the way it fixes it is by not running that code if type is set. makes sense to you ?

@striezel
Copy link

Yes, I see. 👍
But what if type is not set? It would still run that troublesome piece of code then.

@yelhouti
Copy link
Author

@striezel if no type is set it means that it is not something like data:image/png;base64... but rather a real url and therefore should run the code.

@yelhouti
Copy link
Author

@mikolalysenko could you please have look a this two ? thanks

@striezel
Copy link

@striezel if no type is set it means that it is not something like data:image/png;base64... but rather a real url and therefore should run the code.

Since JavaScript is not a statically typed language, you can never be quite sure what stuff people are passing into a function. So I thought some kind of check might possibly be better there. But if the maintainers are fine with it, so am I.

@mikolalysenko could you please have look a this two ? thanks

According to #49 (comment) mikolalysenko has notifications turned off. So there is no guarantuee this issue might get looked at anytime soon. :(

@yelhouti
Copy link
Author

@striezel the idea behind this is that you trust the caller to either provide a URL, or provide a data data:image... and specify the type.
But I agree the code can be improved to check itself for this stuff...
I really doubt that the maintainer will spend time on either adding or reviewing suck code, this is why I proposed the smallest change possible.

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