Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add next.js app router example #101

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

add next.js app router example #101

wants to merge 15 commits into from

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Jan 22, 2024

@kettanaito
Copy link
Member Author

Server-side integration

I got the server-side MSW integration working in Next.js by using the instrumentation hook:

export async function register() {
  if (process.env.NEXT_RUNTIME === 'nodejs') {
    const { server } = await import('./mocks/node')
    server.listen()
  }
}

This allows MSW to intercept server-side requests Next.js makes.

Downsides

  1. Next seems to evaluate the instrumentation hook once. The import graph it creates will not update if you change mocks/handlers.ts because nothing but the instrumentation hook depends on that import. This means stale mocks until you re-run Next.js/force it re-evaluate the instrumentation hook.

* this module and runs it during the build
* in Node.js. This makes "msw/browser" import to fail.
*/
const { worker } = await import('../mocks/browser')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next.js puts this dynamic import from the browser runtime to the Node.js build by moving it to the top of the module.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about fixing like this?

if (typeof window !== 'undefined') {
  const { worker } = await import('../mocks/browser')
  await worker.start()
}

Copy link

@brycefranzen brycefranzen Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't work for me. I had to do this instead:

if (process.env.NEXT_RUNTIME !== "nodejs") {
    const { worker } = await import("../mocks/browser");
    await worker.start();
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brycefranzen, that may actually work around webpack resolving this import in Node.js as well. I still think that's a bug though.

* export conditions and don't try to import "msw/browser" code
* that's clearly marked as client-side only in the app.
*/
if (isServer) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hack. I'm not sure why webpack has trouble resolving export conditions. I suspect this isn't webpack's fault. Next.js runs a pure client-side component in Node.js during SSR build, which results in webpack thinking those client-side imports must be resolved in Node.js.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kettanaito I think the 'use client' directive is a bit of a misnomer. Components marked with that directive can still be SSR and are by default in Next unless you lazy load with ssr: false. Obviously anything in useEffect would only run on the client, so I'm not sure why the dynamic import you have in the other file is placed in a Node.js runtime. Let me know if I'm missing any context.

Having said that, I pulled this repository down and ran dev and build and both succeeded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarifying, @dbk91!

I suspect webpack extracts that import and puts it at the top of the module for whichever optimization. This is a bit odd since import() is a valid JavaScript API in the browser so it can certainly be client-side only.

I know this example succeeds. I've added tests to confirm that and they are passing. But I'm not looking for the first working thing. I'm looking for an integration that'd last and make sense for developers. This one, in its current state, doesn't, as it has a couple of fundamentals problems.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, that makes sense! I totally missed your follow up messages in your original Tweet—I was expecting something non-functional and didn't realize there was extra work to get these tests passing.

Either way, I've been following this for quite some time and appreciate the work you've put into MSW and specifically this integration. My team was using it prior to upgrading to app router and we've sorely missed it, but that's on us for upgrading.

@SalahAdDin
Copy link

SalahAdDin commented Jan 31, 2024

I'm our company we are using this example as a reference.

@mizamelo
Copy link

mizamelo commented Feb 9, 2024

@SalahAdDin

I'm our company we are using this example as a reference.

is it working? I've tried to use, but it's showing these messages below:

Internal error: TypeError: fetch failed

`[MSW] Warning: intercepted a request without a matching request handler:

• POST https://telemetry.nextjs.org/api/v1/record`

carloscuesta added a commit to carloscuesta/carloscuesta.me that referenced this pull request Feb 9, 2024
@SalahAdDin
Copy link

@SalahAdDin

I'm our company we are using this example as a reference.

Is it working? I've tried to use it, but it's showing these messages below:

Internal error: TypeError: fetch failed

`[MSW] Warning: intercepted a request without a matching request handler:

• POST https://telemetry.nextjs.org/api/v1/record`

Not checked it yet, We just set it up.

'use client'
import { useEffect, useState } from 'react'

export function MockProvider({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using suspense?

mockProvider.tsx

'use client'

let triggered = false

async function enableApiMocking() {
  const { worker } = await import('../mocks/browser')
  await worker.start()
}

export function MockProvider() {
  if (!triggered) {
    triggered = true
    throw enableApiMocking()
  }

  return null
}

layout.tsx

export default function RootLayout({
  children,
}: Readonly<{
  children: React.ReactNode
}>) {
  return (
    <html lang="en">
      <body className={inter.className}>
        <MockProvider />
        {children}
      </body>
    </html>
  )
}

By doing so, we can avoid wrapping children in the mock provider client component.
But I am not sure if this is a good solution.

useEffect Suspense
ss2 ss

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this is to defer the rendering of the children until the service worker is activated. You are proposing keeping the state internally but I don't see it affecting {children}. So they will render, and if they make any HTTP requests, those will not be intercepted because the worker is not ready yet.

@SalahAdDin
Copy link

@kettanaito I don't know why but MSW is not intercepting page request. The mock is enabled but does not catch any fetch.

@pandeymangg
Copy link

how does playwright work with this? My test makes the actual api call instead of the mocked call

@kettanaito
Copy link
Member Author

@pandeymangg, there should be nothing specific to Playwright here. You enable MSW in your Next.js app, then navigate to it in a Playwright test and perform the actions you need.

@jogilvyt
Copy link

Thanks @kettanaito - that makes sense, and yes setupRemoteServer sounds like exactly what I'm looking for. I'll keep an eye on the PR ❤️

@Yan-pg
Copy link

Yan-pg commented Sep 24, 2024

HI, I'm using next 14 and I can't integrate with mws on server Module not found: Can't resolve '_http_common', someone had this same error?

@r34son
Copy link

r34son commented Sep 24, 2024

HI, I'm using next 14 and I can't integrate with mws on server Module not found: Can't resolve '_http_common', someone had this same error?

Look at vercel/next.js#70262

@pstachula-dev
Copy link

Does anyone have any example about middleware?

MSW 2.0 is not working with middlewares I have an error:

Invariant Violation: Failed to create a WebStorageCookieStore: `localStorage` is not available in this environment. This is likely an issue with MSW. Please report it on GitHub: https://github.com/mswjs/msw/issues
    at <unknown> (webpack-internal:///(instrument)/./node_modules/outvariant/lib/index.mjs:75)
    at eval (webpack-internal:///(instrument)/./node_modules/msw/lib/core/utils/cookieStore.mjs:147:108)

@kettanaito
Copy link
Member Author

@pstachula-dev, that looks like a rather old version of MSW. npm i msw@latest, and let me know.

@pstachula-dev
Copy link

pstachula-dev commented Sep 25, 2024

@kettanaito It was MSW 2.4.3 on the latest version 2.4.9 I have different error:

Module not found: Can't resolve '_http_common'

https://nextjs.org/docs/messages/module-not-found

@kettanaito
Copy link
Member Author

@pstachula-dev, for that you have to wait for Next.js to release that bugfix. It's already been merged (see vercel/next.js#70262).

@pstachula-dev
Copy link

pstachula-dev commented Sep 25, 2024

@kettanaito Update...
Test repo: https://github.com/pstachula-dev/msw-nextjs-error
MSW: 2.4.9
Nextjs: 15.0.0-canary.166

Module not found: Can't resolve '_http_common' - this errors is gone 🆗

But now I have new problems:

 Compiled /_not-found in 1218ms (920 modules)
  node_modules/outvariant/lib/index.mjs (69:1) @ <unknown>
  Failed to create a WebStorageCookieStore: `localStorage` is not available in this environment. This is likely an issue with MSW. Please report it on GitHub: https://github.com/mswjs/msw/issues
  67 | var invariant = (predicate, message, ...positionals) => {
  68 |   if (!predicate) {
> 69 |     throw new InvariantError(message, ...positionals);
     | ^
  70 |   }
  71 | };
  72 | invariant.as = (ErrorConstructor, predicate, message, ...positionals) => {

@kettanaito
Copy link
Member Author

@pstachula-dev, can you please provide a reproduction repo for this?

The error means you are running browser code in a non-browser environment. Properly describing what you are doing, how, and what you expect as a result will help tremendously.

@pstachula-dev
Copy link

@kettanaito I have already posted repository above :)

Test repo: https://github.com/pstachula-dev/msw-nextjs-error

@felipepalazzo
Copy link

@pstachula-dev I upgraded to v15.0.0-canary.171, and I don't see any errors now

@pstachula-dev
Copy link

pstachula-dev commented Sep 26, 2024

@felipepalazzo The situation is quite dynamic, this version is from 17h ago 😆

Hmm I have still same problems: with canary.171.
Node 20.17.0

  node_modules/outvariant/lib/index.mjs (69:1) @ <unknown>
  Failed to create a WebStorageCookieStore: `localStorage` is not available in this environment. This is likely an issue with MSW. Please report it on GitHub: https://github.com/mswjs/msw/issues
  67 | var invariant = (predicate, message, ...positionals) => {
  68 |   if (!predicate) {
> 69 |     throw new InvariantError(message, ...positionals);
     | ^
  70 |   }
  71 | };
  72 | invariant.as = (ErrorConstructor, predicate, message, ...positionals) => {

Comment on lines +23 to +28
<html lang="en">
<body className={inter.className}>
<MSWProvider>{children}</MSWProvider>
</body>
</html>
)
Copy link

@vedantshetty vedantshetty Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the main goal of the provider is to ensure the service workers are loaded before the components makes any API calls

But it's not ideal that we need to change our application behaviour to only use client components.

We could update the layout.tsx code to conditionally wrap the children if the application is running in dev, this could still let us use server rendering in production.

But even with that, what about using server component specific logic (eg: Using next/navigation redirect)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So MSWProvider being a client component forces the rest of its children tree to be client components only? Is that the issue?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kettanaito It won't force the components within MSWProvider to be client components unless you import them directly into a file with the "use client" directive—server components can be passed as children or other props to client components.

However, I think passing null to fallback in the Suspense boundary in msw-provider.tsx results in a blank page until the promise resolves on the client-side. What we've done in our projects is pass children as the fallback so the original tree is rendered while the promise resolves. I have no idea if this has performance implications at scale, but it's worked for our purposes.

Copy link

@vedantshetty vedantshetty Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbk91 You're absolutely right. Can ignore my concerns since they are incorrect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbk91,

However, I think passing null to fallback in the Suspense boundary in msw-provider.tsx results in a blank page until the promise resolves on the client-side.

That is intended. Your app mustn't render until MSW is activated. If it does, it may fire HTTP requests and nothing will be there to intercept them. You must use null as the suspense fallback.

if (process.env.NEXT_RUNTIME === 'nodejs') {
const { server } = require('../mocks/node')
server.listen()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it recommended to do it here, or in the instrumentation file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bitttttten, it is recommended to enable it in layout.tsx. You can forget about the instrumentation file. I've tried it before, but it doesn't support HMR and isn't a part of your app's life-cycle. That was a bad choice, and the Next.js team members have pointed that out for me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what way is it suggested to change the mock per test if the suggested way to set up the mocks is via the layout file?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@votemike wouldnt each url be in a way idempotent? do you mean you want to call 1 url multiple times and see different results?
if you want to run different things depending on search parameters and what not you can do that in the mock

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the same URL with different return values.
Something like a product page.
Product A with stock
Product A out of stock
Product A coming soon
Product A 404 or 500ing.

I suppose I could vary the URL to pick up the mock I want. But in reality, production code would be calling the same URL and getting different return values at different times. Hence my desire to be able to vary the mock per test.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebws
Copy link

sebws commented Oct 24, 2024

As a workaround, to fix HMR not disposing of the registered worker, we can tell the HMR code how to dispose of it ourselves.

In browser.ts, append to the end of the file

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

You may need a ts-expect-error.

What this is doing is adding the function () => { worker.stop(); } to a list of functions called when the module is disposed of (in this case since it's being replaced). This makes it so client-side HMR is working as expected (at least with minimal testing so far).

Originally mentioned here: vercel/next.js#69098

@kettanaito
Copy link
Member Author

@sebws, still curious how does calling worker.stop() help?

All worker.stop() does it tells the worker script that this client has to be ignored. It doesn't clean up the handlers, doesn't affect the worker's state otherwise. I think what worker.stop() achieves in your suggest is ignores the previous, persisted worker instance from the last refresh so it doesn't conflict with the newly created instance after a new refresh (HMR). Thus providing once again that it's a workaround.

@sebws
Copy link

sebws commented Oct 24, 2024

@kettanaito I'll have to double check. I remember the worker no longer being present in the memory of the page.

There's a chance I misunderstood if it was working correctly, so I was hoping you might give it a shot.

@sebws
Copy link

sebws commented Oct 24, 2024

Yep, as far as I can see without it, there'd be extra SetupWorkerApis, when taking a heap snapshot in the memory tab of chrome devtools.

With the workaround, there's just the one (a new one each reload). My guess would be that if worker.stop() doesn't kill the worker, whatever it does do, removes the retainers of the worker that otherwise keep it in memory.

import { MSWProvider } from './msw-provider'

if (process.env.NEXT_RUNTIME === 'nodejs') {
const { server } = require('../mocks/node')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this mean the mocks are bundled with the production code?
Does that have any undesirable side-effects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@votemike, this likely needs tweaking by adding a new env variable like MOCKS_ENABLED and checking that one too.

Comment on lines +19 to +23
export function MSWProvider({
children,
}: Readonly<{
children: React.ReactNode
}>) {
Copy link

@sebws sebws Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function MSWProvider({
children,
}: Readonly<{
children: React.ReactNode
}>) {
export default ({
children,
}: Readonly<{
children: React.ReactNode
}>) => {

@kettanaito

Some success 😄

The reason the interval is cleared in the Svelte example is because this listener for beforeunload is called.

In the Next example, this event listener is not called (presumably beforeunload doesn’t get called due to how Next.js handles page changes). Therefore, nothing clears the intervals and the workers are not garbage collected.

I'm not 100% sure how it works, since in Svelte beforeunload is happening but it doesn't seem like window.location.reload is being called. I did notice this meant that changing the handler used in the server rendered component doesn't cause it to be reloaded.

I believe Next.js doesn't perform a full page reload because the MSWProvider has a parent (layout.tsx) so a Fast Refresh is triggered. In a normal React setup, the changed file is at the root (pre-rendering) and has no parents, so a full page refresh is triggered.

We can take advantage of the fact that Fast Refresh use the casing of exports in a file to check if hot reload is possible. See this docs page https://nextjs.org/docs/messages/fast-refresh-reload

Exporting a component as an anonymous component breaks this, and so it always triggers a full reload. Therefore, we can make the change here to get HMR (via full page reload).

I think given this info, it's worth still thinking about module.hot.dispose() which does not force a full page reload. This mimics the behaviour of Svelte, including unfortunately that changing the handlers doesn't reflect in the server-rendered components (until manual reload).

In my eyes it depends upon if it's a bug or not a bug that beforeunload isn't being run in Next.js when there's HMR for the MSWProvider. If that isn't a bug, then there's no reason for setInterval to be cleared and so no reason for the extra worker to disappear. As a relevant note, Svelte mentions something along these lines in their HMR package. But at the moment I don't see a massive amount of difference between using beforeunload here to do this vs module.hot.dispose()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried both approaches, named and anonymous export, but both fails on:
[MSW] Warning: intercepted a request without a matching request handler:

In browser logs I clearly see that the worker has started.

Starting MSW worker
[MSW] Mocking enabled.

On server side the requests are mocked, but on client side all requests are just warning logged.

What I am missing? Using Next 14.2

My provider looks like this:

'use client';

import { Suspense, use } from 'react';

const mockingEnabledPromise =
  typeof window !== 'undefined' && process.env.NEXT_RUNTIME !== 'nodejs'
    ? import('../mocks/browser').then(async ({ worker }) => {
        console.log('Starting MSW worker');
        await worker.start({
          onUnhandledRequest(request, print) {
            if (request.url.includes('_next')) {
              return;
            }
            const excludedRoutes = [
              'cognito-idp.eu-west-1.amazonaws.com',
              'cognito-identity.eu-west-1.amazonaws.com',
              'google-analytics.com',
            ];
            const isExcluded = excludedRoutes.some((route) => {
              return request?.url?.includes(route);
            });
            if (isExcluded) {
              return;
            }
            print.warning();
          },
        });
      })
    : Promise.resolve();

export default ({
  children,
}: Readonly<{
  children: React.ReactNode;
}>) => {
  return (
    <Suspense fallback={null}>
      <MSWProviderWrapper>{children}</MSWProviderWrapper>
    </Suspense>
  );
};

function MSWProviderWrapper({
  children,
}: Readonly<{
  children: React.ReactNode;
}>) {
  use(mockingEnabledPromise);
  console.log('MSWProviderWrapper children', children);
  return children;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@funes79 not sure this is relevant to this comment I'm afraid. seems like an issue with how you've set up the handlers.

@kettanaito did you see my comment above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what issues you are trying to solve here.

The current state of this PR gives you client-side mocking. If something is not mocked, follow the Debugging runbook, the issue is likely elsewhere.

The only thing missing in the current state is a proper HMR support for client-side mocking.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you responding to funes? My original comment is about client side HMR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebws, yes, that is correct.

Thanks for diving deeper into the HMR issue.

Therefore, nothing clears the intervals and the workers are not garbage collected.

So you are saying that interval keeps the worker object (not the service worker, mind) persist between HMR? That still sounds a bit odd to me. Do you have proof that clearing that interval indeed solves the issue?

I don't see module.hot.dispose() as the way forward, to be frank. This is a low-level hackery that an average Next.js user shouldn't be exposed to. I don't want to ask developers to do that. The issue is clearly specific to how Next.js handlers client-side HMR, otherwise we had the same issue in other frameworks. This is just a mention that the proper fix for this one is likely on Next.js' side, and that's why we have vercel/next.js#69098.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking again, it looks like more generally it's about beforeunload being called and what that does, in particular deregistering the actual service worker.

      context.events.addListener(window, "beforeunload", () => {
        if (worker.state !== "redundant") {
          context.workerChannel.send("CLIENT_CLOSED");
        }
        window.clearInterval(context.keepAliveInterval);
      });

it's not just the interval which is keeping the worker object in memory. we can prove this by logging the creation of the interval, clearing it, and the issue persists.

I may have made the original post at 2am and I don't have a full picture of how msw works so some of this is best guess

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for context on how I'm looking at this for if you or others are interested, I'm using the "Memory" tab of chrome devtools, and taking heap snapshots at various points. then in the filter, adding SetupWorkerApi and looking at the retainers.

I found this pretty confusing but key for me has been clicking through when there is a line of code referenced, as well as trying out what happens when you right click -> ignore this retainer.

@kettanaito kettanaito changed the title add next.js (app directory) example add next.js app router example Nov 13, 2024
await page.goto('/', { waitUntil: 'networkidle' })

const greeting = page.locator('#server-side-greeting')
await expect(greeting).toHaveText('Hello, John!')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await expect(greeting).toHaveText('Hello, John!')
await expect(greeting).toHaveText('Hello, Sarah!')

If the test is about checking the mocked data, it should match what's in the handler, right? Same for the other test. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.