Skip to content

Improve worker build for replays #6371

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

Closed
Tracked by #6459
mydea opened this issue Dec 1, 2022 · 2 comments
Closed
Tracked by #6459

Improve worker build for replays #6371

mydea opened this issue Dec 1, 2022 · 2 comments
Assignees
Labels
Package: replay Issues related to the Sentry Replay SDK Type: Improvement

Comments

@mydea
Copy link
Member

mydea commented Dec 1, 2022

Currently, the web worker for replay is built via yarn build:worker. This builds the worker script and stores it as a string in src/worker.js, which is then actually used.

While this works, it has some downsides:

  1. Since this is built & checked in, it is possible to have a different version of this locally/on CI/when building.
  2. You need to remember to run this every now and then locally & check the generated file in (if it changed)
  3. It is not ideal to have a two-step build process inside of replay (this is a bit custom and requires a bunch of custom/duplicate build stuff in there).

Some notes on what doesn't work

Sadly, just using something like https://github.com/darionco/rollup-plugin-web-worker-loader does not work as easily as expected.
While it is possible to get it to work, jest & jsdom do not like this. Basically, jsdom-worker only works with string-based workers, not with an actual js file reference. And it's also not really possible to do something like fs.readFileSync('worker.js') as that would not include all the dependencies etc.

Proposed solution

After some experimentation, we believe the best approach is to move the worker into a private sub-package, e.g. @sentry/replay-compression-worker. This package would build the worker into a single worker.js file, similar to what we have now, but:

  1. in a standalone package
  2. In a .js file, not a string

Then, replay can depend on this (and internally inline it at build time), and we can make it work with rollup-plugin-web-worker-loader (because there exists a fully built file that we can read from disk & inline).

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@mydea mydea reopened this Jan 3, 2023
@billyvg billyvg changed the title [Replay] Improve worker build Improve worker build Feb 15, 2023
@billyvg billyvg changed the title Improve worker build Improve worker build for replays Feb 15, 2023
@mydea mydea closed this as completed Feb 21, 2023
@Lms24
Copy link
Member

Lms24 commented Feb 27, 2023

This was addressed in #7139 (removing this issue from our board as a result)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK Type: Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants