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

chore: upgrade rrweb to alpha.17 #1489

Merged
merged 23 commits into from
Nov 11, 2024
Merged

chore: upgrade rrweb to alpha.17 #1489

merged 23 commits into from
Nov 11, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 23, 2024

Changes

Trying something out...

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Oct 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Nov 4, 2024 11:35pm

@daibhin daibhin changed the title Dn chore/use rrweb record chore: upgrade rrweb to alpha.17 Oct 23, 2024
@PostHog PostHog deleted a comment from github-actions bot Oct 23, 2024
Copy link

github-actions bot commented Oct 23, 2024

Size Change: +76.7 kB (+2.55%)

Total Size: 3.09 MB

Filename Size Change
dist/all-external-dependencies.js 204 kB +11 kB (+5.69%) 🔍
dist/array.full.js 353 kB +11 kB (+3.2%)
dist/array.full.no-external.js 352 kB +11 kB (+3.21%)
dist/module.full.js 353 kB +11 kB (+3.21%)
dist/module.full.no-external.js 352 kB +11 kB (+3.21%)
dist/recorder-v2.js 113 kB +10.9 kB (+10.68%) ⚠️
dist/recorder.js 113 kB +10.9 kB (+10.68%) ⚠️
ℹ️ View Unchanged
Filename Size
dist/array.full.es5.js 251 kB
dist/array.js 168 kB
dist/array.no-external.js 167 kB
dist/dead-clicks-autocapture.js 12.8 kB
dist/exception-autocapture.js 8.77 kB
dist/external-scripts-loader.js 2.19 kB
dist/main.js 169 kB
dist/module.js 168 kB
dist/module.no-external.js 167 kB
dist/surveys-preview.js 56.7 kB
dist/surveys.js 62.1 kB
dist/tracing-headers.js 1.33 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@pauldambra
Copy link
Member

we were queueing #1464 on these upgrades but 🤔

@pauldambra
Copy link
Member

i guess that this isn't working because postcss is already bundled in with rrweb, not being included by our bundler 🤔

@pauldambra
Copy link
Member

tested locally and can still record and playback.

the bundle size increase is pretty extreme but we can loop back and improve that once we have things fixed

@pauldambra
Copy link
Member

Screenshot 2024-11-02 at 23 31 05

holy cow that increase is almost all in the console recorder 🤯

@pauldambra
Copy link
Member

🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 @daibhin search in the console recorder js for postcss 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣

@pauldambra
Copy link
Member

pauldambra commented Nov 2, 2024

i believe bundle size fix is in rrweb-io/rrweb#1594

@daibhin
Copy link
Contributor Author

daibhin commented Nov 3, 2024

🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 @daibhin search in the console recorder js for postcss 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣

Crikey! We might need to contribute our bundle size increase checker back to rrweb 😅 I should really never have been allowed to increase the size of that package so much in the first place

@daibhin
Copy link
Contributor Author

daibhin commented Nov 3, 2024

@pauldambra genius!! I removed the postcss code from the console plugin and the package sizes are finally back in a place of reasonable increase. Want to do a little bit of cleanup given all the changes we've made in this PR but ultimately think we're good to ship this upgrade

rollup.config.js Outdated
Comment on lines 84 to 85
// we know that recorder doesn't use postcss, but the bundler can't determine that
external: ['postcss'],
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 still need this? it didn't work, right?

Suggested change
// we know that recorder doesn't use postcss, but the bundler can't determine that
external: ['postcss'],

@@ -72,7 +71,7 @@ describe('MutationRateLimiter', () => {

test('returns event if _any_ adds are left', () => {
const event = makeEvent({
adds: [{ parentId: 0, nextId: 0, node: {} as unknown as serializedNodeWithId }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will have to come back and type this once rrweb-io/rrweb#1593 merges

@daibhin daibhin added the bump minor Bump minor version when this PR gets merged label Nov 5, 2024
@pauldambra pauldambra self-requested a review November 11, 2024 20:28
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

works locally for me....

@pauldambra pauldambra merged commit e534ffc into main Nov 11, 2024
16 checks passed
@pauldambra pauldambra deleted the dn-chore/use-rrweb-record branch November 11, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants