Worker registered on different ports in Testcafe

[benmonro]

Describe the bug

i am now using msw in testcafe w/ no problems as of 0.20.4 (was thinking I should add a recipe for this to your docs, if you want let me know and I'll open a PR).

However, when i upgrade to 0.20.5, it appears that mocking is not working.

Environment

• msw: 0.20.4 & 5
• nodejs: 12
• yarn: latest

Please also provide your browser version.
chrome latest

To Reproduce

Steps to reproduce the behavior:

1. clone this branch of my repo: https://github.com/benmonro/msw-server/tree/msw-error
2. yarn
3. yarn testcafe
   note, it works w/ this version as it's using 0.20.4
4. yarn add -D msw@0.20.5
5. see that tests fail, MSW doesn't register, network requests all fail.

Expected behavior

A clear and concise description of what you expected to happen.
mocks work. :)

Screenshots

If applicable, add screenshots to help explain your problem.

[msutkowski]

Hi - random person here. Just a few notes on this - I was trying to see if any of the changes I helped introduced into the 0.20.5 release impacted this. I had to remove yarn.lock from ./app, as well as yarn start before running the tests.

The current issue with this is that it was actually broken(maybe?) pre 0.20.5? The problem is in getWorkerByRegistration - testCafe is looking at port :1337, and the app itself is running in :3000.

Script URL: https://localhost:1337/mockServiceWorker.js
absoluteWorkerUrl: http://localhost:3000/mockServiceWorker.js

I'm working on seeing if I can solve it quickly, but that's the starting info.

[benmonro]

@msutkowski (aka Random person). actually that's the correct port. testcafe runs tests through a proxy. you'll notice it uses the same port w/ no problems on 0.20.4. that's what the file located at testcafe/registerServiceWorker.js is doing, it ensures that MSW is properly registered as a service worker w/ the correct URL w/out the proxy stuff that testcafe does.

[msutkowski]

@benmonro Understood! What I was trying to say is that the absoluteWorkerUrl and worker.scriptURL have to match with 0.20.5. Previously, this wasn't actually enforced and would've called worker.start() on whatever serviceWorker was registered whether it was msw or not, and could've been unintended behavior for sure (ex: if you registered two workers at once for whatever reason, it never would've worked assuming msw was registered second).

Here is a temporary solution:

        worker.start({
          serviceWorker: {
            url: "https://localhost:1337/mockServiceWorker.js"
          }
        })

in your registerServiceWorker.js:

if (!url.includes("mockServiceWorker")) {
    return getProxyUrl.apply(this, arguments);
  }

I'd have that serviceWorker.url definition be defined by the environment. The only thing that's strange about this is that once it moves to the 2nd test, the bundle gets chewed up? I'm not sure of what's causing this, but it looks like this:

I'm going to spend a few more minutes on seeing if I can figure out what this issue is, but it has nothing to do with the proxy/msw from what I can tell. I'm in the reactiflux discord under the same user name if you want to look at this together - would be happy to jump on a liveshare or whatnot with a fellow san diegan 🎉 . If not, I'll update what I find here (if anything).

[benmonro]

would love to what's the url for the discord?

[msutkowski]

https://discord.gg/reactiflux - i'm normally active in the #redux channel, but i'm the only msutkowski on there.

[benmonro]

damn ok i'm a dummy how do i find you on here lol, i'm benmonro

[msutkowski]

Just wanted to leave a note where we left off here: the issue is basically getting the proxy, service worker registration, and test cafe to all play nicely. The reason this worked previously is that the check on worker.scriptURL and absoluteWorkerUrl wasn't 'strict'. The reason it's 'breaking' now is that these are different - one is registed on the proxy port, while the other is on the port react runs on (3000 vs 1337). Current options: hack around test cafe some more to see if there is a way to get it's config options to not chew things up like in the screenshot above, or by introducing an additional configuration parameter to bail out of the strict scriptURL check.

Going to tinker more tomorrow and see if we can come up with a good idea for this unless someone might have other thoughts. Things we discussed: 1) adding a strict flag that someone could set to false to just match on the service worker file name like mockServiceWorker.js instead of https://hostname:port/filename.js. 2) allowing a proxyURL param that would be checked along with absoluteWorkerUrl if specified.

[benmonro]

@msutkowski thanks again for following up and spending time. I think a 3rd option we discussed was to treat the default as non strict mode too? To be clear I'm totally fine with all 3 options though I'd probably lean towards option 1.

[kettanaito]

Thanks for the insights on this, @msutkowski!

One thing I fail to understand is why it resolves absolute worker URL to port 3000, when the connection is established at the proxy port. Does this mean the absolute URL (and the MSW itself) is evaluated before the proxy is prepended?

If a worker script URL is different, I would suggest to specify an explicit worker registration URL in the options to worker.start:

