feat(browser): Export `Replay` integration from Browser SDK #6508

Lms24 · 2022-12-13T10:10:00Z

This PR exports the Replay integration from @sentry/replay in @sentry/browser. This will eliminate the need for users to separately install the @sentry/replay package as they have to do right now. Instead, they can just use Replay like it's part of the core Browser SDK or any FE Framework SDK that builds on top of Browser:

import * as Sentry from '@sentry/browser'; // or @sentry/react, ...

Sentry.init({
  // dsn, other options, etc
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1.0,
  integrations: [new Sentry.Replay()]
});

Alternatively, import { Replay } from '@sentry/browser' works, too.

Note: This does not affect users who imported the integration from @sentry/replay so it should be fully backwards compatible.

Bundle Size Concerns

As far as I tested, for the NPM package, everything Replay-related is tree-shaken out by bundlers if Replay is not used.

For standalone CDN bundles of browser and browser+tracing, we have to first remove the export from index.ts in the Browser package, before we transpile and bundle so that the integration isn't included in them. In the future, we can think about a complete bundle that contains Replay but this is out of scope for this PR and its issue.

To remove the export from index.ts, this PR introduces a new rollup plugin that removes all code between specific comment guards. I chose this approach over creating a separate index.bundle.ts file because we'd anyhow need to apply some rollup intervention to select the correct index file for the browser+tracing bundles. Since we're already experienced by now with rollup plugins, I figured, it'd be a nicer way.

ref: #6326

github-actions · 2022-12-13T12:54:11Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.78 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	61.27 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.57 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	54.8 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.34 KB (0%)
@sentry/browser - Webpack (minified)	66.48 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.36 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.57 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.76 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.19 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	41.87 KB (0%)
@sentry/replay - Webpack (gzipped + minified)	38.1 KB (0%)

wip

This reverts commit 527f34d.

lforst

I love this. Very pragmatic for what it achieves IMO. Almost ready to approve this. I would just like to resolve all conversations before pressing the button.

rollup/plugins/bundlePlugins.js

lforst · 2022-12-14T12:40:48Z

rollup/plugins/bundlePlugins.js

