Repository navigation
Conversation
🦋 Changeset detectedLatest commit: bad942e The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tim-smart
left a comment
There was a problem hiding this comment.
Stage-1 verdict: changes requested. The existing runtime suite passes (65 tests), and type tests pass on TS 5.9.3 and 6.0.3 (33 assertions per target), but the paging cursor constraint and null-data handling have uncovered contract gaps. The public make implementation also retains the cast/any pattern explicitly excluded by EFF-1913 and the EFF-1830 rough-draft verdict.
SSE, WebSocket, and the HTTP subscriptions option are intentionally deferred to stage 8; the HTTP subscribe stub is not a finding. No production code or tests were modified. This is a COMMENT review because the authenticated GitHub account is also the PR author and cannot submit REQUEST_CHANGES on its own PR.
- pages / items reject a required or non-string $after variable and accept an optional string-only one - data: null without errors is a DecodeError even when the result codec accepts null, on both the strict and the partial path These fail against a9ff888 on purpose; the fixes land in a separate implementation run.
tim-smart
left a comment
There was a problem hiding this comment.
Re-reviewed stage 1 at 6f13daa. The three previous findings are addressed; I found no remaining production correctness blocker in this follow-up.
- Paging now requires an optional after variable accepting string. I accept the Architect's clarification that after?: string is also valid: the helper omits it initially and never sends null. Required and non-string cursors are rejected.
- Null or missing data without errors now produces DecodeError before either result decoder runs, including codecs accepting null.
- The client construction now carries operation-specific types through the runtime and method implementations. The remaining mapped-record, middleware-next and paging-overload boundaries are confined and documented. I no longer consider the make assertion a blocker; it is not defining the public API from an untyped implementation.
The retryableOnly change is correct on inspection. It rejects non-GraphQLClientError and non-retryable inputs before calling the schedule step. Cause.done is handled by Channel.retry by re-failing with the original stream error, not by completing successfully or replacing it with the schedule's terminal value.
One non-blocking test follow-up remains in packages/effect/test/graphql/GraphQLClient.subscriptions.test.ts: add a focused middleware subscribe failure regression. Assert the exact middleware error is preserved and there is only one subscription attempt. Cover the default schedule with a null error (which would expose the old retryAfter property-access defect), and a custom schedule with a step callback that must not execute (which pins the early guard rather than merely eventual non-retryability). This is worth a separate test-only run because none of the current nine subscription tests exercises a middleware error.
Validation rerun: 66 runtime tests pass; 39 type assertions per target pass on TS 5.9.3 and 6.0.3 (78 total); pnpm check passes. No source or tests changed. SSE/WebSocket transport work remains deferred to stage 8.
API correctness gate: pass. Recommend the focused retry regression before closing out stage-1 validation. This is a COMMENT review because the authenticated account is also the PR author.
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
…ng, comment characters Tests-only run after the stage-2 review of PR #8979. - Descriptions on operations, fragments and variable definitions, which the September 2025 edition (the current published edition EFF-1829 names) adds to the executable grammar: conformance cases, AST shape expectations, the shorthand/field/end-of-input diagnostics, and the extension diagnostic reworded as graphql-js 16.12+ words it. The compact printer drops these descriptions, so print/parse round trips are compared without them. - Compact printing settled as token-minimal: a single space only where two adjacent non-punctuator tokens would otherwise merge. Three value literals lose the separator after `]` or `}`. - Comments and other positions must reject unpaired surrogates, which are not source characters under the "any Unicode scalar value" rule; astral and control characters in comments stay valid. Production code is untouched; the new expectations fail until the follow-up implementation run lands.
…r, attribution Follow-up to the stage-2 review of PR #8979; production code only. - Descriptions on operations, fragments and variable definitions, as the September 2025 edition allows. The AST nodes gain `description`, the parser reads it, the shorthand rejects it with graphql-js's wording, and the extension diagnostic uses the 16.12+ wording. The printer drops descriptions from the compact document. - Comments are scanned by Unicode scalar value, so an unpaired surrogate in a comment is reported like one anywhere else. - The printer is token-minimal: arguments, list values and object fields no longer get a separator when punctuation already separates them. - Lexer.ts and Parser.ts carry the graphql-js v16.14.2 MIT notice (Copyright (c) GraphQL Contributors), since both adapt its structure. Headers now name the September 2025 edition. 210 of 210 generator tests pass; pnpm lint, check and jsdocs --check pass.
- pages / items reject a required or non-string $after variable and accept an optional string-only one - data: null without errors is a DecodeError even when the result codec accepts null, on both the strict and the partial path These fail against a9ff888 on purpose; the fixes land in a separate implementation run.
…ng, comment characters Tests-only run after the stage-2 review of PR #8979. - Descriptions on operations, fragments and variable definitions, which the September 2025 edition (the current published edition EFF-1829 names) adds to the executable grammar: conformance cases, AST shape expectations, the shorthand/field/end-of-input diagnostics, and the extension diagnostic reworded as graphql-js 16.12+ words it. The compact printer drops these descriptions, so print/parse round trips are compared without them. - Compact printing settled as token-minimal: a single space only where two adjacent non-punctuator tokens would otherwise merge. Three value literals lose the separator after `]` or `}`. - Comments and other positions must reject unpaired surrogates, which are not source characters under the "any Unicode scalar value" rule; astral and control characters in comments stay valid. Production code is untouched; the new expectations fail until the follow-up implementation run lands.
…r, attribution Follow-up to the stage-2 review of PR #8979; production code only. - Descriptions on operations, fragments and variable definitions, as the September 2025 edition allows. The AST nodes gain `description`, the parser reads it, the shorthand rejects it with graphql-js's wording, and the extension diagnostic uses the 16.12+ wording. The printer drops descriptions from the compact document. - Comments are scanned by Unicode scalar value, so an unpaired surrogate in a comment is reported like one anywhere else. - The printer is token-minimal: arguments, list values and object fields no longer get a separator when punctuation already separates them. - Lexer.ts and Parser.ts carry the graphql-js v16.14.2 MIT notice (Copyright (c) GraphQL Contributors), since both adapt its structure. Headers now name the September 2025 edition. 210 of 210 generator tests pass; pnpm lint, check and jsdocs --check pass.
8a6494f to
6d4a227
Compare
|
We tried the pkg.pr.new build at 091d1a1 against a production CMS GraphQL API that we call today from an Effect v4 server through gql.tada and our own Effect wrapper. Setup
Results
Gaps we hit
We are happy to open PRs for 1 and 2 if the direction fits. Runnable reproductions for each gap, with an invented schema and TS 7.0.2 / 6.0 side by side: https://github.com/Tarekkharsa/effect-graphql-feedback |
Thanks for testing it out! The gaps should be addressed now. |
|
Thanks, that was fast! I re-ran the probes against 7ee8dca: https://github.com/Tarekkharsa/effect-graphql-feedback
Generation, One note: |
cbe82e6 to
506f923
Compare
Closes EFF-1911
Closes EFF-1977
Closes EFF-1978
Closes EFF-1979
Adds a typed GraphQL client to core
effectas the experimentaleffect/graphqlmodule, and a standalone generator,@effect/graphql-generator, that emits operations for it from.graphqldocuments. The generator needs onlyeffectand@effect/platform-node. The lexer, parser, schema model and validation are built in, andgraphql-jsisn't needed.effect/graphql(coreeffect)GraphQL.query/mutation/subscription(name, { document, variables?, result })are plain operation values.GraphQL.middleware(op, Tag)attaches middleware to one operation. Variables are passed decoded and encoded through their Schema, so custom scalar codecs apply to inputs and results.GraphQLGroup.make(...ops)/merge(...groups). The generator emits one group per file. Group middleware runs outside operation middleware.GraphQLMiddleware.Service<M, { requires?, error? }>()(key)tags whose value is{ execute, subscribe }, plusmapRequest(f)for headers, auth and extensions.GraphQLClient.make(group, { subscriptionRetry? })gives one method per operation, called as(variables?, { headers?, urlParams?, context?, partial? }?). Queries and mutations return anEffect, subscriptions aStream. Retryable transport failures resubscribe through the whole middleware chain (exponential from 500 ms x1.5, capped at 5 s, honouringretryAfter).errorsfails a call by default with aResponseError, which carries the response's HTTPstatus(unset over graphql-ws and for graphql-sse events). AResponseErroris never retryable, whatever the status.{ partial: true }on queries and mutations returns{ data, errors }.GraphQLClient.pages/itemspage forward through cursor connections over a nullable$after: String. A missing$after,pageInfoornodesis a compile error.GraphQLClient.makeTest(group)(handlers)is a typed mock client for tests, wired in withLayer.succeed(Service, ...). Optional handlers keyed by operation name receive the caller's variables and options and return decoded results (anEffect, or aStreamfor subscriptions). Nothing is encoded, decoded or validated and no middleware runs.{ partial: true }gives{ data, errors: [] }, methods keep.operationsopages/itemswork, and an operation without a handler dies with anUnimplementedError.GraphQLClient.layerTest(Service, group)(handlers)wrapsmakeTestin a service layer. Bind it once asstatic readonly layerTest = GraphQLClient.layerTest(Service, group)to useService.layerTest({ Operation: () => Effect.succeed(result) })in tests. Handler inference comes from the group, and a group that cannot implement the service is rejected.GraphQLResolver.ordered(method, { variables, path, typename? })andfindById(method, { key, variables, path, typename? })build aRequestResolverover a client method, matching results to keys by position (nodes(ids:)) or by key (searches, collections);GraphQLResolver.request(resolver)gives(key) => Effect.pathis checked one key at a time,typenamenarrows a__typenameunion, and anullor missing entry fails withNoSuchElementError. Underordered, a GraphQL error at[...path, i]fails only entryi; other GraphQL errors fail the batch, and so doespageInfo.hasNextPage: truenext to the list. Delay and batch size are composed with.pipe.GraphQLProtocol. The service'sexecuteandsubscribeanswer{ body, status? }rather than a bare body, so a scripted transport returns{ body: { data } }. The client decodesbodyand copiesstatusontoExecutionResult, where middleware can read or rewrite it.GraphQLRequestcarries the operationkindandurlParamsalongsideheaders.layerHttp({ url, queryMethod? }):POSTfor queries and mutations by default.queryMethod: "GET"sends queries asGETwithquery,operationName,variablesandextensionsas URL params, while mutations stayPOST. Subscriptions use graphql-sse distinct mode on the same URL. Per-call or middlewareurlParamsare appended to every HTTP request.layerWebSocket({ url, connectionParams?, headers?, keepAlive?, idleTimeout? }): one lazy, multiplexed graphql-ws socket with ack gating, client ping and close-code classification.layerHttp({ url, subscriptions: { webSocket } }):POSTplus graphql-ws subscriptions.GraphQLClientError { operation, reason }, withResponseError,TransportError,EncodeError,DecodeErrorandPaginationErrorreasons. All areSchema.TaggedErrors withisRetryable, andretryAfteris read fromRetry-After.GraphQL.otherTypenameandGraphQL.enumLiterals, the lenient decoding helpers generated code uses for__typenameunions and result enums.@effect/graphql-generatorgraphqlgen [--config <path>] [--watch | --check], built oneffect/cli, loadsgraphql.config.ts(defineConfigfrom@effect/graphql-generator/Config, using graphql-config'sschema/documentskeys). The config is loaded with a plainimport(), so it needs Bun, Deno or Node >= 22.18."<module>#<export>"Schema codecs, and unmapped custom scalars decode asSchema.Jsonwith one warning.foo.graphqlgeneratesfoo.graphql.tsnext to it: one operation value per definition, aVariables/Resulttype namespace per operation, a Schema per fragment and a group per file. One shared module holds the scalars, enums, input objects and__typenameunions the operations reach.__typenameunions with a lenient catch-all member. Also covered:@skip/@include(optional keys),@oneOfinputs, subscriptions, descriptions and@deprecatedas JSDoc.--checkoutput, 2 for config errors or invalid CLI usage (including unknown flags and--watch --check). Stale generated files are deleted, but only when they carry the generated header.--watchdebounces changes, caches parsed inputs, writes only changed files, keeps the last good output on error and re-imports the config when it changes.Generator.generate(config, { cwd })is the programmatic core. It never writes to disk or stdout..graphql.tsfiles thatpnpm checktypechecks, and a runtime test runs them against a mockHttpClient.Docs
packages/tools/graphql-generator/README.mdcovers install, runtime requirement, config, scalars, the lint/format exclusion for*.graphql.ts, CLI flags and exit codes, watch mode, paging with$after, partial results and batching.ai-docs/src/52_graphql/has a getting-started example (an inline query shaped like generated output, an HTTP client service with auth middleware, a call with a note on partial results, and cursor paging), a testing example that replaces the client service with a boundGitHub.layerTesthelper, and a batching example withGraphQLResolver.ordered. They have no generated fixtures or generator dependency.LLMS.mdis regenerated. Subscriptions and error handling are covered in theeffect/graphqlAPI reference.Other changes
@effect/graphql-generatoris added to the changesetsfixedversion group, thetsconfig.tests.jsonpaths, the vitest projects and the root README catalog.*.graphql.ts) are excluded from oxlint and dprint.The
Sockethandshake-headers change that graphql-ws needs shipped separately onmain.