worker.start({
  serviceWorker: {
    url: "https://localhost:1337/mockServiceWorker.js"
  }
})

This is not a workaround, you are specifying where to register and expect the worker. If this approach solves the issue, I'd choose it.

[benmonro]

It doesn't solve it though because then it'll won't resolve when the page goes to download the script. It actually creates a different problem.

[msutkowski]

@kettanaito It's a confusing situation and I think @benmonro can definitely explain it better than me but I'll try to shed some light: basically, setting the serviceWorker.url does work, but test cafe overwrites URLs with its proxy hash (which is what caused the screenshot I attached above) - so to me, it seems like it's primarily a configuration issue and managing test cafe side effects. Ideally, we'd find an 'optimal' way to fix the URL rewriting on the test cafe end because msw is technically doing the right thing here, but I'm not sure if that's exactly possible - which is why we discussed adding an escape hatch with the loose check/proxyURL options. I'll be able to spend some more time on this later today if no one beats me to it :).

[benmonro]

Yeah, so in a nutshell, testcafe controls everything through a proxy server it sets up to run your tests called 'hammerhead.' Hammerhead allows testcafe to control the page during a test in the browser directly. But yeah a side effect of this is that they have to change all urls to go through their proxy to avoid CORS issues and other such problems. So that's why we discussed option 2 above, a proxyUrl config option as a way to identify if msw is being served through a proxy. The problem is if we just set serviceWorker.url to the testcafe url (testcafe has an api window["%hammerhead%].utils.url.getProxyUrl), then hammerhead will not be able to resolve that itself (it still communicates with your server on port 3000 in this example).

So to summarize how I got testcafe & msw to play nice together:

1. my app serves /mockServiceWorker.js from port 3000.
2. hammerhead will serve this from http://localhost:1337/<hash>/http://localhost:3000/mockServiceWorker.js
3. in a testcafe client script I am making sure that MSW is registered as a service worker on http://localhost:1337/mockServiceWorker.js (even though it's served from the url in 2)

This allows MSW to be registered at the correct URL to intercept fetches to /foo for example.

So if we set serviceWorker.url to the URL in 2) then hammerhead will actually try to fetch from that (on the backend) and it will be a circular reference that won't be resolved, so MSW never get's injected into the page.

That's why either a proxyUrl or strict:false flag would solve this problem as then everything in steps 1-3 would still happen, but in MSW the check against the absoluteUrl OR a proxyUrl would still pass and MSW would be injected & flow through 1-3 again.

hopefully that makes sense. :)

[kettanaito]

Thank you for clarifying on this, @msutkowski, @benmonro.

I see now. Since previously we were less strict in resolving the worker registration, we mistakingly disregarded a worker URL, which allowed MSW to pick up a worker registration at a different URL than the current location. This is what made it work in the previous versions, but fails now with the strict worker registration check.

That being said, I'd treat the current registration retrieval logic as expected behavior:

msw/src/setupWorker/start/utils/getWorkerByRegistration.ts

Lines 5 to 20 in a126de2

	export const getWorkerByRegistration = (
	registration: ServiceWorkerRegistration,
	absoluteWorkerUrl: string,
	): ServiceWorker | null => {
	const allStates = [
	registration.active,
	registration.installing,
	registration.waiting,
	]
	const existingStates = allStates.filter(Boolean) as ServiceWorker[]
	const mockWorker = existingStates.find((worker) => {
	return worker.scriptURL === absoluteWorkerUrl
	})
	return mockWorker || null
	}

The fact that we used to resolve with a registration that was not necessarily the one you provisioned in your app was a bug, and thus a patch version release. I would be careful in introducing new options to cover this, for them not to be workarounds for the sake of unexpected behavior support.

@benmonro is it possible to configure Testcafe to register the worker at http://localhost:1337/<hash>/http://localhost:3000/mockServiceWorker.js (in the client script)? Since the only reason a workaround was introduced was to intercept root-level requests like /foo, I fail to understand why it resolves service worker as http://localhost:1337/<hash>/http://localhost:3000/mockServiceWorker.js, but a root-level request http://localhost:1337/foo. I'd expect it to be http://localhost:1337/<hash>/http://localhost:3000/foo, since the entire proxy URL + your original URL is the root of the application.

I'm sorry if I can't be of more help in this question, since the entire proxy setup of Testcafe introduces much bigger problems that the ones it tries to solve. I can yet again recommend to dive into their setup and find a way to register workers and handle root-level requests as a regular app does.

[benmonro]

@kettanaito would you be opposed to option 1 or 2 above?

[msutkowski]

@benmonro Can you try something out for me? I found a way to 'trick' hammerhead... but first, please don't laugh at how ridiculous this is 😅

