Dynamic "import" creates a race condition with initial load requests

[kettanaito]

Verify canary release

• I verified that the issue exists in the latest Next.js canary release

Provide environment information

This issue is not related to the precise Next version but rather to the with-msw example code.

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.6.0: Wed Aug 10 14:28:23 PDT 2022; root:xnu-8020.141.5~2/RELEASE_ARM64_T6000
Binaries:
  Node: 16.13.2
  npm: 8.1.2
  Yarn: 1.22.17
  pnpm: 7.4.0
Relevant packages:
  next: 12.3.4
  eslint-config-next: N/A
  react: 17.0.2
  react-dom: 17.0.2

Which example does this report relate to?

with-msw

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

• Originally reported in MSW + NextJS 12+ : doesn't intercept first request, waitUntilReady not working mswjs/msw#1474 (I describe what/why happens here).

I believe due to #25607, MSW example has migrated from require() to dynamic imports import() in order to tree-shake MSW from production builds. That, however, introduced a different problem:

• Since await import() is async, and there's no top-level await to wait for the initMocks() function, there's a race condition created between the client-side code and the mocks being imported.

This race condition manifests upon initial page requests as those may be ignored by MSW since the code is not done resolving await import() when those requests happen.

Expected Behavior

1. The example imports/requires MSW setup synchronously.
2. Alternatively, the example ensures that the app doesn't run until MSW is imported. This import is not a costly operation, it's just a matter of asynchronicity and awaiting it.
3. Side-effect: MSW must not be included in the production bundle as an indirect result of this fix/change.

To Reproduce

• See the reproduction repo in MSW + NextJS 12+ : doesn't intercept first request, waitUntilReady not working mswjs/msw#1474

[kettanaito]

