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

Replace URL.canParse() with better supported code #80

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aloisklink
Copy link

URL.canParse was only pretty recently created, and is not available in many environments.

We can instead wrap a URL creation in a try..catch, as listed in MDN. Unfortunately, this is a bit slower, but at least it works almost everywhere:

This is a fast and easy alternative to constructing a URL within a try...catch block.

Taken from https://github.com/mdn/content/blob/392ce991114e55e2187510b640ab545d09258a16/files/en-us/web/api/url/canparse_static/index.md?plain=1#L13

Fixes #78


I've also updated the CI to run on Node.JS v18.0, instead of Node.JS v18.x. This is because #78 would have caught this issue, as URL.canParse() was only added in Node.JS v18.17.

[URL.canParse][1] was only pretty recently created, and is not
available in many environments.

We can instead wrap a `URL` creation in a `try..catch`, as listed in
MDN. Unfortunately, this is a bit slower, but at least it works almost
everywhere.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static

Fixes: braintree#78
This would have caught
like braintree#78, as
`URL.canParse()` was only added in Node.JS v18.17
@aloisklink aloisklink requested a review from a team as a code owner September 23, 2024 13:47
@aloisklink
Copy link
Author

If it helps, @ibooker said in #78 (comment) that this bug has an internal tracking ID of BTWEB-171.

return URL.canParse(url);
try {
// We can replace this with URL.canParse() once it's more widely available
new URL(url); // eslint-disable-line no-new -- We're only checking for exceptions
Copy link

@dcousens dcousens Sep 24, 2024

Choose a reason for hiding this comment

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

return URL.canParse?.(url) ?? (new URL(url) ? true : false)

Copy link
Author

Choose a reason for hiding this comment

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

This won't work, since new URL(url) throws an exception if it fails, so it needs to be wrapped in a try..catch.

But, we could do something like:

  if (URL.canParse) {
    return URL.canParse(url);
  }
  try {
    // We can replace this with URL.canParse() once it's more widely available
    new URL(url); // eslint-disable-line no-new -- We're only checking for exceptions
    return true;
  } catch (error) {
    return false;
  }

However, I'm not a fan of this. Sure, it's slightly faster in environments that do support URL.canParse, but the code is more complicated, and it's much harder to test, as we'd need environments that both support URL.canParse and don't support it.

Copy link

@dcousens dcousens Sep 24, 2024

Choose a reason for hiding this comment

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

to be clear, I didn't suggest to remove the try catch

Copy link

@dcousens dcousens Sep 24, 2024

Choose a reason for hiding this comment

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

I guess it's a useless suggestion though, when the documentation says:

It returns true for the same values for which the URL() constructor would succeed, and false for the values that would cause the constructor to throw.

If that holds true in perpetuity, then don't worry about my suggestion

@@ -12,6 +12,6 @@ jobs:
- name: Use Node.js
uses: actions/setup-node@v1
with:
node-version: "18.x"
node-version: "18.0"
Copy link

@Slokilla Slokilla Oct 8, 2024

Choose a reason for hiding this comment

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

Why do you pin it ? I have read your explanation, but I don't understand more.

Copy link
Author

@aloisklink aloisklink Oct 8, 2024

Choose a reason for hiding this comment

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

It's because URL.canParse doesn't exist in Node.JS v18.0, since it was only added in Node.JS v18.17.

So this test will make sure that we don't accidentally add anything that breaks with old versions of Node.JS v18. (if we do, we'd ideally want to make a new MAJOR change and state that the minimum supported of Node.JS has changed in the release notes).

For example, Amazon Linux 2023 only comes with Node.JS v18.12.

Copy link

Choose a reason for hiding this comment

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

Oh, that's a clever move. Thanks for explanation.

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.

Overly high JS baseline requirement, from using URL.canParse()
3 participants