feat(wasm): initialised sentryWasmImages for webworkers#18812
feat(wasm): initialised sentryWasmImages for webworkers#18812andreiborza merged 19 commits intogetsentry:developfrom
Conversation
|
Thanks for the contribution @harshit078. Are you done working on this or are you done (asking since it's a draft). |
|
Hey @andreiborza , yes I have finished working on this PR and converted it to review. |
|
working on comments addressed by cursor ! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
dev-packages/browser-integration-tests/suites/wasm/webWorker/test.ts
Outdated
Show resolved
Hide resolved
dev-packages/browser-integration-tests/suites/wasm/webWorker/assets/worker.js
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/wasm/webWorker/assets/worker.js
Dismissed
Show dismissed
Hide dismissed
|
Hey @andreiborza , the PR is ready to review. |
|
@harshit078 awesome, thanks! I'll take a look. |
dev-packages/browser-integration-tests/suites/wasm/webWorker/assets/worker.js
Dismissed
Show dismissed
Hide dismissed
| /** | ||
| * Registers a WASM module and forwards its debug image to the parent thread. | ||
| */ | ||
| function registerModuleAndForward( |
There was a problem hiding this comment.
m: I think we should call registerModule form registry.ts in here, that way we get some deduplication going when needed. It'll also get rid of this almost identical code duplication.
We'll need to adapt registerModule to return the image so we can post it in this method.
There was a problem hiding this comment.
agreed, I must have not taken a deep look into this module while implementing. I'll replace it with registerModule
| @@ -0,0 +1,86 @@ | |||
| // This worker manually just replicates what the actual Sentry.registerWebWorkerWasm() does | |||
There was a problem hiding this comment.
m: Could we highlight in the comment why we are doing that as opposed to actually using Sentry. registerWebWorkerWasm() please?
|
@harshit078 thanks again. Just had a look, looks mostly fine to me. Just a minor comment around code duplication. |
andreiborza
left a comment
There was a problem hiding this comment.
Awesome, thanks so much for working on this!
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #18779