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 #1488

Closed
wants to merge 4 commits into from
Closed

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 22, 2024

Changes

Following the big bump in #1276 which seems to have rolled out fine it is probably now worth moving to the latest version which has no scary changes.

Reapplied all patches

Best new feature is dialog element support

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 22, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Oct 22, 2024 6:16pm

@daibhin daibhin added the bump patch Bump patch version when this PR gets merged label Oct 22, 2024
@daibhin daibhin requested a review from a team October 22, 2024 16:36
package.json Outdated Show resolved Hide resolved
Comment on lines -25 to +27
+ const rootDomain = window.location.origin
+ const stylesheetPath = href.replace(window.location.href, '')
+ const potentialStylesheetHref = rootDomain + '/' + stylesheetPath
+ const rootDomain = window.location.origin;
+ const stylesheetPath = href.replace(window.location.href, '');
+ const potentialStylesheetHref = rootDomain + '/' + stylesheetPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real difference here, just added ; at the end of each line

Copy link

Size Change: +1.57 MB (+54.38%) 🆘

Total Size: 4.46 MB

Filename Size Change
dist/all-external-dependencies.js 406 kB +224 kB (+123.56%) 🆘
dist/array.full.js 559 kB +224 kB (+67.08%) 🆘
dist/array.full.no-external.js 558 kB +224 kB (+67.28%) 🆘
dist/module.full.js 559 kB +224 kB (+67.09%) 🆘
dist/module.full.no-external.js 558 kB +224 kB (+67.29%) 🆘
dist/recorder-v2.js 327 kB +224 kB (+218.95%) 🆘
dist/recorder.js 327 kB +224 kB (+218.82%) 🆘
ℹ️ View Unchanged
Filename Size
dist/array.full.es5.js 248 kB
dist/array.js 155 kB
dist/array.no-external.js 154 kB
dist/exception-autocapture.js 8.75 kB
dist/external-scripts-loader.js 2.19 kB
dist/main.js 156 kB
dist/module.js 155 kB
dist/module.no-external.js 154 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

Screenshot 2024-10-22 at 18 40 14 holy cow

@daibhin
Copy link
Contributor Author

daibhin commented Oct 22, 2024

Screenshot 2024-10-22 at 18 40 14 holy cow

Nooooiceeee 😎


I'm not sure what's causing this. Here is the list of changes between 16 and 17:

Screenshot 2024-10-22 at 18 43 45

Perhaps all of postcss is being pulled in? It should only be needed during playback

@pauldambra
Copy link
Member

Perhaps all of postcss is being pulled in? It should only be needed during playback

that's my first thought too

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.

weeeellllll, do i approve this or not.

it's way tidier since we lose the patches
it works locally, which is the definitive test to prove all is well
the bundled file is waaaaayyyyy bigger which would be nice to avoid

@pauldambra
Copy link
Member

yep @daibhin comparing v16 and v17 built output includes things like

.NODE_ENV&&"CssSyntaxError"===e.name&&t&&t.from&&(/\.scss$/i.test(t.from)?e.message+="\nYou tried to parse SCSS with the standard CSS parser; try again with the postcss-scss parser":/\.sass/i.test(t.from)?e.message+="\nYou tried to parse Sass with the standard CSS parser; try again with the postcss-sass parser":/\

so pretty sure that postcss is being included

@pauldambra
Copy link
Member

but then the console recording plugin is as big as rrweb when looking at the bundle
Screenshot 2024-10-22 at 21 08 43

so, 🤷

@daibhin
Copy link
Contributor Author

daibhin commented Oct 23, 2024

Closing in favour of #1489

@daibhin daibhin closed this Oct 23, 2024
@daibhin daibhin deleted the dn-chore/updagrade-rrweb branch October 23, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants