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.

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

⚗[REPLAY-465] Base tag support (feature flagged): Remove URL transformation from relative to absolute #1106

Merged

Conversation

jagracey
Copy link
Contributor

@jagracey jagracey commented Oct 4, 2021

Motivation

As per internal RFC "Supporting Replay of Relative URLs When Tag Used", we want to support the tag. In short, this tag will cause the document to resolve relative URLs with the provided URL instead.

Changes

We don't need to transform URLs from relative to absolute on the SDK recorder side. Instead we will handle this on the replay side.

Testing

Removes tests for code that transforms URLs. Manual testing for now behind a feature flag.


I have gone over the contributing documentation.

@jagracey jagracey requested review from a team as code owners October 4, 2021 11:44
@jagracey jagracey changed the title [REPLAY-465] Base tag support: Remove URL transformation from relative to absolute ✨[REPLAY-465] Base tag support: Remove URL transformation from relative to absolute Oct 4, 2021
@jagracey jagracey changed the title ✨[REPLAY-465] Base tag support: Remove URL transformation from relative to absolute ⚗[REPLAY-465] Base tag support (feature flagged): Remove URL transformation from relative to absolute Oct 4, 2021
@@ -58,6 +64,9 @@ export function makeSrcsetUrlsAbsolute(attributeValue: string, baseUrl: string)

export function makeUrlAbsolute(url: string, baseUrl: string): string {
try {
if (isExperimentalFeatureEnabled('base-tag')) {
return url.trim()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the .trim() here? I'd suggest removing it.

Copy link
Contributor Author

@jagracey jagracey Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was pondering that myself. Why did we trim in the first place? Updated in the meantime.

@codecov-commenter
Copy link

Codecov Report

Merging #1106 (42a3648) into main (dca52cd) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1106      +/-   ##
==========================================
- Coverage   89.07%   89.04%   -0.04%     
==========================================
  Files          91       91              
  Lines        4193     4199       +6     
  Branches      956      959       +3     
==========================================
+ Hits         3735     3739       +4     
- Misses        458      460       +2     
Impacted Files Coverage Δ
...ckages/rum/src/domain/record/serializationUtils.ts 94.44% <50.00%> (-5.56%) ⬇️
packages/core/src/tools/timeUtils.ts 94.28% <0.00%> (+2.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca52cd...42a3648. Read the comment docs.

@jagracey jagracey merged commit 3e3a1bc into main Oct 5, 2021
@jagracey jagracey deleted the john.gracey/remove-absolute-url-transformation-feature-flagged branch October 5, 2021 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants