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

Use non-forked rrweb #34

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Use non-forked rrweb #34

merged 3 commits into from
Nov 16, 2023

Conversation

tevanoff
Copy link
Contributor

@tevanoff tevanoff commented Nov 9, 2023

Issue: CAP-1192

What Changed

Use standard, non-forked rrweb.

How to test

  • Use canary version on test projects
  • See all images load in Chromatic build
  • No changes in Chromatic E2E tests using the canary

Change Type

  • maintenance
  • documentation
  • patch
  • minor
  • major
📦 Published PR as canary version: 0.0.38--canary.34.22b8530.0

✨ Test out this PR locally via:

npm install @chromaui/[email protected]
# or 
yarn add @chromaui/[email protected]

@tevanoff tevanoff changed the base branch from main to todd/fix-css-attribute-mapping November 9, 2023 23:43
@tevanoff tevanoff force-pushed the rrweb-no-fork branch 3 times, most recently from 012ea55 to 486d853 Compare November 13, 2023 17:04
@tevanoff tevanoff changed the base branch from todd/fix-css-attribute-mapping to main November 13, 2023 17:04
@tevanoff tevanoff requested review from skitterm and jwir3 November 13, 2023 18:08
"@segment/analytics-node": "^1.1.0",
"fs-extra": "^11.1.1",
"mime": "^3.0.0",
"rrweb-snapshot": "^2.0.0-alpha.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rrweb is up to like alpha.11, but alpha.4 is the one published as latest.

@@ -30,8 +30,7 @@ export class ArchiveFile {
}

originalSrc() {
// Assets that we capture will be stripped of the domain in the source
return `${this.url.pathname}${this.url.search}`;
return this.url.toString();
Copy link
Contributor Author

@tevanoff tevanoff Nov 13, 2023

Choose a reason for hiding this comment

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

This is the main change required after all of the previous asset mapping work. We're now expecting all asset paths to be full URLs since that's what rrweb does, so we don't strip off the protocol and origin here anymore.

@tevanoff tevanoff marked this pull request as ready for review November 13, 2023 18:12
},
},
{
type: NodeType.Element,
attributes: {
src: '/img?src=some-path',
src: 'http://localhost:3000/img?src=some-path',
Copy link
Member

Choose a reason for hiding this comment

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

Do we save the URLs like this in the archives or as relative paths? If we save as absolutes, that would be a non-starter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The asset archive is built by watching network requests, so they are always the full URL there. This change makes it easier to match the assets from the archive with the attributes in the DOM since both sides will be full URLs now.

Copy link
Member

Choose a reason for hiding this comment

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

Let me rephrase my question.

What happens when you render in the Capture Cloud? Do the archives try to pull from a relative path or http://localhost:3000? The localhost lookup will not work in the capture cloud so those need to be replaced by relative paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, they are relative in the archive storybook so that they load from the capture cloud. Otherwise all the images in the build would be broken!

@@ -18,6 +18,7 @@
<div><img src="/img?url=fixtures/pink.png"/></div>
<div><img src="/img"/></div>
<div><img src="/img/another"/></div>
<div><img src="relative/purple.png"/></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thafryer added that relative path test for AP-3789

Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

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

LGTM!

@tevanoff tevanoff merged commit 54512f9 into main Nov 16, 2023
2 checks passed
@thafryer
Copy link
Member

🚀 PR was released in v0.0.38 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants