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

fix: import graphql lazily #2250

Merged
merged 4 commits into from
Aug 29, 2024
Merged

fix: import graphql lazily #2250

merged 4 commits into from
Aug 29, 2024

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Aug 29, 2024

Changes

MSW depends on graphql only when using the graphql request handlers, and only to import its parse() method (other imports are type imports and are not present on runtime, neither can they affect it).

  • This change makes graphql a dynamic import where it's needed. This way, even though MSW exports { graphql } from './graphql' in the root, it will never result in import X from 'graphql' unless you import { graphql } from 'msw' on runtime.

In a long term, we should drop http/graphql handlers from the root entry and keep them only in msw/core/http and msw/core/graphql.

  • Also adds a custom error message if importing graphql fails for any reason. This will remind the developer that they have to install graphql if they want to intercept GraphQL requests.

@@ -40,7 +39,9 @@ export function parseDocumentNode(node: DocumentNode): ParsedGraphQLQuery {
}
}

function parseQuery(query: string): ParsedGraphQLQuery | Error {
async function parseQuery(query: string): Promise<ParsedGraphQLQuery | Error> {
const { parse } = await import('graphql')
Copy link
Member Author

Choose a reason for hiding this comment

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

Import cache should help us here since every GraphQL handler calls parseQuery().

@kettanaito
Copy link
Member Author

I can confirm this fixes the issue on a fresh Vitest project.

@Lalem001
Copy link
Contributor

I can confirm this fixes the issue on a fresh Vitest project.

I can confirm it works in my project as well

@kettanaito
Copy link
Member Author

Thanks for verifying this, @Lalem001 👍 Waiting for the final build and then releasing this.

@kettanaito kettanaito merged commit 1799e06 into main Aug 29, 2024
12 checks passed
@kettanaito kettanaito deleted the fix/graphql-peer branch August 29, 2024 15:13
@kettanaito
Copy link
Member Author

Released: v2.4.1 🎉

This has been released in v2.4.1!

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.

@calebevans-modeln
Copy link

@kettanaito Just wanted to say thank you so much for this fix! I installed msw 2.4.0 yesterday and ran into this exact issue. Was in the middle of creating a reproduction repository this morning when I discovered I couldn't reproduce the issue anymore (because v2.4.1 was published two hours ago). All that to say, I very much appreciate the fix!

@kettanaito
Copy link
Member Author

@calebevans-modeln, my pleasure! It looks like some folks are still experiencing it. Glad to hear it was solved for you. Looking forward to more reproduction repos to hunt it down for good.

@jgosmann
Copy link

Unfortunately, this didn't fix the issue for us. Running the TypeScript compiler tsc for type checking, presents us with these errors:

node_modules/msw/lib/core/graphql.d.ts:1:30 - error TS2307: Cannot find module 'graphql' or its corresponding type declarations.

1 import { DocumentNode } from 'graphql';
                               ~~~~~~~~~

node_modules/msw/lib/core/GraphQLHandler-Cu4Xvg4S.d.ts:1:63 - error TS2307: Cannot find module 'graphql' or its corresponding type declarations.

1 import { OperationTypeNode, DocumentNode, GraphQLError } from 'graphql';
                                                                ~~~~~~~~~


Found 2 errors in 2 files.

Errors  Files
     1  node_modules/msw/lib/core/graphql.d.ts:1
     1  node_modules/msw/lib/core/GraphQLHandler-Cu4Xvg4S.d.ts:1

I'd try fine-grained imports as recommended here, but I cannot figure out from which file to import HttpResponse.

@kettanaito
Copy link
Member Author

@jgosmann, does setting skipLibCheck: true in your tsconfig.json help?

@kettanaito
Copy link
Member Author

Jest patch

If anybody's having this issue still, try #2258. It ships require('graphql') for CJS but keeps a dynamic import for ESM.

@flaviodelgrosso
Copy link

Hello, it seems the fix is not working for me. I tried granular import and it's working but when I try to import HttpResponse from 'msw/core/HttpResponse' tsc complains about not finding corresponding type declarations.

@kettanaito
Copy link
Member Author

@flaviodelgrosso, we don't export HttpResponse as a standalone export right now so TypeScript errors on that. We should discover why the fixes aren't working.

@maxdavidson
Copy link

This broke our Webpack setup, since it can't resolve the graphql dependency during compilation if it's not installed.

We needed to manually add a resolution fallback of false in our Webpack config to make it work:

resolve: {
  fallback: {
    graphql: false
  }
}

@jgosmann
Copy link

jgosmann commented Sep 5, 2024

@jgosmann, does setting skipLibCheck: true in your tsconfig.json help?

Yes, the error disappears with that setting, but I'd see this rather as a workaround than a fix.

@kettanaito
Copy link
Member Author

@maxdavidson, when I was working with webpack last time in Next.js 14 support, I've encountered a weird bug that webpack extracts dynamic imports and evaluates them on the root scope of the module. I wonder if that's a behavior consistent to webpack in general, not just in Next.js.

Can you please submit a reproduction repo with webpack for me to look at?

@grebenyuksv
Copy link

grebenyuksv commented Sep 27, 2024

We've managed to solve the following error in Storybook+Vite by downgrading to 2.4.0 (the one right before this PR):

    error: [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.

    error: TypeError: Failed to fetch dynamically imported module: http://127.0.0.1:6006/node_modules/.cache/storybook/1c3385a5d25e538d10b518b310c74d3ca2690b6aaffeadccd74da79736171f86/sb-vite/deps/graphql-JJE5APMP.js?v=50c46e56
    TypeError: Failed to fetch dynamically imported module: http://127.0.0.1:6006/node_modules/.cache/storybook/1c3385a5d25e538d10b518b310c74d3ca2690b6aaffeadccd74da79736171f86/sb-vite/deps/graphql-JJE5APMP.js?v=50c46e56

Seems like the series of the recent attempts to make the graphql package truly optional has led to this. This error might totally have been caused by a misconfiguration on our side, I'm just sharing the knowledge. Narrowing the problem down to this has already taken many hours, so I can't dive deeper right now.

@kettanaito
Copy link
Member Author

@grebenyuksv, this has been rolled back on the latest release of MSW. Please update to the latest. Thanks.

@grebenyuksv
Copy link

Wow, thanks for responding so fast, @kettanaito! Rolled back or re-worked?

In the currently latest 2.4.9, I'm seeing graphql imported in a slightly different way than this PR suggests but still dynamically: https://github.com/mswjs/msw/blame/452686d27cc84c1a18262e9d0b18cf64b13aa71d/src/core/utils/internal/parseGraphQLRequest.ts#L50

async function parseQuery(query: string): Promise<ParsedGraphQLQuery | Error> {
  ...
  const { parse } = require('graphql')
  ...
}

For our case, however, only the static import works. By no means am I saying the problem lies inside MSW. On the contrary, I'm inclined to think it's somewhere in the other parts of our Storybook + Vite + [apparently, Jest-based] @storybook/test-runner. I don't have time to dive deeper, so I just wanted to share the workaround in case anyone encounters this.

@kettanaito
Copy link
Member Author

@grebenyuksv, thanks for pointing that out. It looks like I've reverted that change poorly. It should be a top-level import like it was before. Would you like to open a PR to fix that?

@grebenyuksv
Copy link

Ok, @kettanaito, will do within 48 hours.

grebenyuksv added a commit to grebenyuksv/msw that referenced this pull request Sep 30, 2024
@grebenyuksv
Copy link

It doesn't let me request your review, @kettanaito 🤔 #2298

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