-
-
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): Add ReplayCanvas
integration
#9826
Conversation
size-limit report 📦
|
Hmm, turns it this seems to not be tree shakeable automatically 😬 maybe this is related to getsentry/rrweb#141, though, I wonder 🤔 |
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.
What's the reason for solving this via an integration? Bundle size? Right now it only sets options, right?
|
||
const mergedExperimentsOptions = { | ||
...this._initialOptions._experiments, | ||
...additionalOptions._experiments, |
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.
Are we excluding _experiments
from being mangled? I guess we do but this broke CDN bundles before so we should double check 😅
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.
Oh wait, this is just a property this should be safe, no?
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, this is excluded from being mangled! as we rely on this for some things 😅
@@ -0,0 +1,52 @@ | |||
import { getCanvasManager } from '@sentry-internal/rrweb'; |
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.
Oh it's for this, right? Only if the integration is used, the canvas manager will be added to the bundle
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.
Yes, exactly - basically wrapping this, but providing a nicer way to add this than to ask people to pass some method into the replay config etc.
Also, it helps with CDN bundles, where this otherwise also very hard/impossible to solve 😬
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.
Will we have any issues creating this bundle because rrweb is devDep?
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.
should not be the case, but something is not quite right yet here 😅 so still need to look into it a bit more!
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.
This seems great to me from a DX standpoint!
@@ -0,0 +1,52 @@ | |||
import { getCanvasManager } from '@sentry-internal/rrweb'; |
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.
Will we have any issues creating this bundle because rrweb is devDep?
packages/replay/src/canvas.ts
Outdated
this._canvasOptions = { | ||
fps: 4, | ||
quality: 0.6, | ||
...options, | ||
}; |
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.
In the future we'll want to control this a bit more (e.g. preconfigure quality settings like low, med, high).
3230bc7
to
fa6d477
Compare
We've been using esbuild (default, from`@size-limit/preset-small-lib`) for most jobs, but webpack for the one with custom config, which is a bit inconsistent. This now updates this to always use webpack, which should be more consistent... If we eventually get rid of the custom webpack config (e.g. when we merge this or something like it: #9826), we can revert this back to use esbuild everywhere.
fa6d477
to
0940301
Compare
packages/replay/src/canvas.ts
Outdated
// TODO FN: Allow to configure this | ||
// But since we haven't finalized how to configure this, this is predefined for now | ||
// to avoid breaking changes | ||
this._canvasOptions = { | ||
fps: 4, | ||
quality: 0.6, | ||
}; |
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.
I'm going to update this and move the following over to this integration: https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay/src/replay.ts#L348
hmm, still seems to not tree shake :( |
Adding this integration in addition to `Replay` will set up canvas recording.
66bc1a3
to
1044586
Compare
Closing in favor of #10112 |
This bump contains the following changes: - fix(rrweb): Use unpatched requestAnimationFrame when possible [#150](getsentry/rrweb#150) - ref: Avoid async in canvas [#143](getsentry/rrweb#143) - feat: Bundle canvas worker manually [#144](getsentry/rrweb#144) - build: Build for ES2020 [#142](getsentry/rrweb#142) Extracted out from #9826 Closes #6946
Adding this integration in addition to
Replay
will set up canvas recording.This is a bit hacky because interacting between multiple integrations is not really great/easy. But I think it is the best user experience we can provide there, AND it also provides a direct way to use this with CDN bundles as well - users just have to add the
replaycanvas
integration bundle in addition to the regular browser+replay bundle and it should just work.