feat(core): Deprecate `parseUrl` by lforst · Pull Request #14404 · getsentry/sentry-javascript

lforst · 2024-11-21T13:31:27Z

Deprecates and removes usages of parseUrl which can be replaced with the URL constructor with our current version support.

github-actions · 2024-11-21T13:39:20Z

size-limit report 📦

Path	Size	% Change	Change
@sentry/browser	22.78 KB	-0.44%	-102 B 🔽
@sentry/browser - with treeshaking flags	21.48 KB	-0.44%	-96 B 🔽
@sentry/browser (incl. Tracing)	35.41 KB	-0.06%	-20 B 🔽
@sentry/browser (incl. Tracing, Replay)	72.12 KB	+0.02%	+9 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	62.4 KB	-0.03%	-18 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas)	76.41 KB	-0.01%	-5 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback)	88.88 KB	-0.02%	-10 B 🔽
@sentry/browser (incl. Feedback)	39.51 KB	-0.27%	-106 B 🔽
@sentry/browser (incl. sendFeedback)	27.41 KB	-0.37%	-103 B 🔽
@sentry/browser (incl. FeedbackAsync)	32.22 KB	-0.3%	-98 B 🔽
@sentry/react	25.48 KB	-0.41%	-105 B 🔽
@sentry/react (incl. Tracing)	38.26 KB	-0.07%	-25 B 🔽
@sentry/vue	26.93 KB	-0.38%	-105 B 🔽
@sentry/vue (incl. Tracing)	37.23 KB	-0.05%	-16 B 🔽
@sentry/svelte	22.91 KB	-0.51%	-119 B 🔽
CDN Bundle	23.95 KB	-0.4%	-97 B 🔽
CDN Bundle (incl. Tracing)	36.98 KB	-0.03%	-11 B 🔽
CDN Bundle (incl. Tracing, Replay)	71.67 KB	-0.03%	-22 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback)	77.02 KB	-0.04%	-28 B 🔽
CDN Bundle - uncompressed	70.72 KB	-0.27%	-190 B 🔽
CDN Bundle (incl. Tracing) - uncompressed	110.15 KB	+0.03%	+28 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed	222.68 KB	+0.02%	+28 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	235.89 KB	+0.02%	+28 B 🔺
@sentry/nextjs (client)	38.36 KB	-0.08%	-31 B 🔽
@sentry/sveltekit (client)	35.92 KB	-0.05%	-16 B 🔽
@sentry/node	134.41 KB	-0.06%	-74 B 🔽
@sentry/node - without tracing	96.25 KB	-0.07%	-65 B 🔽
@sentry/aws-serverless	106.5 KB	-0.07%	-72 B 🔽

View base workflow run

codecov · 2024-11-21T13:39:50Z

❌ 17 Tests Failed:

Tests completed	Failed	Passed	Skipped
653	17	636	28

View the top 3 failed tests by shortest run time

transactions.test.ts Sends an API route transaction

Stack Traces | 0.021s run time

transactions.test.ts:4:5 Sends an API route transaction

transactions.test.ts Sends an API route transaction from module

Stack Traces | 0.021s run time

transactions.test.ts:4:5 Sends an API route transaction from module

transactions.test.ts Sends successful transaction

Stack Traces | 0.028s run time

transactions.test.ts:4:5 Sends successful transaction

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

mydea · 2024-11-21T14:49:43Z

packages/browser-utils/src/metrics/browserMetrics.ts

-    attributes['server.address'] = parsedUrl.host;
+  try {
+    // The URL constructor can throw when there is no protocol or host.
+    const parsedUrl = new URL(resourceUrl);


can resourceUrl be a path-only, without a host? 🤔 just double checking.

mydea · 2024-11-21T14:50:42Z

packages/browser-utils/src/metrics/browserMetrics.ts

+    if (parsedUrl.protocol) {
+      attributes['url.scheme'] = parsedUrl.protocol.split(':').pop(); // the protocol returned by parseUrl includes a :, but OTEL spec does not, so we remove it.
+    }
+    if (parsedUrl.host) {


can host even be empty? 🤔

mydea · 2024-11-21T14:51:05Z

packages/browser-utils/src/metrics/browserMetrics.ts

+  try {
+    // The URL constructor can throw when there is no protocol or host.
+    const parsedUrl = new URL(resourceUrl);
+    if (parsedUrl.protocol) {


can protocol be empty?

mydea · 2024-11-21T14:51:41Z

packages/browser/src/integrations/breadcrumbs.ts

-    let parsedFrom = from ? parseUrl(from) : undefined;
-    const parsedTo = parseUrl(to);
+    let parsedFrom = from ? new URL(from, DUMMY_URL_BASE) : undefined;
+    const parsedTo = new URL(to, DUMMY_URL_BASE);


we should always try-catch this, to avoid stuff blowing up IMHO!

mydea · 2024-11-21T14:54:09Z

packages/bun/src/integrations/bunserver.ts

          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
        };
+
+        const parsedUrl = new URL(request.url, DUMMY_URL_BASE);


try-catch, maybe?

mydea · 2024-11-21T14:56:34Z

IMHO this is fine, but:

let's always try-catch new URL( calls to ensure we do not throw for weird input or so.
Let's also make sure we do not regress anything by not handling relative paths only. Now, we seem to handle it sometimes, but not always.

Generally I wonder if it would not be easier/better to just let the method be and just rewrite it to use new URL under the hood 😅 then we could centralize/streamline both points I mentioned above. I'm also fine with repeating this everywhere we use this, but it does feel a bit boiler-platey to me 🤷

lforst · 2024-11-22T10:53:09Z

Generally I wonder if it would not be easier/better to just let the method be and just rewrite it to use new URL under the hood 😅 then we could centralize/streamline both points I mentioned above. I'm also fine with repeating this everywhere we use this, but it does feel a bit boiler-platey to me 🤷

I had the exact same thought today, the only problem I had with it was that it is maybe too generic to be exported as API from a Sentry package. At the same time, nobody probably cares.

lforst · 2024-11-22T11:02:31Z

Let's keep this funciton and replace the implementation with URL in a non-breaking way.

feat(core): Deprecate parseUrl

26f38eb

lforst mentioned this pull request Nov 21, 2024

Deprecate unfitting exports from utils #14267

Closed

13 tasks

AbhiPrasad approved these changes Nov 21, 2024

View reviewed changes

mydea reviewed Nov 21, 2024

View reviewed changes

lforst closed this Nov 22, 2024

lforst deleted the lforst-rm-parseUrl branch December 3, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(core): Deprecate `parseUrl`#14404

feat(core): Deprecate `parseUrl`#14404
lforst wants to merge 1 commit intodevelopfrom
lforst-rm-parseUrl

lforst commented Nov 21, 2024

Uh oh!

github-actions bot commented Nov 21, 2024

Uh oh!

codecov bot commented Nov 21, 2024 •

edited

Loading

Uh oh!

mydea Nov 21, 2024

Uh oh!

mydea Nov 21, 2024

Uh oh!

mydea Nov 21, 2024

Uh oh!

mydea Nov 21, 2024

Uh oh!

mydea Nov 21, 2024

Uh oh!

mydea commented Nov 21, 2024

Uh oh!

lforst commented Nov 22, 2024

Uh oh!

lforst commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lforst commented Nov 21, 2024

Uh oh!

github-actions bot commented Nov 21, 2024

size-limit report 📦

Uh oh!

codecov bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 17 Tests Failed:

Uh oh!

mydea Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

mydea commented Nov 21, 2024

Uh oh!

lforst commented Nov 22, 2024

Uh oh!

lforst commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 21, 2024 •

edited

Loading