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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64689dc
fix: replace `URL.canParse()`
aloisklink Sep 23, 2024
5fcba26
ci: test using Node.JS v18.0
aloisklink Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

- run: npm install
- run: npm test
8 changes: 7 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ function decodeHtmlCharacters(str: string) {
}

function isValidUrl(url: string): boolean {
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

return true;
} catch (error) {
return false;
}
}

function decodeURI(uri: string): string {
Expand Down