fix(replay): fix rrweb issue & vendor it #6335
Conversation
mydea
requested review from
billyvg,
lforst,
Lms24,
lobsterkatie and
AbhiPrasad
| "tslib": "^1.9.3" | ||
| }, | ||
| "dependencies": { | ||
| "@sentry/core": "7.22.0", | ||
| "@sentry/types": "7.22.0", | ||
| "@sentry/utils": "7.22.0", | ||
| "lodash.debounce": "^4.0.8", | ||
| "rrweb": "^1.1.3" | ||
| "lodash.debounce": "^4.0.8" |
There was a problem hiding this comment.
We can probably think about also either inlining this, or actually replace it with some custom code. but we can tackle this later, I'd say!
There was a problem hiding this comment.
We have custom (kinda-)debounce logic in IdleTransaction and our Vue SDK (component tracking). So generally I'm +1 for replacing it with custom code. But as you said, that's definitely something to do for later.
size-limit report 📦
|
| @@ -89,6 +90,7 @@ | |||
| "mocha": "^6.1.4", | |||
| "nodemon": "^2.0.16", | |||
| "npm-run-all": "^4.1.5", | |||
| "patch-package": "^6.5.0", | |||
There was a problem hiding this comment.
This has to be a top-level dependency because rrweb is hoisted to the root node_modules directory, right?
There was a problem hiding this comment.
yes, this runs as postinstall when running yarn, so it needs to be on the very top. Also note that the patch itself is also at the root of the project!
| @@ -1,6 +1,7 @@ | |||
| import replace from '@rollup/plugin-replace'; | |||
| import typescript from '@rollup/plugin-typescript'; | |||
| import { defineConfig } from 'rollup'; | |||
| import resolve from '@rollup/plugin-node-resolve'; | |||
There was a problem hiding this comment.
No action required, just putting this here for future reference:
From the @rollup/plugin-node-resolve documentation:
resolveOnly: [...] An Array which instructs the plugin to limit module resolution to those whose names match patterns in the array. Note: Modules not matching any patterns will be marked as external
So iiuc, this is essentially syntactic sugar (EDIT: ok, more than syntactic sugar, as matching has to be performed) around adding everything to the external property in the rollup config except for the modules matching the patterns. I'm fine with using it as long as we can make it work with our rollup configs. In case that doesn't work, we at least know how to work around it.
There was a problem hiding this comment.
which is weird, as it wasn't marked as external before, right? But still. not included in the bundle 🤔 MAYBE we can drop the config option and only include resolve() (as it is also done in rollup.config.worker.ts), and then it just takes the config from external... I'll give it a try!
There was a problem hiding this comment.
Update: Yes, that works. So I will remove the resolveOnly config, so it just uses the defined externals!
mydea
force-pushed
the
fn/replay-fix-rrweb
branch
from
95e1852 to
b9b9281
Compare
|
Note: I tested this in a minimal vue+vite app, and the overall bundle size stayed more or less the same (it actually decreased by ~1kb with this branch). |
mydea
force-pushed
the
fn/replay-fix-rrweb
branch
13 times, most recently
from
9db6566 to
3e382cb
Compare
mydea
force-pushed
the
fn/replay-fix-rrweb
branch
from
3e382cb to
93f3220
Compare
As e.g. `rrweb` has a `node_modules` folder in the build dir.
mydea
force-pushed
the
fn/replay-fix-rrweb
branch
from
93f3220 to
b250c32
Compare
While debugging the replay issue getsentry/sentry-replay#297, we found that most likely the problem is upstream in rrweb. I created an issue there to fix it: rrweb-io/rrweb#1057
Really, the change is minimal, but the PR is not yet merged & even when it is merged it is not clear when this will be released. Even when it will be released, it will probably go into 2.0.0-alpha.X, which will need some time from us to actually update (as there are no upgrade docs yet etc.)
So, with that in mind, this PR does two things:
patch-packageto fix the exact issue we care about in rrwebDetails
1. Using
patch-packageto fix rrwebThis approach has two main benefits:
a) We can fix this issue very quickly
b) We still maintain the ability to easily update rrweb in the future
patch-package works at build-time via
postinstall, literally fixing the code in your node_modules folder.2. Inlining rrweb
The only downside of this, really, is that users cannot manually update/pin rrweb to any other version as the one we include. But I think that is acceptable for us. It will be on us to update rrweb whenever they ship fixes/new updates/improvements.
Without this,
patch-packagecould not actually work for users that install@sentry/replayvianpm install, as they would not get the patched version. So inlining this is basically a requirement for this to work.Closes getsentry/sentry-replay#297