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 data URL support #95

Merged
merged 17 commits into from
Sep 20, 2019
Merged

Conversation

fisker
Copy link
Contributor

@fisker fisker commented Jul 7, 2019

fixes: #94 & #84

questions:

  1. url data:text/plain;charset=UTF-8;,foo mentioned in Add support for data URLs #84, is not valid url accounding to valid-data-url, should I change Regex to loose strict one?
  2. any more test cases need cover?
  3. are we going to support other protocols like file: ftp: etc

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus sindresorhus changed the title Add dataURLs support Add data URL support Sep 17, 2019
@sindresorhus
Copy link
Owner

url data:text/plain;charset=UTF-8;,foo mentioned in #84, is not valid url accounding to valid-data-url, should I change Regex to loose strict one?

Yes, but make sure it's a valid data URL according to the spec first: https://tools.ietf.org/html/rfc2397

any more test cases need cover?

I can't really think of more cases.

are we going to support other protocols like file: ftp: etc

Would be useful to support file: as it's quite common. Not interested in supporting ftp though.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Would be good to mention data URL support in the readme and index.d.ts. I don't think it's obvious.

@fisker
Copy link
Contributor Author

fisker commented Sep 18, 2019

Would be good to mention data URL support in the readme and index.d.ts. I don't think it's obvious.

Not sure how do you want to do this, add an example? or explain url parameter?

@sindresorhus
Copy link
Owner

or explain url parameter?

This

@fisker
Copy link
Contributor Author

fisker commented Sep 18, 2019

@sindresorhus ready to review

@sindresorhus sindresorhus merged commit 7df5aff into sindresorhus:master Sep 20, 2019
@sindresorhus
Copy link
Owner

Looks good :)

@fisker fisker deleted the normalize-dataurls branch September 20, 2019 09:32
@fisker
Copy link
Contributor Author

fisker commented Sep 20, 2019

I just found I might made a mistake, accounding to MSDN https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs#Common_problems data URL don't support query strings

//cc @sindresorhus

@sindresorhus
Copy link
Owner

Would you be able to submit a PR with a fix? We should probably not do anything with the data part, so ignore all logic like query string, hash, path, etc.

@fisker
Copy link
Contributor Author

fisker commented Sep 20, 2019

Yes, working on it

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.

Normalize data URLs
2 participants