-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(replay): Allow to treeshake rrweb features #9274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11kb is impressive!
__SENTRY_DEBUG__: false, | ||
__RRWEB_EXCLUDE_CANVAS__: true, | ||
__RRWEB_EXCLUDE_SHADOW_DOM__: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm kind of unfortunate that we have inverse semantics here to the __SENTRY_DEBUG__
flag but i actually prefer the verbose EXCLUDE
naming pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was/am also thinking about this - but IMHO for an opt-in behavior it's nicer to set this to true
if I want to shake this out... 🤔 Maybe we can think about this for __SENTRY_DEBUG__
in v8 as well, make this __SENTRY_STRIP_DEBUG__: true
or something like this..
9d7a3f2
to
3b40785
Compare
rollup/bundleHelpers.js
Outdated
@@ -34,6 +35,11 @@ export function makeBaseBundleConfig(options) { | |||
const markAsBrowserBuildPlugin = makeBrowserBuildPlugin(true); | |||
const licensePlugin = makeLicensePlugin(licenseTitle); | |||
const tsPlugin = makeTSPlugin('es5'); | |||
const rrwebBuildPlugin = makeRrwebBuildPlugin({ | |||
excludeCanvas: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billyvg should we exclude canvas right now for CDN bundles? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah since we don't support playback. Makes sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
size-limit report 📦
|
77a96e5
to
26985d6
Compare
I've tried to see how big of a difference it will make in a nuxt module where I'm importing EDIT: Well, I'm not importing whole |
So these flags will only impact the build output if replay is used. If replay is not used, all of it should already be shaken out! |
Of course :) Thanks. And I see that what those do exactly is documented at https://docs.sentry.io/platforms/javascript/configuration/tree-shaking/ |
This depends on getsentry/rrweb#114 to be merged first, but allows to configure build time flags to shake out certain rrweb features that may not be used.
It also adds a size limit entry that shows the total bundle size with everything that can be shaken out removed, incl. debug stuff. Bundle size is about ~11kb gzipped less in this scenario, which is not bad.