Msw: Stopping and starting MSW will cause multiple "instances"

[bjarkehs]

Describe the bug

I have a use case where only I start MSW when user selects "development" data. First time it works fine, but if the user logs out and in again, i.e. stopping and starting MSW, then now it'll return twice requests twice. It will also write "Mocking enabled" twice. Basically every time I stop and start MSW it adds an extra instance.

Environment

• msw: 0.19.3
• nodejs: 13.11.0
• yarn: 1.22.4

Microsoft Edge: Version 83.0.478.50

To Reproduce

• Start a worker.
• Call .stop() on that worker.
• Start a new worker.

Expected behavior

Expected mocking to only be enabled once

[kettanaito]

Hey, @bjarkehs! Thank you for reporting this. That's definitely not the expected behavior.

I've also experienced a similar issue when doing a code change that produces a Fast Refresh update chunk, while preparing a NextJS usage example. I'd be great if these two issues have the same origin, as their manifestations are almost identical.

[marcosvega91]

I was looking into this and the problem is that when starting a new worker the instance remains the same( same clientId) so the postMessage is handled by the stopped instance and the new one.
I think that maybe we can remove handlers when we stop the insrance. What do you think?

[kettanaito]

Looks interesting. Do I understand it correctly, that we are talking about this listener?

https://github.com/mswjs/msw/blob/219ca18b538957e6e44f4444f87ece528ceffbe3/src/setupWorker/start/utils/activateMocking.ts#L12-L32

It should also affect the listener responsible for response resolution, as the issue reports responses being sent twice as well.

@marcosvega91, I think it's a good idea to remove all listeners from a worker instance once called worker.stop(). Would you be interested in making a proof of concept that it solves the issue?

[marcosvega91]

Yes of course this listener but it will only print to console. So it's a problem but not the real problem.
I think that the most important is the part below, because it is responsible for fetching :https://github.com/mswjs/msw/blob/219ca18b538957e6e44f4444f87ece528ceffbe3/src/setupWorker/start/createStart.ts#L37-L39

Maybe we could use sendToClient with a type MOCK_DEACTIVATE and intercept the message where we add listeners and remove them.

[kettanaito]

I see. When a client is closed (including refresh), it signals the closed event to the worker. That way worker checks if there are any more active client left, and if none, unregisters itself. This means it should be impossible to have those listeners persist between refreshes (each refresh = new unique client ID, and new registration flow).

The issue described here is happening on runtime. That is possible, as any hot update chunk will execute the .start() logic again, and since the unload event of the client wasn't fired, it will simply attach another listener. In that case, it should somehow know to remove previous listeners.

I believe we should tackle this issue from the HMR point of view, not necessarily taking the .stop() call as a pre-requisite to reproduce this issue. For example, if you try the NextJS example, edit and save _any_ file, you'll observe a similar (same?) issue, with logs and requests being duplicated. Generally, it's multiplied by N, where N is the number of hot updates received since the initial load.

Tackling HMR scenario is much harder, since it's a runtime code application, rather than an explicit developer's interaction with the library's API. I'm thinking if it would be possible to have something like a custom hot reload mechanism for the worker instance?

if (module.hot) {
  if (module.hot.accepts(/* Really anything at this point */, () => {
    // Pseudo-code. Prune all attached event listeners. 
    // Execute the `setupWorker` file as a part of a hot reload chunk
    // will trigger the logic that attaches the listeners anew.
    navigator.serviceWorker.removeAllListeners()
  })
}

[marcosvega91]

I have tried nextjs example but I don't see the problem there, I have only one log for [MSW] Mocking enabled.
Hot reload should be good if we work locally but how we can handle manual stop and start of the worker?

We should add two logic maybe:

1) as you said hot-reload should remove all listeners
2) stop() should remove listeners too

[kettanaito]

@marcosvega91, I think you're completely right. We can even ship those two separately, if it makes sense to you. Please, would you be interested in helping us with either of those? That'd be awesome.

[marcosvega91]

Yes I can give you an hand 😄