Skip to content

Deprecate and remove parseUrl and refactor getSanitizedUrlString #15767

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
Open
2 of 13 tasks
AbhiPrasad opened this issue Mar 21, 2025 · 2 comments
Open
2 of 13 tasks

Deprecate and remove parseUrl and refactor getSanitizedUrlString #15767

AbhiPrasad opened this issue Mar 21, 2025 · 2 comments
Labels
Improvement Package: core Issues related to the Sentry Core SDK

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 21, 2025 •
edited
Loading

Description

Now that we support ES2020 (aka not IE11 anymore) and Node.js 18+, we can get rid of parseUrl in favor of a method that just uses the built-in URL object. This will save us some bundle size (given we can remove that regex), and we get performance benefits from using native code.

sentry-javascript/packages/core/src/utils-hoist/url.ts

Line 17 in 3d63621

export function parseUrl(url: string): PartialURL {

Instead of just blanket replacing parseUrl, we'll slowly convert all it's usages to using a new helper, which looks something like so:

/**
 * Parses string to a URL object
 *
 * @param url - The URL to parse
 * @returns The parsed URL object or undefined if the URL is invalid
 */
export function parseStringToURL(url: string): URL | undefined {
  try {
    // Node 20+, Chrome 120+, Firefox 115+, Safari 17+
    if ('canParse' in URL) {
      // Use `canParse` to short-circuit the URL constructor if it's not a valid URL
      // This is faster than trying to construct the URL and catching the error
      return (URL as unknown as URLwithCanParse).canParse(url) ? new URL(url) : undefined;
    }
  } catch {
    // empty body
  }

  return undefined;
}

With that, we can also refactor getSanitizedUrlString, which should provide some performance benefits.

sentry-javascript/packages/core/src/utils-hoist/url.ts

Line 55 in 3d63621

export function getSanitizedUrlString(url: PartialURL): string {

Reminder list:

  • Can we remove extractQueryParamsFromUrl?
@AbhiPrasad AbhiPrasad added the Package: core Issues related to the Sentry Core SDK label Mar 21, 2025
This was referenced Mar 21, 2025
Closed
Merged
Merged
AbhiPrasad added a commit that referenced this issue Mar 25, 2025
@AbhiPrasad
ref(browser-utils): Give parseUrl a better name (#15813)
f2f21ee 
While working on
#15767 I ran into a
`parseUrl` function defined in
`packages/browser-utils/src/instrument/xhr.ts`.

This usage is fine, but was a bit confusing, so I renamed the method and
expanded the docstring. I also expanded the types a little bit.
@AbhiPrasad
Copy link
Member Author

It's super annoying to remove parseUrl because new URL breaks when you give it relative URLs 😢

I'm gonna try to solve this a different way.

@AbhiPrasad AbhiPrasad mentioned this issue Mar 27, 2025
Merged
AbhiPrasad added a commit that referenced this issue Apr 1, 2025
@AbhiPrasad
ref(core): Improve URL parsing utilities (#15882)
f0fc41f 
The bottleneck of many of the tasks written down in our Node SDK
performance improvement task
#15861 is
`parseUrl`, defined here:

https://github.com/getsentry/sentry-javascript/blob/3d63621714b31c1ea4c2ab2d90d5684a36805a43/packages/core/src/utils-hoist/url.ts#L17

We created #15767
to track removal of `parseUrl`. See more details in the GH issue.

While working on tasks for
#15767, I initially
PR'd #15768, which
introduced a `parseStringToURL` method as a replacement for `parseUrl`.
While trying to implement that method though I realized
`parseStringToURL` has a lot of downsides.

This PR removes `parseStringToURL` in favor of a new
`parseStringToURLObject`. Given `parseStringToURL` was never exported by
the core SDK, this is not a breaking change.

To understand `parseStringToURLObject`, let's first look at it's method
signature:

```ts
function parseStringToURLObject(url: string, urlBase?: string | URL | undefined): URLObject | undefined
```

`parseStringToURLObject` takes a string URL and turns it into a
`URLObject`, that is typed like so:

```ts
type RelativeURL = {
  isRelative: true;
  pathname: URL['pathname'];
  search: URL['search'];
  hash: URL['hash'];
};

type URLObject = RelativeURL | URL;
``` 

the JavaScript URL built-in does not handle relative URLs, so we need to
use a separate object to to track relative URLs. Luckily it's pretty
small surface area, we only need to worry about `pathname`, `search`,
and `hash`.

For ease of usage of this function, I also introduced a
`isURLObjectRelative` helper. This will make sure that users can handle
both relative and absolute URLs with ease.

Given `packages/core/src/fetch.ts` was using `parseStringToURL`, I
refactored that file to use `parseStringToURLObject`. The change feels
way better to me, much easier to read and understand what is going on.
@AbhiPrasad
Copy link
Member Author

landed in a decently elegant solution for relative urls: #15882

onurtemizkan pushed a commit that referenced this issue Apr 3, 2025
@AbhiPrasad @onurtemizkan
ref(core): Improve URL parsing utilities (#15882)
860d7ca 
The bottleneck of many of the tasks written down in our Node SDK
performance improvement task
#15861 is
`parseUrl`, defined here:

https://github.com/getsentry/sentry-javascript/blob/3d63621714b31c1ea4c2ab2d90d5684a36805a43/packages/core/src/utils-hoist/url.ts#L17

We created #15767
to track removal of `parseUrl`. See more details in the GH issue.

While working on tasks for
#15767, I initially
PR'd #15768, which
introduced a `parseStringToURL` method as a replacement for `parseUrl`.
While trying to implement that method though I realized
`parseStringToURL` has a lot of downsides.

This PR removes `parseStringToURL` in favor of a new
`parseStringToURLObject`. Given `parseStringToURL` was never exported by
the core SDK, this is not a breaking change.

To understand `parseStringToURLObject`, let's first look at it's method
signature:

```ts
function parseStringToURLObject(url: string, urlBase?: string | URL | undefined): URLObject | undefined
```

`parseStringToURLObject` takes a string URL and turns it into a
`URLObject`, that is typed like so:

```ts
type RelativeURL = {
  isRelative: true;
  pathname: URL['pathname'];
  search: URL['search'];
  hash: URL['hash'];
};

type URLObject = RelativeURL | URL;
``` 

the JavaScript URL built-in does not handle relative URLs, so we need to
use a separate object to to track relative URLs. Luckily it's pretty
small surface area, we only need to worry about `pathname`, `search`,
and `hash`.

For ease of usage of this function, I also introduced a
`isURLObjectRelative` helper. This will make sure that users can handle
both relative and absolute URLs with ease.

Given `packages/core/src/fetch.ts` was using `parseStringToURL`, I
refactored that file to use `parseStringToURLObject`. The change feels
way better to me, much easier to read and understand what is going on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Improvement Package: core Issues related to the Sentry Core SDK
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@stephanie-anderson @AbhiPrasad