-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Export Replay
integration from Browser SDK
#6508
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
Changes from all commits
9aeb63e
8c3b97e
56f3932
92661a6
9ef6d3a
10ff1a7
8552e6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/** | ||
* Options added to the Browser SDK's init options that are specific for Replay. | ||
* Note: This type was moved to @sentry/types to avoid a circular dependency between Browser and Replay. | ||
*/ | ||
export type BrowserClientReplayOptions = { | ||
/** | ||
* The sample rate for session-long replays. | ||
* 1.0 will record all sessions and 0 will record none. | ||
*/ | ||
replaysSessionSampleRate?: number; | ||
|
||
/** | ||
* The sample rate for sessions that has had an error occur. | ||
* This is independent of `sessionSampleRate`. | ||
* 1.0 will record all sessions and 0 will record none. | ||
*/ | ||
replaysOnErrorSampleRate?: number; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,16 @@ | |
* Typescript plugin docs: https://github.com/ezolenko/rollup-plugin-typescript2 | ||
*/ | ||
|
||
import path from 'path'; | ||
|
||
import commonjs from '@rollup/plugin-commonjs'; | ||
import deepMerge from 'deepmerge'; | ||
import license from 'rollup-plugin-license'; | ||
import resolve from '@rollup/plugin-node-resolve'; | ||
import replace from '@rollup/plugin-replace'; | ||
import { terser } from 'rollup-plugin-terser'; | ||
import typescript from 'rollup-plugin-typescript2'; | ||
import MagicString from 'magic-string'; | ||
|
||
/** | ||
* Create a plugin to add an identification banner to the top of stand-alone bundles. | ||
|
@@ -164,6 +167,37 @@ export function makeTSPlugin(jsVersion) { | |
return plugin; | ||
} | ||
|
||
/** | ||
* Creates a Rollup plugin that removes all code between the `__ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__` | ||
* and `__ROLLUP_EXCLUDE_FROM_BUNDLES_END__` comment guards. This is used to exclude the Replay integration | ||
* from the browser and browser+tracing bundles. | ||
* If we need to add more such guards in the future, we might want to refactor this into a more generic plugin. | ||
lforst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided to pull this out into its own PR so we have a better history as to why we made this change: #6543 |
||
|
||
const plugin = { | ||
transform(code, id) { | ||
const isBrowserIndexFile = path.resolve(id) === browserIndexFilePath; | ||
if (!isBrowserIndexFile || !replacementRegex.test(code)) { | ||
return null; | ||
} | ||
|
||
const ms = new MagicString(code); | ||
const transformedCode = ms.replace(replacementRegex, ''); | ||
return { | ||
code: transformedCode.toString(), | ||
map: transformedCode.generateMap({ hires: true }), | ||
}; | ||
}, | ||
}; | ||
|
||
plugin.name = 'excludeReplay'; | ||
|
||
return plugin; | ||
} | ||
|
||
// We don't pass these plugins any options which need to be calculated or changed by us, so no need to wrap them in | ||
// another factory function, as they are themselves already factory functions. | ||
export { resolve as makeNodeResolvePlugin }; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2232,7 +2232,7 @@ | |
resolved "https://registry.yarnpkg.com/@jridgewell/set-array/-/set-array-1.1.2.tgz#7c6cf998d6d20b914c0a55a91ae928ff25965e72" | ||
integrity sha512-xnkseuNADM0gt2bs+BvhO0p78Mk762YnZdsuzFV018NoG1Sj1SCQvpSqa7XUaTam5vAGasABV9qXASMKnFMwMw== | ||
|
||
"@jridgewell/[email protected]", "@jridgewell/sourcemap-codec@^1.4.10": | ||
"@jridgewell/[email protected]", "@jridgewell/sourcemap-codec@^1.4.10", "@jridgewell/sourcemap-codec@^1.4.13": | ||
version "1.4.14" | ||
resolved "https://registry.yarnpkg.com/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.14.tgz#add4c98d341472a289190b424efbdb096991bb24" | ||
integrity sha512-XPSJHWmi394fuUuzDnGz1wiKqWfo1yXecHQMRf2l6hztTO+nPru658AyDngaBe7isIxEkRsPR3FZh+s7iVa4Uw== | ||
|
@@ -16502,6 +16502,13 @@ magic-string@^0.26.2: | |
dependencies: | ||
sourcemap-codec "^1.4.8" | ||
|
||
magic-string@^0.27.0: | ||
version "0.27.0" | ||
resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.27.0.tgz#e4a3413b4bab6d98d2becffd48b4a257effdbbf3" | ||
integrity sha512-8UnnX2PeRAPZuN12svgR9j7M1uWMovg/CEnIwIG0LFkXSJJe4PdfUGiTGl8V9bsBHFUtfVINcSyYxd7q+kx9fA== | ||
dependencies: | ||
"@jridgewell/sourcemap-codec" "^1.4.13" | ||
|
||
make-dir@^1.0.0: | ||
version "1.3.0" | ||
resolved "https://registry.yarnpkg.com/make-dir/-/make-dir-1.3.0.tgz#79c1033b80515bd6d24ec9933e860ca75ee27f0c" | ||
|
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.
Just wondering: Why are we moving this type into the types package?
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.
So that @sentry/replay does not need to import anything from @sentry/browser, to avoid circular dependencies.