feat!: msw/graphql, graphql as an optional peer dependency#2691
feat!: msw/graphql, graphql as an optional peer dependency#2691kettanaito wants to merge 20 commits into
msw/graphql, graphql as an optional peer dependency#2691Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Andarist, hi 👋 Mentioning you just in case so this wouldn't conflict with the work you're doing. TL;DR |
|
This is not a new feature for the end user, but a change to the auxiliary tools (dependencies etc) right? According to conventional commits this would be the |
|
@THETCR, this changes how you import GraphQL handlers in a breaking way. This is entirely user-facing: -import { graphql } from 'msw'
+import { graphql } from 'msw/graphql'As such, this will be released as a breaking change. There's no way of achieving a lean module graph without taking most of the things out of the root export. |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
commit: |
UpdateWould be nice to merge after #2760 since this PR also drops the GraphQL plugin. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/typings/graphql-typed-document-node.test-d.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnrelated:
TypedDocumentNodereverted to a value import.Line 3 dropped the
typekeyword from theTypedDocumentNodeimport, even though it's only used in a type position (line 5). This looks like an unintended side-effect of the import-splitting edit rather than part of the GraphQL entrypoint migration. tsd only statically analyzes.test-d.tsfiles rather than executing them, so this won't break at runtime, but it's inconsistent with type-only usage and could cause friction if this file is ever processed under stricter compiler settings (e.g.isolatedModules/verbatimModuleSyntax).Proposed fix
-import { TypedDocumentNode } from '`@graphql-typed-document-node/core`' +import type { TypedDocumentNode } from '`@graphql-typed-document-node/core`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/typings/graphql-typed-document-node.test-d.ts` around lines 1 - 3, The TypedDocumentNode import in the graphql-typed-document-node tsd test should remain type-only since it is only used in a type position. Update the import in the test file to use the type modifier again, keeping the existing HttpResponse and graphql value imports unchanged, and make sure the change is confined to the test import setup rather than the GraphQL migration logic.test/typings/graphql-typed-document-string.test-d.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame unrelated type-import regression as
graphql-typed-document-node.test-d.ts.
DocumentTypeDecoration(line 3) is used only in a type position but is no longer imported withtype. Likely collateral from the mechanical import split rather than an intentional change.Proposed fix
-import { DocumentTypeDecoration } from '`@graphql-typed-document-node/core`' +import type { DocumentTypeDecoration } from '`@graphql-typed-document-node/core`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/typings/graphql-typed-document-string.test-d.ts` around lines 1 - 3, The type-only import for DocumentTypeDecoration in graphql-typed-document-string.test-d.ts was split incorrectly, leaving it as a value import even though it is only used in a type position. Update the import alongside the other typings in this test file so DocumentTypeDecoration is imported with type, matching the pattern used in graphql-typed-document-node.test-d.ts and preserving the mechanical import split behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 126-130: Add a types condition to the ./graphql export in
package.json so the new subpath matches the other public exports and TypeScript
can resolve declarations correctly. Update the export map alongside the existing
module-sync, module, import, and default entries for the graphql subpath,
keeping the structure consistent with the other package export entries.
In `@tsup.config.ts`:
- Around line 63-79: The graphql subpath export is missing a types entry, so
TypeScript consumers cannot resolve the generated declarations. Update the
package.json export for the graphql entry to include a types condition alongside
the existing import/require targets, pointing it to the graphql declaration file
produced by graphqlConfig. Keep the existing build setup in tsup.config.ts
unchanged and align the graphql export shape with the other subpaths.
---
Nitpick comments:
In `@test/typings/graphql-typed-document-node.test-d.ts`:
- Around line 1-3: The TypedDocumentNode import in the
graphql-typed-document-node tsd test should remain type-only since it is only
used in a type position. Update the import in the test file to use the type
modifier again, keeping the existing HttpResponse and graphql value imports
unchanged, and make sure the change is confined to the test import setup rather
than the GraphQL migration logic.
In `@test/typings/graphql-typed-document-string.test-d.ts`:
- Around line 1-3: The type-only import for DocumentTypeDecoration in
graphql-typed-document-string.test-d.ts was split incorrectly, leaving it as a
value import even though it is only used in a type position. Update the import
alongside the other typings in this test file so DocumentTypeDecoration is
imported with type, matching the pattern used in
graphql-typed-document-node.test-d.ts and preserving the mechanical import split
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e2f807b-6c9e-44b3-ab70-d5eb8ccc52d9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
config/plugins/esbuild/graphQLImportPlugin.tsconfig/replaceCoreImports.jspackage.jsonsrc/core/experimental/frames/http-frame.test.tssrc/core/experimental/frames/websocket-frame.test.tssrc/core/experimental/handlers-controller.test.tssrc/core/handlers/RequestHandler.tssrc/core/index.tssrc/core/utils/executeHandlers.tssrc/core/utils/internal/isHandlerKind.test.tssrc/core/utils/internal/parseGraphQLRequest.test.tssrc/graphql/graphql-handler.test.tssrc/graphql/graphql-handler.tssrc/graphql/graphql.test.tssrc/graphql/graphql.tssrc/graphql/index.tssrc/graphql/parse-graphql-request.tstest/browser/graphql-api/anonymous-operation.mocks.tstest/browser/graphql-api/anonymous-operation.test.tstest/browser/graphql-api/cookies.mocks.tstest/browser/graphql-api/custom-predicate.mocks.tstest/browser/graphql-api/custom-predicate.test.tstest/browser/graphql-api/document-node.mocks.tstest/browser/graphql-api/errors.mocks.tstest/browser/graphql-api/extensions.mocks.tstest/browser/graphql-api/link.mocks.tstest/browser/graphql-api/logging.mocks.tstest/browser/graphql-api/multipart-data.mocks.tstest/browser/graphql-api/mutation.mocks.tstest/browser/graphql-api/operation-reference.mocks.tstest/browser/graphql-api/operation.mocks.tstest/browser/graphql-api/query.mocks.tstest/browser/graphql-api/response-patching.mocks.tstest/browser/graphql-api/variables.mocks.tstest/browser/msw-api/setup-worker/listHandlers.mocks.tstest/browser/setup/webpackHttpServer.tstest/node/graphql-api/anonymous-operations.test.tstest/node/graphql-api/batched-queries.apollo.test.tstest/node/graphql-api/batched-queries.batched-execute.test.tstest/node/graphql-api/compatibility.node.test.tstest/node/graphql-api/content-type.test.tstest/node/graphql-api/cookies.test.tstest/node/graphql-api/custom-predicate.node.test.tstest/node/graphql-api/extensions.node.test.tstest/node/graphql-api/response-patching.node.test.tstest/node/graphql-api/typed-document-node.test.tstest/node/graphql-api/typed-document-string.test.tstest/node/msw-api/setup-server/listHandlers.node.test.tstest/node/msw-api/setup-server/scenarios/graphql.node.test.tstest/node/regressions/many-request-handlers-jsdom.test.tstest/node/regressions/many-request-handlers.test.tstest/node/regressions/mixed-graphql-http-with-query-in-body.node.test.tstest/support/alias.tstest/typings/custom-handler.test-d.tstest/typings/custom-resolver.test-d.tstest/typings/graphql-custom-predicate.test-d.tstest/typings/graphql-typed-document-node.test-d.tstest/typings/graphql-typed-document-string.test-d.tstest/typings/graphql.test-d.tstest/typings/regressions/request-handler-type.test-d.tstest/typings/setup-server.test-d.tstest/typings/setup-worker.test-d.tstsup.config.ts
💤 Files with no reviewable changes (3)
- config/plugins/esbuild/graphQLImportPlugin.ts
- test/browser/setup/webpackHttpServer.ts
- src/core/index.ts
Motivation
mswleaks thegraphqlnamespace through the root entrypoints, which makesgraphqlimport problematic for developers who aren't even using GraphQL. This negatively impacts the bundle footprint as well.This is a change in a set of changes aimed to improve DX and bundle size of
mswacross the board.graphqlnamespace and any related values/types are no longer exported frommsw. Please use a designatedmsw/graphqlexport path to work with GraphQL requests.msw/core/graphqlexport path has been changed tomsw/graphqlfor brevity.Todos
feat/3.0. This is not meant to be released separately.defineNetwork) #2650 to benefit fromsetupWorker/setupServeracceptingAnyHandlerand notRequestHandler, which is incompatible withGraphQLHandler