Skip to content

ref(replay): Split replay integration & container class - #6407

Merged
mydea merged 3 commits into
masterfrom
fn/replay-split
Dec 6, 2022
Merged

ref(replay): Split replay integration & container class#6407
mydea merged 3 commits into
masterfrom
fn/replay-split

Conversation

@mydea

@mydeamydea commented Dec 5, 2022

Copy link
Copy Markdown
Member

A part of #6406, this splits up the main index.ts file of replay into two parts:

  1. The actual Sentry SDK Integration class. The main concern of this is to handle the setup etc.
  2. A Replay Container class which encapsulates the replay state, as well as all the actual functionality etc.

}

if (session.id !== this.session?.id) {
session.previousSessionId = this.session?.id;

Check failure

Code scanning / CodeQL

Insecure randomness

This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context.
@github-actions

github-actionsBot commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.67 KB (-0.03% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)60.94 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified)54.49 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.26 KB (0%)
@sentry/browser - Webpack (minified)66.24 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.14 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.52 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)24.96 KB (+0.02% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)41.79 KB (-0.36% 🔽)
@sentry/replay - Webpack (gzipped + minified)37.91 KB (+0.06% 🔺)

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the change because a) single responsibility is always good and b) (as we previously discussed) it gets us one step closer to converting the replay logic into a functional structure. We can start thinking about this if we have the time after we finish our other points on the roadmap. Added a point to #5326.

describe('integration settings', () => {
describe('blockAllMedia', () => {
it('sets the correct configuration when `blockAllMedia` is disabled', async () => {
const { replay } = await mockSdk({ replayOptions: { blockAllMedia: false } });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good change (never knew that the previous syntax actually works 🤔 )

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, we should prob. do that everywhere! But just stumbled over this because of some other refactoring made necessary here 😅

@mydea

mydea commented Dec 5, 2022

Copy link
Copy Markdown
MemberAuthor

Note: I also added an entry to the MIGRATION docs.

Comment threadpackages/replay/src/index.ts Outdated
@mydea
mydeaforce-pushed the fn/replay-split branch 3 times, most recently from 09c1797 to 23c964dCompareDecember 6, 2022 12:30
@mydea
mydea merged commit 591f9cb into masterDec 6, 2022
@mydea
mydea deleted the fn/replay-split branch December 6, 2022 13:10
Sign up for freeto 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.

4 participants

@mydea@billyvg@Lms24@github-advanced-security