Skip to content
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

fix: service worker communicates with the latest client only #573

Merged
merged 2 commits into from
Mar 24, 2024

Conversation

vladetsky
Copy link
Contributor

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

For some reason service worker is set to send messages to the latest client only. This leads to error finding instance, when user opens several tabs in the browser and switches not to the latest tab. More description here.

Use cases and why

Open several tabs in the browser with the same site. Partytown should run with service worker, not Atomics.
Switch to the first tab. Now anytime web worker tries to send anything to the main thread, you'll get error finding instance. The problem is in below code, where service worker is set to send messages to the latest tab only.

const sendMessageToSandboxFromServiceWorker = (accessReq: MainAccessRequest) =>
  new Promise<MainAccessResponse>(async (resolve) => {
    const clients = await (self as any as ServiceWorkerGlobalScope).clients.matchAll();
    const client = [...clients].sort((a, b) => {
      if (a.url > b.url) return -1;
      if (a.url < b.url) return 1;
      return 0;
    })[0];

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link

vercel bot commented Mar 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
partytown ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 22, 2024 0:43am

@techfg
Copy link
Contributor

techfg commented Mar 23, 2024

@vladetsky - Awesome to see this get fixed, thank you! I've confirmed that all of the tests added in #493 which adds tests for #492 pass with your fix applied. Once this PR is merged, I'll adjust #493 so it can be merged which will add some additional test coverage for your fix.

@vladetsky
Copy link
Contributor Author

@vladetsky - Awesome to see this get fixed, thank you! I've confirmed that all of the tests added in #493 which adds tests for #492 pass with your fix applied. Once this PR is merged, I'll adjust #493 so it can be merged which will add some additional test coverage for your fix.

Thanks for testing this fix @techfg !

@gioboa
Copy link
Member

gioboa commented Mar 24, 2024

Thanks @vladetsky @techfg for your help

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vladetsky for this great improvement 👏

@tibineagu
Copy link

tibineagu commented Mar 25, 2024

@vladetsky unfortunately I think this might still be happening.

Please check out tibineagu/partytown-instance-error - it's using 0.10.1 which should include your fix.

I tried to keep the repro as simple as possible. The proxied function simply gets and sets from localStorage.

As soon as another tab is open, the errors start popping up again.

@vladetsky
Copy link
Contributor Author

@vladetsky unfortunately I think this might still be happening.

Please check out tibineagu/partytown-instance-error - it's using 0.10.1 which should include your fix.

I tried to keep the repro as simple as possible. The proxied function simply gets and sets from localStorage.

As soon as another tab is open, the errors start popping up again.

@tibineagu You have an outdated inline script here. Please update it to 0.10.1 and check.

@tibineagu
Copy link

@vladetsky oh my god you're right - i must have copied that from the wrong place.

Works as expected with the snippet code from lib/partytown.js

Sorry about the mix-up! 😬

@vladetsky
Copy link
Contributor Author

@vladetsky oh my god you're right - i must have copied that from the wrong place.

Works as expected with the snippet code from lib/partytown.js

Sorry about the mix-up! 😬

No worries @tibineagu , thanks for testing!

@fenny-19
Copy link

I am using latest version that is 0.10.2 version but still I am getting Error finding instanceID on window 1, can anyone guide me how to resolve that issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants