search

deprecate / metal-uri

Class for parsing and formatting URIs.
Other
3 stars 7 forks source link

Issue-5 - JS error will occur when using the URI constructor with a protocol handler like tel or mailto #7

Closed kienD closed 7 years ago

kienD commented 7 years ago

@fernandosouza,

I made the changes to fix issue-5.

I'm somewhat unsure about the changes I made since it is working off a list of uri schemes to avoid. The other way I was thinking of doing this was to create a whitelist of possible uris but the possible uri schemes are endless.

fernandosouza commented 7 years ago

Just started reviewing :)

:octocat: Sent from GH.

fernandosouza commented 7 years ago

Just started reviewing :)

:octocat: Sent from GH.

fernandosouza commented 7 years ago

Hey @kienD. I've included more test cases. Could you take a look?

cc. @brunobasto

kienD commented 7 years ago

@fernandosouza,

All the changes make sense to me. I just added another possible test case.

Thanks!

fernandosouza commented 7 years ago

LGTM

jbalsas commented 7 years ago

@Robert-Frampton, since we're on it... could we revisit the state of #5 and this PR in light of our recent changes to see if it's still needed and if so, to see if we can fix it?

robframpton commented 7 years ago

@jbalsas, yep. I'll do some testing on the develop branch and report back here.

Edit: this issue is still reproducible on the develop branch.

Edit 2: after looking into it, I think the only way we can reliably implement this behavior is if we go ahead with the new param you guys talked about that makes the parser not infer a default protocol. Otherwise our logic is going to get too complex, and we'll essentially be parsing the string twice.

import Uri from 'metal-uri';

// By setting second param to false, a default protocol is not added
const uri = new Uri('tel:123-123-1234', false);

uri.getProtocol() === 'tel:';

@jbalsas, do you think it's worth implementing this?

jbalsas commented 7 years ago

Let's try that, @Robert-Frampton!

jbalsas commented 7 years ago

Closing here since I guess we'll be creating a new PR.