I would propose to swap await import with require but that seemed to produce undesired tree-shaking behavior in the past (although the issue around that is still open, #25607).

next.js/examples/with-msw/mocks/index.ts

Lines 1 to 13 in 9fe5317

	async function initMocks() {
	if (typeof window === 'undefined') {
	const { server } = await import('./server')
	server.listen()
	} else {
	const { worker } = await import('./browser')
	worker.start()
	}
	}
	initMocks()
	export {}

-const { worker } = await import('./browser')
+const { worker } = require('./browser')

Does this have to do with dynamic require that is not statically analyzable?

[balazsorban44]

require is not tree-shaken, however, I believe there is a different way of resolving this issue:

// pages/_app.tsx
import { useEffect } from 'react'
import { AppProps } from 'next/app'

export default function App({ Component, pageProps }: AppProps) {
  useEffect(() => {
    // This is the recommended way to set up Web/Service Workers in the browser. See
    // https://github.com/vercel/next.js/blob/canary/examples/with-web-worker/pages/index.tsx
    // https://github.com/vercel/next.js/blob/canary/examples/with-service-worker/pages/index.tsx
    if (process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
      Promise.all([import('msw'), import('../mocks/handlers')]).then(
        ([msw, { handlers }]) => msw.setupWorker(...handlers).start()
      )
    }
  }, [])
  return <Component {...pageProps} />
}

// pages/_document.ts
export { default } from 'next/document'

if (process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
  require('msw/node')
    .setupServer(...require('../mocks/handlers').handlers)
    .listen()
}

Since _document is only evaluated server-side, it never increases the client bundle size. What do you think?

[andykenward]

require is not tree-shaken, however, I believe there is a different way of resolving this issue:

// pages/_app.tsx
import { useEffect } from 'react'
import { AppProps } from 'next/app'

export default function App({ Component, pageProps }: AppProps) {
  useEffect(() => {
    // This is the recommended way to set up Web/Service Workers in the browser. See
    // https://github.com/vercel/next.js/blob/canary/examples/with-web-worker/pages/index.tsx
    // https://github.com/vercel/next.js/blob/canary/examples/with-service-worker/pages/index.tsx
    if (process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
      Promise.all([import('msw'), import('../mocks/handlers')]).then(
        ([msw, { handlers }]) => msw.setupWorker(...handlers).start()
      )
    }
  }, [])
  return <Component {...pageProps} />
}

// pages/_document.ts
export { default } from 'next/document'

if (process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
  require('msw/node')
    .setupServer(...require('../mocks/handlers').handlers)
    .listen()
}

Since _document is only evaluated server-side, it never increases the client bundle size. What do you think?

@balazsorban44 I tried your suggestion. But for client-side fetch requests, the client-side worker loads after the request is made on a next.js page (child component) via a useEffect. I think we would have to have a loading state for the worker on the client-side?

I do have a working example that supports client-side fetch requests mocked via MSW. Take a look at mswjs/msw#1340 (comment) . But it does involve changing the default next.js setup, which isn't ideal.

[kettanaito]

Thanks for the suggestion, @balazsorban44. I have a few concerns with the client-side part:

1. useEffect doesn't block rendering, so Component will render, rendering the underlying page, which will trigger a request in its respective useEffect, resulting in the same problem, would it not? Should we use something like useLayoutEffect instead?
2. This depends that useEffect won't be called on the server. I think relying on that is a bit risky, as, to my knowledge, useEffects are called during SSR.

Overall, the setup grows in complexity and feels less and less as something we should recommend people to do manually (despite using the example template). I wonder, is there a way to abstract this? I was thinking of a webpack plugin or some other, more automated means to achieve this.

[balazsorban44]

1. I am thinking that useEffect in general should not be used to trigger data fetching but it might be a different discussion. Loading a Service/Web worker for MSW is not different than any other case from the Next.js perspective, so I do believe it's the correct solution here. A loading state could probably be added to be able to determine if it's ready yet. Will ask about useLayoutEffect.
2. Actually, useEffect never executes server-side and is the safe place to use browser APIs for example.

[kettanaito]

I see.

Do you have any opinion on abstracting this setup someplace else? Are there perhaps other tools that have a complex setup and utilize a different pattern than including it in the template directly?

My main concern here is that this kind of setup is brittle due to its verbosity and internal detail awareness. Despite being a template, this is still the code that people will ship in their apps but they have, effectively, no authorship around this code. As a consequence, if there's an issue with this setup, it makes it hard to fix for people as they cannot re-apply the template and would have to apply whichever fix is appropriate manually. It really feels like something we should distribute rather than inline.

I was thinking if adding a new entry to the webpack compilation wouldn't be beneficial here. Something like:

// next.config.js
module.exports = {
  webpack(config) {
    config.entry.push('./msw.setup.js')
    // Use a webpack plugin to create "msw.setup.js" module
    // in the in-memory file system.
    return config
  }
}

Then, on the consumer's side:

// next.config.js
import { withMSW } from '@mswjs/webpack-plugin'

module.exports = withMSW()

The plugin would ensure the correct module import and initialization, and the with-msw template would still be relevant to show the directory structure and have a quick start.

The plugin is, indeed, limiting in terms of its dependency on the MSW setup structure on the consumer's side as it needs to know where the handlers.js are, etc. We can parametrize this though. Still a lesser price to pay in terms of maintenance.

[laurencefass]

I raised a different problem with Next 13 beta version here but it was closed down as being the same problem defined here, without any investigation or discussion. MSW works fine with the pages folder but fails with the app folder.

Having integration with a tool like msw is going to be important to demonstrate the new fetching features and closing down an investigation into why msw is not compatible with Next 13 appDir will only delay progress in testing those features.

@balazsorban44 please can you repoen that ticket relating to Next 13 until at least it is established these are the same problem. There is currently no msw+Next13 demo available and that issue could form the basis for it. You have closed down any possibility of that by closing the ticket.

If you cannot guarantee that fixing the issue for Next 12 will fix issues for Next 13 beta specifically the app directory features with server side components then it is quite reasonable to have to open tickets relating to different major versions of Next.

[balazsorban44]

Replied there #43758 (comment)

[StampixSMO]

Since this issue has been sleeping for a while: has anyone been able to make any progress in adding MSW into a Nextjs 13 app directory structure?

[philwolstenholme]

I think the official line is that the MSW maintainer will wait until the app router is stable before investigating this issue (it's caused by how the Next example is using MSW, not caused by MSW itself), and the Next maintainers will wait a bit longer to update the Next examples directory to use the app router. So, unless the community can figure it out themselves, we will need to wait.

I don't have enough Node knowledge to know if this will work, but I was wondering about using this trick https://jake.tl/notes/2021-04-04-nextjs-preload-hack#using-preloading to 'preload' MSW in dev mode with a require call inside it to avoid the dynamic import, but also not add MSW to my production bundles. I'm not sure when I'll have a chance to try it, so if anyone else would like to give it a go then please do!

[kettanaito]

The @philwolstenholme 's stance is correct. At the moment, I don't see Next.js providing library authors with the alternative to _app.js with the new App router release (Next 13). That is the main reason why MSW example stops working in the next version of Next. I do hope the Next team considers this as there are quite a number of Next+MSW users out there, and it'd be great to have the means to execute browser/server-side code once before the app loads (to defer its rendering as we're doing with the older versions of Next). This is not something we can do on the MSW side, this must be provided by the framework.

@philwolstenholme, regarding the module preloading, what you're suggesting will work for Node.js but in Node.js things are already synchronous and working well. It is the browser integration that suffers from this race condition between:

• The msw module being imported (which is an async import).
• The service worker registering by MSW;
• Async requests on initial page load that don't await the previous two points.

That being said, you can give the preloading a try! If it works, it may be a reasonable workaround until a better solution is found.

[kettanaito]

It would be great to hear the official Next team stance on the alternatives to the _app.js in terms of logic with their new Next 13 release. As far as I can tell, there are no alternatives, which causes this very problem.

[philwolstenholme]

I've not had a chance to try it yet, and this is a bit out of my comfort zone, but I'm thinking of adding back the require, but then trying to use something like resolve.alias in some production-only custom Webpack config to replace it with a stub or false.

Re: using the Node preloading/node --require idea, I'll give that a go and see if it means MSW is available to React Server Components/app directory pages on the server.

Edit: ah, maybe not! I got stuck trying to use ESM code with node --require, which makes sense:

❯ node --require ./src/mocks/server.js ./node_modules/.bin/next dev
node:internal/modules/cjs/loader:1277
      throw err;
      ^

[Error [ERR_REQUIRE_ESM]: require() of ES Module [path redacted]/src/mocks/server.js not supported.
Instead change the require of server.js in null to a dynamic import() which is available in all CommonJS modules.] {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.14.0

[sylvhama]

I was able to fix the race condition in next v13.1.6 by using dynamic imports in _app.tsx with a top level await:

if(process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
  const { initMocks } = await import('path/to/your/mocks');
  await initMocks();
}

In order to do this you will need to update next.config.js and its webpack config so it can support topLevelAwait, see #11185

[philwolstenholme]

I've just spotted https://nextjs.org/docs/app/building-your-application/optimizing/instrumentation – does this give a potential entry point for using MSW with the Next app router?

If you export a function named register from a instrumentation.ts (or .js) file in the root directory of your project (or inside the src folder if using one), we will call that function whenever a new Next.js server instance is bootstrapped.

Good to know

• This feature is experimental. To use it, you must explicitly opt in by defining experimental.instrumentationHook = true; in your next.config.js.
• The instrumentation file should be in the root of your project and not inside the app or pages directory. If you're using the src folder, then place the file inside src alongside pages and app.
• If you use the pagesExtension config option to add a suffix, you will also need to update the instrumentation filename to match.
• We have created a basic with-opentelemetry example that you can use.

When your register function is deployed, it will be called on each cold boot (but exactly once in each environment).

Sometimes, it may be useful to import a file in your code because of the side effects it will cause. For example, you might import a file that defines a set of global variables, but never explicitly use the imported file in your code. You would still have access to the global variables the package has declared.

[kettanaito]

@philwolstenholme, it looks like it does. Upon me trying the instrumentation hook, I've stumbled upon an issue that it tries to resolve browser exports of imported modules despite being a server-side hooks (wrote about it mswjs/msw#1644 (comment)). This may as well be a bug in Next or I may be using the hook incorrectly. I'm fairly confident in MSW's exports as we've tested them on various occasions and with different tools and they are working as expected.

The fact that import 'foo' in instrumentation.ts even considers to look up the browser export path hints at something wrong with the module resolution, regardless if the foo package ships the browser field or not.