Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecate and remove parseUrl and refactor getSanitizedUrlString #15767

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
Package: core Issues related to the Sentry Core SDK

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 21, 2025

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.

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.

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
AbhiPrasad added a commit that referenced this issue Mar 25, 2025
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 added a commit that referenced this issue Apr 1, 2025
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
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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Package: core Issues related to the Sentry Core SDK
Projects
None yet
Development

No branches or pull requests

1 participant