-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add debugId sync APIs between web worker and main thread #16981
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
Conversation
dev-packages/browser-integration-tests/suites/integrations/webWorker/assets/worker.js
Dismissed
Show dismissed
Hide dismissed
ef4ba1a
to
3acf7a8
Compare
dev-packages/e2e-tests/test-applications/browser-webworker-vite/src/worker.ts
Dismissed
Show dismissed
Hide dismissed
@@ -28,7 +28,7 @@ function fixPackageJson(cwd: string): void { | |||
|
|||
// 2. Fix volta extends | |||
if (!packageJson.volta) { | |||
throw new Error('No volta config found, please provide one!'); | |||
throw new Error("No volta config found, please add one to the test app's package.json!"); |
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.
adjusted this message because I didn't realize the volta config needed to be added to the e2e test apps. Might be a me-problem but I think the message is now fool-proof :D
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.
LGTM.
What happens if you have more than one worker and try and add them both? If you add the integration twice will it remove the one you added first?
Good point, thanks for raising! The first installed integration wins and the second instance wouldn't be added (=> setupOnce wouldn't be called and debugIds not added). I think we have a couple of options:
I'm tentatively tending towards 2 but don't yet have a strong opinion. Any thoughts on this? Will also take it to the team to discuss shortly. (set the PR to draft in the meantime) |
Another possibly more hacky solution is to set a different name for each instance of the Integration: let count = 0;
export const webWorkerIntegration = defineIntegration(({ worker }: WebWorkerIntegrationOptions) => ({
name: `${INTEGRATION_NAME}-${count++}`,
setupOnce: () => { Since this integration isn't default included users wont need to use the name to filter it out and because the name differs, multiples can be added? |
dev-packages/e2e-tests/test-applications/browser-webworker-vite/src/worker2.ts
Dismissed
Show dismissed
Hide dismissed
dev-packages/e2e-tests/test-applications/browser-webworker-vite/src/worker3.ts
Dismissed
Show dismissed
Hide dismissed
I went with options 1 and 2 combined. Decided to do it "properly" instead of the "hack" (though I liked the suggestion) to avoid the side effect it would introduce and to keep the integration name constant. Not sure why users would look up or remove the integration but let's keep the integration names predictable. |
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.
Looks good!
This PR adds two Browser SDK APIs to let the main thread know about debugIds of worker files:
webWorkerIntegration({worker})
to be used in the main threadregisterWebWorker(self)
to be used in the web workerThe communication between workers and main thread is established between both APIs and they have to be used both for the sync to work correctly. Another limitation around this approach is that users must set up
webWorkerInegration
before they register their own message listeners. This ensures that the message fromregisterWebWorker
is not propagated to user-created message listeners. We'll document this thoroughly in docs and I already added a section about this in the JSDoc.Because of the strict co-dependence of both APIs, I decided to add them in one PR (also makes testing easier).
Usage
Multiple Workers
Multiple workers are also supported:
A worker can also be passed to the integration after it was initialized. This is helpful if workers are initialized lazily or at different times:
I decided to keep both APIs very general around web workers because I think there could be other use cases in which we can sync data by using the worker's message channels. This should be as easy as listening for other messages.
also added
webWorkerIntegration
closes #16975
closes #16976