Retrieves worker client from "ServiceWorkerGlobalScope" instead of "FetchEvent.target"

[kettanaito]

GitHub

• Fixes fetchEvent.respondWith Receives an Error in Safari Only #558

Motivation

msw/src/mockServiceWorker.js

Line 193 in af0dada

const client = await event.target.clients.get(clientId)

FetchEvent.target equals null in Safari only. In other browsers FetchEvent.target equals self (ServiceWorkerGlobalScope) instance. Here's a snapshot of the entire "fetch" event scope in Safari:

This pull request substitutes:

-event.target.clients.get(clientId)
+this.clients.get(clientId)

Since clientId comes from the FetchEvent.clientId and is persistent in all browsers, it matters little which ServiceWorker global scope resolves it.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b663d8c:

Sandbox	Source
MSW React	Configuration

[marcosvega91]

Because we don't have a way to test it, we can add a comment referring to the issue to remember in the future why this line has moved outside. What do you think?

[kettanaito]

I think that reason can be looked up in Git commit history. Now I’m wondering if we can test this somehow... the issue happened only on Safari.

[marcosvega91]

Yes I know and I think that we can't run puppeteer in Safari. I know that it can be found in git history but because we don't have a test for it, in case of change, it will not break anything. Of course, if we find a way to test it comment the line will be not very helpful

[kettanaito]

I'll try to analyze what exactly goes different in Safari and see if we can simulate that scenario in a test.

[kettanaito]

I was unable to create a test suite for this. event.target is null in Safari only, I don't see how I can simulate that in a test. I've tried issuing a request from ClientA, then closing that client, and observing what happens in ClientB controlled by the same worker. I'd expect that since the client was closed, the event.target or at least evet.target.clients.get will be null, but it's not. At least, it doesn't throw an exception.

[marcosvega91]

Yes, this morning I have tried to take a look at the issue but no found a way to test too. The only thing that I Have found is that the first time target is not null. It will become null after the first await event.target.clients.get(clientId)

[marcosvega91]

is ready for me :)

[timdeschryver]

This also LGTM.
I wonder if we can do the same at const client = await event.currentTarget.clients.get(clientId) when we receive a message. Because this could potentially also provide a fix for #536?
Since the linked issue (#536) now includes a reproduction, I'll try again to reproduce that behavior in our test suite.

[kettanaito]

@marcosvega91, @timdeschryver thank you for the review, gentlemen!

@timdeschryver that'd be a great fix indeed. I suggest we merge this and see if your suggestion could eliminate the issue. Eager to hear more about your findings.