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: remove v1 rrweb loading #1078

Merged
merged 5 commits into from
Mar 15, 2024
Merged

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Mar 13, 2024

Changes

  • Remove rrweb v1
  • Set rrweb v2 as the default everywhere (incl. array.full.js)

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

github-actions bot commented Mar 13, 2024

Size Change: +87.7 kB (+10%) ⚠️

Total Size: 940 kB

Filename Size Change
dist/array.full.js 224 kB +41.3 kB (+23%) 🚨
dist/array.js 124 kB -398 B (0%)
dist/es.js 124 kB -398 B (0%)
dist/module.js 124 kB -398 B (0%)
dist/recorder.js 106 kB +47.5 kB (+81%) 🆘
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.1 kB
dist/recorder-v2.js 106 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

rollup.config.js Outdated
@@ -22,18 +22,6 @@ const plugins = [

/** @type {import('rollup').RollupOptions[]} */
export default [
{
input: 'src/loader-recorder.ts',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops! We can't do this. Any old versions depending on it will break...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend just creating the old and new file both as the rrweb v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to point dist/recorder.js at src/loader-recorder-v2.ts (as in 818d433)

Or do some people reference src/loader-recorder.ts in their implementation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As in older versions will still load recorder.js or recorder-v2.js so we can't get rid of either file for backwards compatability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pushed a commit to update what I meant

@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Mar 14, 2024
@benjackwhite benjackwhite mentioned this pull request Mar 14, 2024
3 tasks
@daibhin
Copy link
Contributor Author

daibhin commented Mar 14, 2024

Holding off shipping until tomorrow morning so there is a full day in case this causes any unexpected customer issues

@daibhin daibhin merged commit b40a968 into main Mar 15, 2024
13 checks passed
@daibhin daibhin deleted the dn-chore/remove-v1-rrweb-loading branch March 15, 2024 11:14
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.

2 participants