+ */
+export function makeExcludeReplayPlugin() {
+  const replacementRegex = /\/\/ __ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__(.|\n)*__ROLLUP_EXCLUDE_FROM_BUNDLES_END__/m;
+  const browserIndexFilePath = path.resolve(__dirname, '../../packages/browser/src/index.ts');


m: Just a tiny bit worried that if we move this file (or less likely but worse, the place of the guard) that this is gonna break. Thinking can we somehow test for these cases? Or make this more robust.

Honestly, I can't come up with anything obvious that wouldn't be super complicated so I am totally fine with just leaving it as is.

IMHO the best way to check this is via size checks. What about actually decreasing the allowed size limits to reasonable amounts? E.g. currently we have stuff like 100kb in there for browser. What if we just set this to like "current size + 10kb"? Then tests would actually fail when something becomes too large?

Great idea with size-check! I can lower the limits as this should be a good indicator when something goes wrong with the replay export removal.

In terms of this being brittle, yes I agree but then again, we're talking about the main index.ts file which IMO is unlikely to change location or name. Obviously, moving the guard would still be a problem but size-check would let us know about this (fwiw, even without limit adjustments as it failed while I still had a bug in this PR).

If we decide to make this plugin more general at some point, we could pass a "transform files allowlist" or something similar to make changing file entries more easy but for now I'd vote to keep the plugin as is.

Decided to pull this out into its own PR so we have a better history as to why we made this change: #6543

rollup/utils.js

lforst · 2022-12-14T12:47:19Z

packages/browser/src/client.ts

 import { createClientReportEnvelope, dsnToString, logger, serializeEnvelope } from '@sentry/utils';

 import { eventFromException, eventFromMessage } from './eventbuilder';
 import { WINDOW } from './helpers';
 import { Breadcrumbs } from './integrations';
 import { BREADCRUMB_INTEGRATION_ID } from './integrations/breadcrumbs';
 import { BrowserTransportOptions } from './transports/types';
-
-type BrowserClientReplayOptions = {


Just wondering: Why are we moving this type into the types package?

So that @sentry/replay does not need to import anything from @sentry/browser, to avoid circular dependencies.

lforst · 2022-12-14T12:48:05Z

packages/replay/src/integration.ts

 import { getCurrentHub } from '@sentry/core';
-import { Integration } from '@sentry/types';
+import { BrowserClientReplayOptions, Integration } from '@sentry/types';


l/m: Could we keep this as import type?

Lower our `size-limit` maximum bundle size limits. Previously, most of them were set to rather arbitrary high numbers (100Kb mostly). With these adjustments, we lower them to +10Kb of each entry's current size. The reason for doing this is to catch problems with tree shaking or unexpected large bundle size hits more reliably. As outlined in #6508 (comment), we could for instance catch the Replay integration being wrongfully included in to our standalone Browser SDK CDN bundles. In case the limits are hit, the "Size Limit" CI job will fail.

mydea

Amazing work!

…n tests (#6570) This patch fixes the broken NextJS and Remix integration tests which were unable to look up the local version of `@sentry/replay` before. Replay is now a dependency of these tests because they depend on `@sentry/browser` (which in turn depends on Replay after #6508).

This PR updates the Session Replay docs after we released version 7.27.0 of the JS SDKs. With this new version, users don't need to explicitly install `@sentry/replay` anymore, as it is now a dependency of `@sentry/browser` which [exports the Replay integration](getsentry/sentry-javascript#6508). * Update the session replay setup page * Update the session replay onboarding wizard ref #getsentry/sentry-javascript#6326 Note: I'd like to keep this PR open for a few days (Monday or so) just to make sure 7.27.0 isn't breaking anything

billyvg · 2022-12-20T21:54:09Z

@Lms24 So now to test a local replays package, do we link @sentry/browser?

martinklepsch · 2023-03-01T13:14:13Z

Just as a data point: We use Sentry from ClojureScript, which comes with its own compiler and way of integrating npm packages into the bundle. Unfortunately the replay code is not omitted and this change added 130kb to our build.

While this is definitely a minority of the Sentry userbase I'd advocate for a more conservative approach to these types of decisions. This change saved a lot of people 1 line of code but adds 130kb to the bundle for people using less common (not anticipated) setups.

lforst · 2023-03-01T15:31:00Z

@martinklepsch I am gonna make the call here and say that if users care about build/bundle size, they should use tooling that does tree shaking and dead code elimination. The size of the npm module is not something we care about - CDN bundles are a different story since there is nothing users can do to reduce their size.

Lms24 force-pushed the lms-replay-export-from-browser branch 2 times, most recently from 044e434 to 41f9db7 Compare December 13, 2022 10:12

Lms24 marked this pull request as ready for review December 13, 2022 10:35

Lms24 force-pushed the lms-replay-export-from-browser branch from 53a28ea to 2591d2a Compare December 14, 2022 10:33

Lms24 added 4 commits December 14, 2022 11:34

feat(browser): Export Replay integration from Browser SDK

9aeb63e

wip

remove unnecessary plugin sort order modification

8c3b97e

skip files other than browser index

56f3932

Revert "remove unnecessary plugin sort order modification"

92661a6

This reverts commit 527f34d.

Lms24 force-pushed the lms-replay-export-from-browser branch from 2591d2a to 92661a6 Compare December 14, 2022 10:34

remove global regex flag

9ef6d3a

Lms24 requested review from mydea, AbhiPrasad, lforst and billyvg December 14, 2022 11:56

lforst reviewed Dec 14, 2022

View reviewed changes

Lms24 mentioned this pull request Dec 15, 2022

chore(build): Lower size-limit action limits #6543

Merged

Lms24 added 2 commits December 15, 2022 09:08

add plugin order comment

10ff1a7

use import type for BrowserClientReplayOptions

8552e6b

mydea approved these changes Dec 15, 2022

View reviewed changes

lforst approved these changes Dec 15, 2022

View reviewed changes

Lms24 merged commit 2c09d9a into master Dec 15, 2022

Lms24 deleted the lms-replay-export-from-browser branch December 15, 2022 12:19

Lms24 mentioned this pull request Dec 16, 2022

test: Add Replay dependency resolution to Remix and NextJS integration tests #6570

Merged

This was referenced Dec 16, 2022

e.getLastBreadcrumb is not a function. (In 'e.getLastBreadcrumb()', 'e.getLastBreadcrumb' is undefined) #6542

Closed

ref(replay): Update Replay installation with SDK-package import getsentry/sentry-docs#5971

Merged

kachkaev mentioned this pull request Dec 16, 2022

Exporting Replay from @sentry/nextjs #6574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(browser): Export `Replay` integration from Browser SDK #6508

feat(browser): Export `Replay` integration from Browser SDK #6508

Lms24 commented Dec 13, 2022 •

edited

github-actions bot commented Dec 13, 2022 •

edited

lforst left a comment

lforst Dec 14, 2022

mydea Dec 14, 2022

Lms24 Dec 14, 2022 •

edited

Lms24 Dec 15, 2022

lforst Dec 14, 2022

mydea Dec 14, 2022

lforst Dec 14, 2022

Lms24 Dec 15, 2022

mydea left a comment

billyvg commented Dec 20, 2022

martinklepsch commented Mar 1, 2023

lforst commented Mar 1, 2023

feat(browser): Export Replay integration from Browser SDK #6508

feat(browser): Export Replay integration from Browser SDK #6508

Conversation

Lms24 commented Dec 13, 2022 • edited

Bundle Size Concerns

github-actions bot commented Dec 13, 2022 • edited

size-limit report 📦

lforst left a comment

Choose a reason for hiding this comment

lforst Dec 14, 2022

Choose a reason for hiding this comment

mydea Dec 14, 2022

Choose a reason for hiding this comment

Lms24 Dec 14, 2022 • edited

Choose a reason for hiding this comment

Lms24 Dec 15, 2022

Choose a reason for hiding this comment

lforst Dec 14, 2022

Choose a reason for hiding this comment

mydea Dec 14, 2022

Choose a reason for hiding this comment

lforst Dec 14, 2022

Choose a reason for hiding this comment

Lms24 Dec 15, 2022

Choose a reason for hiding this comment

mydea left a comment

Choose a reason for hiding this comment

billyvg commented Dec 20, 2022

martinklepsch commented Mar 1, 2023

lforst commented Mar 1, 2023

feat(browser): Export `Replay` integration from Browser SDK #6508

feat(browser): Export `Replay` integration from Browser SDK #6508

Lms24 commented Dec 13, 2022 •

edited

github-actions bot commented Dec 13, 2022 •

edited

Lms24 Dec 14, 2022 •

edited