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 support for custom protocols with the allowCustomProtocols option #132

Closed
wants to merge 3 commits into from

Conversation

ollyfg
Copy link

@ollyfg ollyfg commented Apr 7, 2021

We have been having problems with app protocols like "my.app://path/to/something".

A quick looks shows that valid protocols can have periods, hyphens and pluses in them and remain valid.

This PR keeps existing behavior unless the allowCustomProtocols option is provided, in which case more complex protocols aren't mangled.

Changes at a glance

Before:

normalizeUrl('my.mobile.app://sindresorhus.com');
//=> 'http://my.mobile.app/sindresorhus.com' ???

After:

normalizeUrl('my.mobile.app://sindresorhus.com', {
	allowCustomProtocols: true
});
//=> 'my.mobile.app://sindresorhus.com'

As you can see the before example doesn't make much sense. I feel like this would probably be a sane default to have enabled, but I don't want to break compatibility, so I'll leave that up to you.

@sindresorhus
Copy link
Owner

but I don't want to break compatibility

What kind of compatibility would it break? I would like to make it on by default. Just trying to think about the repercussions.

@sindresorhus
Copy link
Owner

Thinking out loud, do we even need an option for this? It kinda feels like a bug-fix. The existing behavior is clearly incorrect.

It's correctly handled by new URL, for example:

new URL('custom+with+plusses://sindresorhus.com')
//=> URL {origin: "null", protocol: "custom+with+plusses:", username: "", password: "", host: "", …}

@ollyfg
Copy link
Author

ollyfg commented Apr 9, 2021

You're right, there probably aren't any compatibility changes, I'm always just a bit cautious when I'm adding a new use case and I'm not the author, so there may be things I've missed.

I'll make this the default behavior then 👍

Do you want to keep the error on the view-source protocol?

@sindresorhus
Copy link
Owner

There's one possible ambiguity here. See: sindresorhus/prepend-http#4 Not sure how to handle that. Either disallow auth or require the custom protocol to use //. Thoughts?

@ollyfg
Copy link
Author

ollyfg commented Apr 22, 2021

I don't think this library handles auth in the url does it? Or at least currently it strips it out.

eg:

normalizeUrl('https://username:password@sindresorhus.com')
// -> 'https://sindresorhus.com'

This PR shouldn't affect that, since like you say, it uses // as the delimiter.

Edit: According to the spec, the delimiter between protocol (scheme) and other things should be : (here). I honestly can't understand the parts after that, but there's no obvious requirement for // following it, which is weird. That might just be my misunderstanding though. Shall we keep // as the delimiter and just mention that custom protocols need to use it?

@sindresorhus
Copy link
Owner

I don't think this library handles auth in the url does it? Or at least currently it strips it out.

You're right. Then that's a non-issue.

Shall we keep // as the delimiter and just mention that custom protocols need to use it?

Not sure what you're asking, but we should preserve it if it's there. We cannot normalize it away as it changes the semantics.

@sindresorhus
Copy link
Owner

Do you want to keep the error on the view-source protocol?

I think we could remove it.

@sindresorhus
Copy link
Owner

Bump :)

@sindresorhus
Copy link
Owner

Closing for lack of response. Happy to reopen if you ever get to this.

#152

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.

2 participants