_app.js
const absoluteWorkerUrlParts = ['https',':','//','localhost:1337','/','mockServiceWorker.js']
        
       worker.start({
          serviceWorker: {
            url: absoluteWorkerUrlParts.join('')
          }
        }).then(() => {
          console.log('msw is ready!', navigator.serviceWorker.getRegistration().then(res => console.log('Got reservations:', res)))
          
          setMswReady(true)
        });

registerServiceWorker.js
const getProxyUrl = window["%hammerhead%"].utils.url.getProxyUrl;

window["%hammerhead%"].utils.url.getProxyUrl = function () {
  // "localhost:1337/<sha>/http://localhost:3000"
  // fetch(/todos)
  const url = arguments[0];
  if (!url.includes("/mockServiceWorker")) {
    return getProxyUrl.apply(this, arguments);
  }

  return url;
};

I observed that hammerhead is basically just regex-ing strings or URL instances and rewriting the strings in place (maybe!), not executing functions, and then rewriting the resulting string... so it seems like building the URL will sidestep that rewriting stuff it does. I could be totally offbase here, but this is working for me on both of my machines.

I ran this with the --debug-mode flag and without - both were successful. Even though this should work, it still seems brittle? Let me know what you think.

[benmonro]

Lol sure will try later tonight. Still feels like a proxy URL or strict mode option would be a better option but I will try this when I get home

[msutkowski]

I looked at this a bit more. Although we can make it work like this, I think a discussion about a strict check of worker.scriptURL === absoluteWorkerUrl is warranted. I think the strict should be the default, but allowing it to be set to false would be a good escape hatch for people that run into this scenario. The definition of that would be that it would just match on the service worker file name, and disregard the rest of the URL schema. That'd be more flexible in the case someone doesn't know the port for their proxy or whatnot in advance. The only potential negative I can think of would be the possibility of having two workers with the same name, but I don't really ever see that happening.

Code changes would look like:

const getWorkerByRegistration = (registration, absoluteWorkerUrl, strict = true) => {
    const allStates = [
        registration.active,
        registration.installing,
        registration.waiting,
    ];
    const existingStates = allStates.filter(Boolean);
    const mockWorker = existingStates.find((worker) => {
        if (strict) {
            return worker.scriptURL === absoluteWorkerUrl;
        } else {
            const workerFileName = absoluteWorkerUrl.split('/').pop();
            return worker.scriptURL.includes(workerFileName)
        }
    });
    return mockWorker || null;
};

And we'd just have to add a config option to DEFAULT_START_OPTIONS.

I did test the above code with @benmonro 's project, and it works fine. Prevents the need to specify the serviceWorker.url to worker.start as intended and get's rid of dealing with hammerheads overwriting behavior. I don't know if this is the best option, but it's feasible.

Happy to open a PR and add docs if you're interested.

[benmonro]

here's maybe a 4th option. if we pass in an absolute URL in the config options, use the ===. if we pass in a relative path, just use includes. Thoughts? @msutkowski @kettanaito

[benmonro]

yeah i'm totally fine w/ the strict option as well.

[msutkowski]

I think your 4th option is achieving basically the same thing as the strict option, but it obscures the behavior a little bit. IMO, explicitly stating, "hey, I know the standard behavior is to match the exact script url, but in this case, I'm going to tell you to disregard that and just use the worker file name". Happy to hear everyone's thoughts in general - maybe we're missing something and there is actually an easier way to solve this :)

[benmonro]

all good, i just throw ideas out, i agree, let's go w/ strict mode! works for me

[benmonro]

oh btw @msutkowski that array.join hack totally works. LOVE IT (but yeah no) hahahaha. funny story, today at work my coworker got it to work using unicode characters haha. but then we tried it in prod mode and it failed, we're guessing because webpack optimizes that out.

[msutkowski]

I threw up a quick draft of this idea with more understandable naming to help the discussion. I'm still not 100% sold on the idea that msw needs to solve this problem as we've proved it can be handled without lib changes. That being said, I don't think the escape hatch is a bad idea as I'm sure other folks will run into similar situations.

[benmonro]

Yeah in my view, msw should offer a decent solution for testcafe users. In my view your PR is a perfectly reasonable option for that.

[msutkowski]

Yea, I agree with you. I've thrown up a matching PR into the docs site as well for this flag assuming it moves forward. If anyone has any additional feedback, let me know and I'd be happy to address it.

[benmonro]

@kettanaito any thoughts on @msutkowski 's approach and PR? Would really like to see it go in

[kettanaito]

@benmonro hey. The current state of the PR is great, I've asked to verify the error message, which I'd be thankful for you to do as well, if you have a minute. If the error message is okay, and the CI is green, it's approved.

[msutkowski]

I'm working on verifying this now - I have to update the example repo to be compatible with the new types before it'll run. Will update shortly.

[benmonro]

Sgtm. I trust Matt.