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

Consider adding non-throwing URL.parse(input, base) #372

Closed
annevk opened this issue Feb 5, 2018 · 10 comments · Fixed by #825
Closed

Consider adding non-throwing URL.parse(input, base) #372

annevk opened this issue Feb 5, 2018 · 10 comments · Fixed by #825

Comments

@annevk
Copy link
Member

annevk commented Feb 5, 2018

Rather than throwing it would return null. This allows somewhat cleaner code.

See jsdom/jsdom#2146 for rationale.

@annevk annevk added topic: api needs implementer interest Moving the issue forward requires implementers to express interest labels Feb 5, 2018
@felixfbecker
Copy link

I read through the linked thread but don't see a convincing rationale.

After all, often these strings come as user input, and so it's not "exceptional" for them to be unparseable.

I would say at the layer of the URL API they are exceptional. If they are not exceptional in your application logic because they come from user input, you can catch the error, which makes it non-exceptional at the correct layer.

Even if the implementations don't do this at the moment, the benefit of an exception is that the error can include a nice error message explaining why the URL is invalid, e.g. "Character \u1234 is not allowed in URL" or "Port segment can only contain numbers, found character 'a'". If the URL truly comes from user input, that is a nicer message to display to the user than "Invalid URL".

@annevk
Copy link
Member Author

annevk commented Oct 15, 2018

The scenario is mostly about parsing URLs found in HTML and such. You could probably do some error signaling out-of-band, e.g., the developer console found in browsers, but that might not be relevant to everyone.

Also, note that this isn't about changing the default. This is solely about providing a wrapper, but it does seem that nobody has really been requesting this, so wontfixing might be fine.

@domenic
Copy link
Member

domenic commented May 4, 2020

I still think this, more so than many of the API additions proposed, is a good idea with no real downsides. IMO parser APIs should always provide a mode that returns a failure sentinel instead of throwing an exception. See e.g. https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern

However, I admit that we haven't seen a lot of demand---probably because try/catch is easy enough to use.

@annevk
Copy link
Member Author

annevk commented May 4, 2020

Yeah, if someone wants to add it and has some implementer backing I'd certainly be supportive.

@annevk annevk reopened this Nov 7, 2022
@annevk annevk closed this as completed Nov 7, 2022
@annevk annevk mentioned this issue Nov 7, 2022
@annevk
Copy link
Member Author

annevk commented Mar 13, 2024

https://twitter.com/kilianvalkhof/status/1765312128188088454 and responses (also in WHATWG Chat by Tab) shows there's still appetite for this. It's rather straightforward to add too. I think we should do it.

@annevk annevk reopened this Mar 13, 2024
@valenting
Copy link
Collaborator

I think this would be a nice and easy feature to add.

Before we commit to implementing it, I want to raise the possibility of returning an object instead of a URL or null.
Something like let { url } = URL.parse(input, base); allows us to add future fields to the return value, such as the error-type, if that's something we would be interested in.

In any case, I'm supportive of a non-throwing option to parse URLs.

@annevk
Copy link
Member Author

annevk commented Mar 14, 2024

I like that idea, but I suspect a separate method would be better if we ever decided to do that as URL parsers are highly optimized and you'd probably want a separate code path for the detailed error URL parser. Or at least allow for the possibility of one.

annevk added a commit to web-platform-tests/wpt that referenced this issue Mar 21, 2024
annevk added a commit that referenced this issue Mar 21, 2024
@annevk annevk mentioned this issue Mar 21, 2024
5 tasks
annevk added a commit that referenced this issue Mar 21, 2024
@annevk annevk removed the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 22, 2024
annevk added a commit that referenced this issue Mar 25, 2024
@petamoriken
Copy link

petamoriken commented Mar 27, 2024

@annevk I know it's a bit late, but just as there was discuss of putting in JSON.tryParse because JSON.parse throws SyntaxError, URL.tryParse might be a better choice.

@annevk
Copy link
Member Author

annevk commented Mar 27, 2024

Thanks for bringing that to my attention. I don't see the need for parity with JSON as we don't try to mimic anything from that API in URL. If there was a more widespread pattern of parse and tryParse methods, maybe, but even then I'd hope we only use the try prefix if there's an existing throwing method as it looks rather ungainly.

@eligrey
Copy link

eligrey commented Apr 29, 2024

I wrote a cross-platform polyfill: https://gist.github.com/eligrey/282cb2bef74d81e73bee7e836c82d0a5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants