Module incorrectly persists between hot updates (HMR) in the browser

[kettanaito]

Link to the code that reproduces this issue

mswjs/examples#101

To Reproduce

1. Clone the repo, check out the PR's branch.
2. pnpm install.
3. cd examples/with-next.
4. pnpm dev
5. Open the application URL in the browser.
6. Open the DevTools, select the "Console" tab.
7. Click the "Fetch movies" button on the page. See the list of fetched movies (these are coming from mocks). See a single log output from MSW about the intercepted GraphQL query.
8. Go to src/mocks/handlers.ts. Change the payload of the graphql.query() handler (e.g. remove any word from a movie title).
9. Save the changes.
10. Back in the browser, click "Fetch movies" again.
11. See two logs for the same GraphQL request.

Current vs. Expected behavior

Current behavior

The entire MovieList component gets re-rendered a bunch of times on hot update to handlers.ts. Re-rendering is expected, but it looks like Next.js re-applies event listeners to the same button multiple times.

This is not an MSW issue. You can log something in the MovieList component manually, and see that it re-renders quite a lot. I suspect during those re-renderings, the onClick listener gets applied more than it needs to.

The number of times the listener is excessively applied is directly proportionate to the number of HMR changes issued (e.g. 1 change = 2 listeners; 2 changes = 3 listeners; etc).

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 23.3.0: Wed Dec 20 21:28:58 PST 2023; root:xnu-10002.81.5~7/RELEASE_X86_64
  Available memory (MB): 65536
  Available CPU cores: 16
Binaries:
  Node: 18.19.0
  npm: 10.2.3
  Yarn: 1.22.10
  pnpm: 9.6.0
Relevant Packages:
  next: 15.0.0-canary.121 // Latest available version is detected (15.0.0-canary.121).
  eslint-config-next: N/A
  react: 19.0.0-rc-14a4699f-20240725
  react-dom: 19.0.0-rc-14a4699f-20240725
  typescript: 5.3.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Developer Experience, Runtime

Which stage(s) are affected? (Select all that apply)

next dev (local)

Additional context

No response

[kettanaito]

The only unusual thing that may be related is the MSW setup:

https://github.com/mswjs/examples/pull/101/files#diff-e0bbff5ddfef9a67e6e818bdf0575be8737af37c859effc946ee40e8dc6b30c2

Can some of these hooks/functions lead to component rendering being messed up?

[kettanaito]

Exploration

1. React Strict Mode doesn't seem to be affecting this behavior. I've tried disabling it in next.config.js and the issue can still be reproduced:

// next.config.js
module.exports = {
  reactStrictMode: false,
}

2. Removing use() that awaits the worker start doesn't change anything.
3. Unregistering the worker manually before making a change and triggering HMR does not affect anything. The worker persisting HMR doesn't seem to be a problem (it should, normally).
4. If I introduce any other side effect, like a navigator.serviceWorker.addEventListener('message') in the msw-provider.tsx module, it will be executed twice as well (growing on each HMR). If I move that side effect to a new module, mark it as 'use client', and import in layout.tsx, it will not.

The last point makes me believe that the way msw-provider.tsx is re-evaluated during HMR can be faulty.

[kettanaito]

Okay, I think I understand better what's happening now. It looks like HRM doesn't terminate previously attached request handlers (see the screenshot below).

This is a screenshot of a duplicate request log in the console after HMR. You can see that although HMR has happened, and I've modified the handler, the "old" handler is still firing (the first one), taking precedence over the updated handler (the second one).

In this example, I've decorated the handler instances with the ID property that is random on every evaluation. The expected result here is that each HMR produces a new ID and a single handler. The issue is that it keeps producing more handlers for more hot updates (see my theory on why that happens in the next comment).

This is not a bug in MSW. If you observe the handlers between hot updates, you will see that worker.listHandlers() after HMR correctly lists the updated GraphQL handler.

The fact that the old handler persists and still works is likely related to how the module is re-evaluated. @feedthejim @eps1lon, with this new intel, do you have any thoughts on what may be causing this? Suggestions on how to debug this further are also welcome.

[kettanaito]

Workaround

As I suspected, the "cleanup" of handlers is not happening somewhere in memory in the mocks/browser.ts module (the one importing handlers.ts and providing them to the worker instance).

If I apply this diff, the issue is fixed:

// mocks/browser.ts
import { setupWorker } from 'msw/browser'
-import { handlers } from './handlers'

-export const worker = setupWorker(...handlers)
+export const worker = setupWorker()

// msw-provider.tsx
'use client'

import { Suspense, use } from 'react'
+import { handlers } from '../mocks/handlers'

// ...

await worker.start()
+worker.use(...handlers)

This effectively moves handlers.ts so they are directly imported by msw-provider.tsx.

Can it be that webpack incorrectly updates the modules in HMR? I think Remix used to have the exact issue, but the they've fixed it somehow (cc @pcattori).

It must not matter where you import the handlers. If the hot update originates in handlers.ts, I expect the bundler to correctly understand the update path:

• handlers.ts -> browser.ts -> msw-provider.tsx

But this update path seems to be faulty, resulting in the old and new handlers array persisting at the same time across hot updates.

[kettanaito]

Gave the proposed workaround a try, and it doesn't actually solve the issue. What ends up happening, the worker instance somehow retains the "old" handlers (from before HMR), and worker.use() simply prepends updated handlers to the overall list:

The first two are the updated handlers I explicitly import in msw-provider.tsx and provide to worker.use(). The previous four handlers are outdated handlers that somehow survive HMR.

Darn, I thought this can be a viable workaround for now, but it's not. If some values incorrectly persist across HMR, this can lead to bigger behavior issues for users.

I would love to get some help on this!

[sebws]

I'm not familiar with the code involved, but thought I'd mention since I'm not sure if this was obvious or not:

On each HMR update the SetupWorkerApi is persisting. See screenshot after editing handlers.ts twice

[kettanaito]

@sebws, thanks for providing more insight. Yeah, I'd expect that. The setupWorker APIs is a controller around handlers, so you don't have duplicate request handling due to the de-duplication mechanism in MSW (calling worker.start() multiple times does nothing), but you get duplicate request handlers within the same (latest) worker instance. That is a confusing part.

If it was a typical leak, we'd have N number of workers across N hot updates, each with their own set of handlers. But that's not the case. The latest worker instance accumulates all previous handlers, like I've illustrated on the screenshot above.

[sebws]

@kettanaito ah yep, I did that screenshot without the workaround. with the workaround, on hot reload it is no longer initialising a new worker (I suppose since there's no import of handlers into browser.ts), so adding a resetHandlers call in the mockingEnabledPromise works. it's just that you also get a "redundant worker.start()" message each time :)

[sebws]

just wondering, is starting up a new worker preferable? as far as I've seen before that's normally the way rather than a resetHandlers call so I'm curious if there's a particular reason

[kettanaito]

The thing is, your module is supposed to produce the same result upon evaluation during HMR, normally. We've gathered multiple examples of how other frameworks handle a nearly identical server-side and client-side setups: Remix, Svelte, Vue. There's no need to perform any magic around worker or server. When HMR comes, the old module gets thrown away, the changed module takes place. The other frameworks also act as an additional proof that the issue doesn't lie with MSW, otherwise, anywhere you use setupWorker would keep piling that object in memory between hot updates.

so adding a resetHandlers call in the mockingEnabledPromise works.

It works because it "remediates" the problem by clearing the handlers that persisted between hot updates before attaching new handlers. That's a workaround, not a solution I can recommend.

just wondering, is starting up a new worker preferable?

Well, it's the initial browser integration so, yes, it's not only preferable but is the only way to enable MSW in the browser.

[sebws]

If it was a typical leak, we'd have N number of workers across N hot updates, each with their own set of handlers. But that's not the case. The latest worker instance accumulates all previous handlers, like I've illustrated on the screenshot above.

I'm a little confused by this. What I was seeing, without your next specific workaround, was exactly that, a new worker per hot update.

Only when you change it (in the workaround) do you get the issue you're describing with the handlers being merged. Which makes sense to me, since you're now calling the methods on the same worker (without any reason for it to have been reloaded). So you call start, get redundant start message then add the new handlers to the pre-existing worker. Am I missing something?

[kettanaito]

That is not the case. You can follow the reproduction steps in the first post to get the problematic behavior I'm describing. The list of handlers persists across HMR when it shouldn't. My workaround showcases that it's an import problem.

[sebws]

I followed your reproduction steps just now, but added console.log(worker.listHandlers()) after the worker.start() call in msw-provider. I get just two handlers in both calls, however I still get the duplicate issue.

[kettanaito]

You get duplicate logs because there are old handler surviving HMR, as I've shown here. You don't see duplicates in .listHandlers() because, I suspect, a worker instance still has just 2 handlers at all times. But you are viewing an old worker instance, even after HMR, which you can prove by attaching IDs to handlers and seeing they they are still the old ones, pre-HMR (which is precisely what I did in the mentioned comment).

The old and new evaluation of the same module overlap, which makes it tricky to understand what's going on.

[kettanaito]

I would love to hear some input on this from the Next.js team. This looks like a webpack issue. I really, really hate to ping about this, but it's been over a month since this has been reported. An initial assessment would be great to have to see how we can move forward with this. cc @eps1lon.

[sebws]

you are viewing an old worker instance, even after HMR

Seems like we’re saying the same thing then, because that’s what I’m saying. That you can see old handlers but it’s because the old worker has persisted (with old handlers), not one worker with x handlers per refresh.

[sebws]

I'm having some luck :) It's another funny workaround/dodgy type of thing, however I added the following to mockingEnabledPromise after the call to worker.start

module.hot?.dispose(() => { worker.stop(); });

as well as moving the fallback value in the Suspense from null to false. Otherwise on HMR the browser would occasionally stay empty.

With this, on HMR you get just the one handler persisting!

I don't think this is a sustainable solution but might hopefully help in seeing what is happening

[sebws]

Actually, looks like it's even better to just put module.hot?.dispose(worker.stop) into browser.ts :) Then it doesn't even require changing the suspense from null to false (although it's probably not the worst idea still)

[sebws]

I was getting some odd issues with just worker.stop. Moving it to an arrow function () => { worker.stop(); } has prevented those issues and makes more sense at a glance

[sebws]

Looks like it's still an issue in [email protected]

[SalahAdDin]

Looks like it's still an issue in [email protected]

It will be solved in 16 XD.

[sebws]

mswjs/examples#101 (comment)

@kettanaito

I think I have found why exactly worker.stop() is fixing the issue. The reason the old worker is being retained is because of the interval set on the window object to send the KEEPALIVE_REQUEST here. worker.stop() clears this interval and so there is no longer anything retaining the object in memory.

I'm hoping to see now why the other frameworks aren't having this as an issue if this is the reason.

[kettanaito]

@sebws, such an interesting find, thank you for looking into this! I am surprised HMR doesn't kill that interval though. It should be a part of garbage collection from the previous "frame", and it's not even something the framework is doing.

Context: the interval is there to keep the worker-client channel alive (it gets killed if nothing has been transferred on it in some time). We won't be removing that interval, it's intentional and needed.

Since HMR destroys the previous execution context and its worker, I'd expect it to clear that interval as well. The fact that this doesn't cause issues in other frameworks still circles out Next.js and, likely webpack handling HMR.