Uh oh!
There was an error while loading. Please reload this page.
feat(core): Deprecate parseUrl - #14404
Conversation
size-limit report 📦
|
❌ 17 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| attributes['server.address'] = parsedUrl.host; | ||
| try { | ||
| // The URL constructor can throw when there is no protocol or host. | ||
| const parsedUrl = new URL(resourceUrl); |
There was a problem hiding this comment.
can resourceUrl be a path-only, without a host? 🤔 just double checking.
| 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) { |
| try { | ||
| // The URL constructor can throw when there is no protocol or host. | ||
| const parsedUrl = new URL(resourceUrl); | ||
| if (parsedUrl.protocol) { |
| 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); |
There was a problem hiding this comment.
we should always try-catch this, to avoid stuff blowing up IMHO!
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', | ||
| }; | ||
| const parsedUrl = new URL(request.url, DUMMY_URL_BASE); |
mydea
commented
Nov 21, 2024
IMHO this is fine, but:
Generally I wonder if it would not be easier/better to just let the method be and just rewrite it to use |
lforst
commented
Nov 22, 2024
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
commented
Nov 22, 2024
Let's keep this funciton and replace the implementation with URL in a non-breaking way. |
Ref #14267
Deprecates and removes usages of
parseUrlwhich can be replaced with the URL constructor with our current version support.