mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.97k stars 519 forks source link

"TypeError: A dynamic import callback was invoked without --experimental-vm-modules" in Jest #2254

Closed johnal95 closed 2 months ago

johnal95 commented 2 months ago

Prerequisites

Environment check

Node.js version

20.15.1

Reproduction repository

https://github.com/johnal95/repro-msw-graphql-import-error

Reproduction steps

  1. Install dependencies (this repo uses pnpm)
pnpm install
  1. Run tests
pnpm test
  1. The following errors are printed to the stdout:
    
    [MSW] Failed to parse a GraphQL query: cannot import the "graphql" module.
    Please make sure you install it if you wish to intercept GraphQL requests.
    See the original import error below.

TypeError: A dynamic import callback was invoked without --experimental-vm-modules at importModuleDynamicallyCallback (node:internal/modules/esm/utils:226:11) ... rest of the stack trace



### Current behavior

The `graphql` module fails to be imported, preventing the mocking of GraphQL requests.

Likely introduced with https://github.com/mswjs/msw/commit/1799e0638f0f860c19ba46db7c4287012f2cb716

### Expected behavior

Same bahavior as in the previous version - `2.4.0`. `graphql` module is imported successfully and GraphQL requests are mocked as expected.
kettanaito commented 2 months ago

Hi, @johnal95. Thanks for reporting this!

First, it seems to be an issue exclusively with Jest and its still lacking proper support for ESM. Here's why I suspect that.

  1. This error isn't the case in Vitest.
  2. The same msw imports work without problems in Node.js v20:
import { graphql, HttpResponse } from "msw";
import { setupServer } from "msw/node";

const server = setupServer(
    graphql.query("GetUser", () => {
        return HttpResponse.json({ data: { user: {} } });
    })
);

server.listen();

await fetch("https://api.example.com", {
    method: "POST",
    headers: {
        "Content-Type": "application/json",
    },
    body: JSON.stringify({
        query: `
      query GetUser {
        user {
          id
          name
        }
      }
    `,
    }),
});

The mention of VM heavily implies Node.js is throwing on how Jest is using the node:vm module to run tests. Jest doesn't support running ESM properly in this instance, and dynamic import seems to be a part of ESM.

How to fix this

Follow the suggestion from the error:

// package.json
{
     "scripts": {
-        "test": "jest"
+        "test": "NODE_OPTIONS=--experimental-vm-modules jest"
     },
}

MSW fix

We are not addressing Jest-related issues anymore. As much as I admire Jest, it holds the entire ecosystem back. If you can, use Vitest or other modern testing frameworks.

johnal95 commented 2 months ago

@kettanaito I understand the decision. Although it might be worth noting that a considerable amount of test suites out there still run on Jest. And migrating some of these codebases (such as the one where I originally encountered this issue, which has thousands of test files) to e.g. Vitest might not be a small/easy task.

I definitely agree with the idea of moving to more modern testing frameworks and towards ESM. But it would be nice to maintain support for Jest for a while longer if possible, to allow for more and/or bigger codebases to adapt.

Since the dynamic import of graphql seems to be the only detail causing trouble here.

Could it be an option to transpile the dynamic import, or post-process the build output for parseGraphQLRequest (exclusively for the CJS build)?

E.g.

try {
// replace `await import` by `require`
- const { parse } = await import('graphql')
+ const { parse } = require('graphql')

  // ... rest of implementation
} catch (error) {
  devUtils.error('Failed to parse a GraphQL query: cannot import...')
  throw error
}

(I know it may not look great, but maybe that could keep the lights on a bit longer πŸ™‚)

Using the --experimental-vm-modules flag is not a viable option for my current use case. (and I personally haven't had a great experience with it in the past, e.g. intermittent segfaults) πŸ₯²

kettanaito commented 2 months ago

Since the dynamic import of graphql seems to be the only detail causing trouble here.

Unfortunately, that's only on the surface. It's a symptom of a bigger issueβ€”not installing graphql for developers that don't intend to do GraphQL mocking. Dynamic import works without issues, but the one you experience is relevant only to Jest.

Could it be an option to transpile the dynamic import, or post-process the build output for parseGraphQLRequest (exclusively for the CJS build)?

This sounds like a good idea. Are you aware of require will remain scoped to that parseQuery function? I guess it should, but I haven't experimented with it in a while.

We can have a custom esbuild plugin that will do that for us. Would you help me test that solution in your project?

kettanaito commented 2 months ago

@johnal95, I've opened a fix at #2258. Could you please test it in your project?

johnal95 commented 2 months ago

@kettanaito out of interest why the decision of modifying the ESM build instead of the CJS? πŸ€”

I also just tested a very crude example which would produce a very similar outcome in my repro repo, and seems to be working as expected:

https://github.com/johnal95/repro-msw-graphql-import-error/tree/master

(there is a postprocess-msw script that very crudely makes the change to the msw dist. The first change would be assumed to be committed in msw, and the second would be part of the transpilation/post processing logic)

The idea here being that we could refactor the current code to wrap the require/import in a try catch instead of using Promise catch. Then it's just a matter of replacing "await import" by "require" in that one file.

What do you think?

kettanaito commented 2 months ago

But does the fix I proposed above work?

why the decision of modifying the ESM build instead of the CJS?

Mostly due to the .catch() chaining. Your try/catch proposal is tempting but it will require to store { parse } as a nullable variable first, then assign to it, then the rest of the code is broken if we caught an import error. Now, we need to wrap the entire parse function in try/catch and that may produce faulty errors on other things failing. Now, we have to differentiate between the error origin...

I find it much simpler to replace require() with import().catch().

If you can confirm my fix working, I can proceed with publishing it.

johnal95 commented 2 months ago

Mostly due to the .catch() chaining. Your try/catch proposal is tempting but it will require to store { parse } as a nullable variable first, then assign to it, then the rest of the code is broken if we caught an import error. Now, we need to wrap the entire parse function in try/catch and that may produce faulty errors on other things failing. Now, we have to differentiate between the error origin...

Fair enough.

If you can confirm my fix working, I can proceed with publishing it.

Just tested and I confirm that does solve the issue. Many thanks! πŸ™

kettanaito commented 2 months ago

Released: v2.4.2 πŸŽ‰

This has been released in v2.4.2!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.