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

feat(hasProtocol): optionally allow protocol relative url #16

Merged
merged 5 commits into from
Mar 5, 2021
Merged

Conversation

danielroe
Copy link
Member

No description provided.

@danielroe danielroe requested a review from pi0 March 2, 2021 16:29
@danielroe
Copy link
Member Author

Happy to flip default or disable parseURL support for protocol relative URLs but I think better to accept in parseURL because of prevalence of them in the wild.

README.md Outdated
// Result: false
isURL('/')
// Result: true
isURL('//', true)
Copy link
Member

Choose a reason for hiding this comment

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

// is not really a valid URL even considering scheme is optional since has no path:

URI = scheme:[//authority]path[?query][#fragment]

Copy link
Member

@pi0 pi0 Mar 2, 2021

Choose a reason for hiding this comment

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

Trying in chrome, <a href="//> redirects to about:blank#blocked (or any pattern like ///)

src/utils.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #16 (4445e56) into main (e0a1599) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
+ Coverage   86.91%   87.03%   +0.12%     
==========================================
  Files           7        7              
  Lines         214      216       +2     
  Branches       60       62       +2     
==========================================
+ Hits          186      188       +2     
  Misses         28       28              
Impacted Files Coverage Δ
src/parse.ts 87.50% <100.00%> (ø)
src/utils.ts 98.43% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0a1599...4445e56. Read the comment docs.

@pi0
Copy link
Member

pi0 commented Mar 3, 2021

@danielroe More I'm thinking isURL, should do real URL validation rather than checking protocol existence. Something like http://. is invalid and it can cause troubles if isURL being used in conditions to trsut a user input (see examples in this list: https://mathiasbynens.be/demo/url-regex)

What do you think if in this PR:

  • Keep/Add support for protocol relative parsing
  • Add flag to hasProtocol util to optionally allow relative syntax

And move isURL task to new PR with proper regex (or parser based on perf and complexity)?

@danielroe
Copy link
Member Author

@pi0 Sounds like a good plan. I'll make a stab.

(Separate comment) I wonder about differentiating utils that parse/validate input in accordance with spec compliance vs 'cheap' utils that assume pre validation.

@pi0
Copy link
Member

pi0 commented Mar 3, 2021

@danielroe Agree for naming. hasProtocol since even it is a cheap implementation, it's name is clear it is not validating entire URL.

BTW generally, there is not single standard for URL to follow and this PR is an example about conventions that are supported. (another example plus as space encoding)
My main goal for ufo is making stable utils for unversal/consistent behavior and sticking to standards as much as possible.

@danielroe danielroe changed the title feat: add isURL utility and parse urls without protocol feat: respect protocol relative URLs in hasProtocol and in URL functions Mar 3, 2021
@pi0 pi0 changed the title feat: respect protocol relative URLs in hasProtocol and in URL functions feat(hasProtocol): optionally allow protocol relative URLs Mar 5, 2021
@pi0 pi0 changed the title feat(hasProtocol): optionally allow protocol relative URLs feat(hasProtocol): optionally allow protocol relative url Mar 5, 2021
@pi0 pi0 merged commit 81a3f3e into main Mar 5, 2021
@pi0 pi0 deleted the is-url branch March 5, 2021 11:18
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.

3 participants