Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 48508c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📦 Bundle Stats —
|
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 2.4 KB | - |
| Internal (gzip) | 926 B | - |
| Bundled (raw) | 11.16 MB | - |
| Bundled (gzip) | 2.10 MB | - |
| Import time | 429ms | +13ms, +3.2% |
bin:sanity
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 782 B | - |
| Internal (gzip) | 423 B | - |
| Bundled (raw) | 9.91 MB | - |
| Bundled (gzip) | 1.78 MB | - |
| Import time | 1.64s | +63ms, +4.0% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @sanity/cli-core
Compared against main (5ffc312c)
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 127.8 KB | - |
| Internal (gzip) | 33.1 KB | - |
| Bundled (raw) | 22.96 MB | - |
| Bundled (gzip) | 3.71 MB | - |
| Import time | 337ms | +15ms, +4.8% |
🗺️ @sanity/cli-core:. treemap too large to embed · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @sanity/cli-build
Compared against main (5ffc312c)
@sanity/cli-build/_internal/build
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 118.7 KB | - |
| Internal (gzip) | 30.0 KB | - |
| Bundled (raw) | 18.11 MB | - |
| Bundled (gzip) | 3.66 MB | - |
| Import time | 717ms | +28ms, +4.0% |
@sanity/cli-build/_internal/env
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 1.8 KB | - |
| Internal (gzip) | 644 B | - |
| Bundled (raw) | 1.30 MB | - |
| Bundled (gzip) | 331.9 KB | - |
| Import time | 138ms | +8ms, +6.2% |
@sanity/cli-build/_internal/extract
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 42.8 KB | - |
| Internal (gzip) | 10.3 KB | - |
| Bundled (raw) | 11.89 MB | - |
| Bundled (gzip) | 2.24 MB | - |
| Import time | 448ms | +21ms, +5.0% |
@sanity/cli-build/_internal/manifest
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 31.0 KB | - |
| Internal (gzip) | 7.8 KB | - |
| Bundled (raw) | 842.0 KB | - |
| Bundled (gzip) | 213.9 KB | - |
| Import time | 442ms | +14ms, +3.3% |
🗺️ ./_internal/env · ./_internal/manifest · 2 treemaps too large to embed · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — create-sanity
Compared against main (5ffc312c)
| Metric | Value | vs main (5ffc312) |
|---|---|---|
| Internal (raw) | 908 B | - |
| Internal (gzip) | 483 B | - |
| Bundled (raw) | 931 B | - |
| Bundled (gzip) | 491 B | - |
| Import time | ❌ ChildProcess denied: node | - |
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Array fields lose their array type
- resolveFromType now prints the array type when the path ends on the field, and only unwraps into elements when more path segments remain.
- ✅ Fixed: Index parameters are not inferred
- Single-index placeholders like [$i] are collected like slice bounds, substituted before parse, and forced to number so named fetch requires a correctly typed param.
- ✅ Fixed: Plugin session is not project-scoped
- Each create() keeps its own session and a closed flag so projects do not share watchers and dispose before import settles skips starting one.
Or push these changes by commenting:
@cursor push cbaf857684
Preview (cbaf857684)
diff --git a/packages/@sanity/groq-files/src/__tests__/params.test.ts b/packages/@sanity/groq-files/src/__tests__/params.test.ts
--- a/packages/@sanity/groq-files/src/__tests__/params.test.ts
+++ b/packages/@sanity/groq-files/src/__tests__/params.test.ts
@@ -29,13 +29,49 @@
})
})
- it('types dereferences, `in` arrays, slices, and unconstrained placeholders', () => {
+ it('types dereferences, `in` arrays, slices, indexes, and unconstrained placeholders', () => {
expect(params('*[_type == "post" && author->name == $name]')).toEqual({name: 'string'})
expect(params('*[_type == "post" && _id in $ids]')).toEqual({ids: 'string[]'})
expect(params('*[$start..$end]')).toEqual({end: 'number', start: 'number'})
+ expect(params('*[_type == "post"][$i]')).toEqual({i: 'number'})
+ expect(params('*[$start..$end][$i]')).toEqual({end: 'number', i: 'number', start: 'number'})
expect(params('*[_type == "post" && $q]')).toEqual({q: 'unknown'})
})
+ it('keeps array field types when comparing the whole array', () => {
+ const withTags: SchemaType = structuredClone(schema)
+ const post = withTags.find((entry) => entry.type === 'document' && entry.name === 'post')
+ if (!post || post.type !== 'document') throw new Error('expected post document')
+ post.attributes.tags = {
+ type: 'objectAttribute',
+ value: {of: {type: 'string'}, type: 'array'},
+ }
+ post.attributes.authors = {
+ type: 'objectAttribute',
+ value: {
+ of: {
+ attributes: {
+ name: {type: 'objectAttribute', value: {type: 'string'}},
+ },
+ type: 'object',
+ },
+ type: 'array',
+ },
+ }
+ const result = inferGroqParams('*[_type == "post" && tags == $tags]', withTags)
+ expect(result.ok).toBe(true)
+ if (!result.ok) return
+ expect(Object.fromEntries(result.params.map((param) => [param.name, param.type]))).toEqual({
+ tags: 'string[]',
+ })
+ const nested = inferGroqParams('*[_type == "post" && authors.name == $name]', withTags)
+ expect(nested.ok).toBe(true)
+ if (!nested.ok) return
+ expect(Object.fromEntries(nested.params.map((param) => [param.name, param.type]))).toEqual({
+ name: 'string',
+ })
+ })
+
it('reports a syntax error with a utf-16 offset', () => {
const result = inferGroqParams('[', schema)
expect(result.ok).toBe(false)
diff --git a/packages/@sanity/groq-files/src/params.ts b/packages/@sanity/groq-files/src/params.ts
--- a/packages/@sanity/groq-files/src/params.ts
+++ b/packages/@sanity/groq-files/src/params.ts
@@ -40,17 +40,19 @@
export type ParamInference = {error: unknown; ok: false} | {ok: true; params: GroqParam[]}
const SLICE_PARAM = /\[\s*(?:\$(\w+)|\d+)\s*\.\.\.?\s*(?:\$(\w+)|\d+)\s*\]/g
+/** Single-index access like `[$i]` — ranges are covered by `SLICE_PARAM`. */
+const INDEX_PARAM = /\[\s*\$(\w+)\s*\]/g
/**
* Infers a params object from `$param` placeholders.
* Comparisons against schema fields use that field's type, including string
- * enums. Slice bounds are numbers. A placeholder we cannot place is `unknown`,
- * which still makes the key required.
+ * enums. Slice bounds and single-index access are numbers. A placeholder we
+ * cannot place is `unknown`, which still makes the key required.
*/
export function inferGroqParams(query: string, schema: SchemaType): ParamInference {
- const sliceParams = sliceParamNames(query)
+ const indexParams = indexParamNames(query)
const substituted: Record<string, number> = {}
- for (const name of sliceParams) substituted[name] = 0
+ for (const name of indexParams) substituted[name] = 0
let ast: GroqNode
try {
@@ -69,7 +71,7 @@
}
walk(ast, {documents: undefined, schema}, see, add)
- for (const name of sliceParams) add(name, 'number')
+ for (const name of indexParams) add(name, 'number')
return {
ok: true,
@@ -260,12 +262,15 @@
return found.filter((name) => current.includes(name))
}
-function sliceParamNames(query: string): string[] {
+function indexParamNames(query: string): string[] {
const names: string[] = []
for (const match of query.matchAll(SLICE_PARAM)) {
if (match[1]) names.push(match[1])
if (match[2]) names.push(match[2])
}
+ for (const match of query.matchAll(INDEX_PARAM)) {
+ if (match[1]) names.push(match[1])
+ }
return names
}
diff --git a/packages/@sanity/groq-files/src/schemaTypes.ts b/packages/@sanity/groq-files/src/schemaTypes.ts
--- a/packages/@sanity/groq-files/src/schemaTypes.ts
+++ b/packages/@sanity/groq-files/src/schemaTypes.ts
@@ -155,10 +155,12 @@
return parts.length === 0 ? undefined : unionTypeStrings(parts)
}
+ // Keep the array type when the path ends on the field itself (`tags == $tags`).
+ // Only unwrap into elements when more segments remain (`authors.name`).
+ if (index >= segments.length) return printType(type, inlineLookup(schema), seen)
+
if (type.type === 'array') return resolveFromType(type.of, segments, index, schema, seen)
- if (index >= segments.length) return printType(type, inlineLookup(schema), seen)
-
const segment = segments[index]
if (!segment) return undefined
diff --git a/packages/@sanity/groq-files/typescript-plugin.cjs b/packages/@sanity/groq-files/typescript-plugin.cjs
--- a/packages/@sanity/groq-files/typescript-plugin.cjs
+++ b/packages/@sanity/groq-files/typescript-plugin.cjs
@@ -19,18 +19,18 @@
* @param {unknown} _modules
*/
function init(_modules) {
- /** @type {{close(): void} | undefined} */
- let session
-
return {
create(info) {
+ /** @type {{close(): void} | undefined} */
+ let session
+ let closed = false
const directory = info.project.getCurrentDirectory()
const schema =
info.config && typeof info.config.schema === 'string' ? info.config.schema : undefined
const logger = info.project.projectService && info.project.projectService.logger
void load()
.then((mod) => {
- session?.close()
+ if (closed) return
session = mod.startGroqTypeScriptPlugin({
directory,
logger: logger
@@ -51,6 +51,7 @@
const dispose = info.languageService.dispose
if (typeof dispose === 'function') {
info.languageService.dispose = () => {
+ closed = true
session?.close()
dispose.call(info.languageService)
}You can send follow-ups to the cloud agent here.
Coverage Delta
No baseline available — showing current coverage for 8 changed files Overall Coverage
|
Review:
|
| schemaNote: loaded.note, | ||
| schemaPath, | ||
| }) | ||
| const output = `${file}.d.ts` |
There was a problem hiding this comment.
blocker — allowArbitraryExtensions does not look for <name>.groq.d.ts. TypeScript's convention for arbitrary extensions is <name>.d.groq.ts ({basename}.d.{ext}.ts, TS 5.0 release notes). I verified with the repo's TS 6.0.3 by running generateGroqDeclarations on example/ and type-checking the result:
moduleResolution: NodeNext+posts.groq.d.ts(whatexample/tsconfig.jsondoes):TS2307: Cannot find module './posts.groq' or its corresponding type declarationsfor both imports.NodeNext+posts.d.groq.ts: passes.Bundler+posts.groq.d.ts: passes, but only because of node10-style extension probing (posts.groq→posts.groq.d.ts);allowArbitraryExtensionsis irrelevant there.Bundler+posts.d.groq.tswithout the flag:TS6263: Module './posts.groq' was resolved to '.../posts.d.groq.ts', but '--allowArbitraryExtensions' is not set— so flag +.d.groq.tsis the combination the README should document.
As shipped, the TS 5/6 path only works in bundler/node10 and the committed example fails. Suggest const output = file.replace(/\.groq$/, '.d.groq.ts'), teach shouldRegenerate (line 69) to skip .d.groq.ts, fix README line 60 and example/tsconfig.json, and add a test that imports a .groq file through tsc (today's typecheck.test.ts imports ./posts.js, so it never exercises resolution). This also matches TS 7.1's own declaration-emit naming for mapped files (App.d.svelte.ts).
There was a problem hiding this comment.
Fixed: declarationPath now writes <name>.d.groq.ts. README, example/tsconfig.json, and .gitignore updated. declarations.test.ts copies example/ to a temp dir, generates the declarations, and runs the real tsc (NodeNext + allowArbitraryExtensions) over main.ts plus a check.ts of type assertions. shouldRegenerate moved to PR C and only matches *.groq or the exact schema path, so .d.groq.ts never triggers.
|
|
||
| ## TypeScript 5 and 6 | ||
|
|
||
| The language service cannot invent a virtual file. Two opt-in fallbacks write the same TypeScript next to the query, and `allowArbitraryExtensions` makes `import x from './file.groq'` resolve to `file.groq.d.ts`: |
There was a problem hiding this comment.
blocker (same as declarations.ts) — this is not how allowArbitraryExtensions behaves: it resolves ./file.groq to file.d.groq.ts. With the current .groq.d.ts output the flag does nothing, and the import only resolves under bundler/node10 resolution (and fails under node16/nodenext).
There was a problem hiding this comment.
Fixed: the README now documents posts.d.groq.ts with allowArbitraryExtensions, links the TS 5.0 release notes, and shows the NodeNext config the test runs.
| "noEmit": true, | ||
| "rootDir": "." | ||
| }, | ||
| "files": ["main.ts", "posts.groq.d.ts", "postBySlug.groq.d.ts"] |
There was a problem hiding this comment.
blocker (follow-up) — this project does not type-check as committed (TS2307 on both .groq imports under NodeNext, see the declarations.ts comment), and nothing checks it: the package tsconfig.json includes only ./src and eslint ignores example/**. Either wire the example into a test or check:types (generate the declarations into a temp dir, run tsc), or drop it; a non-compiling example inside the package is worse than none.
There was a problem hiding this comment.
Fixed: example/ is now wired into declarations.test.ts, which generates the declarations in a temp copy and type-checks main.ts and a check.ts of assertions with the repo's tsc. The example also gained postsByRole.groq (enum param) and postsByTags.groq (array param). PR B runs the same example through TS 7.1's content mapper.
| const fetchParams = options.params.length === 0 ? '' : `, params: __GroqFileParams` | ||
| const result = isTypeName(options.resultType) ? options.resultType : 'unknown' | ||
|
|
||
| return `${notes.length > 0 ? `${notes.join('\n')}\n` : ''}${options.generated} |
There was a problem hiding this comment.
should-fix — splicing the entire TypeGenerator output into every virtual module has three consequences I reproduced against typescript@7.1.0-beta:
- The
.groqmodule's type-level exports do not match the runtime module.import {internalGroqTypeReferenceTo} from './posts.groq'type-checks (codegen emitsexport declare const internalGroqTypeReferenceTo: unique symbol), but the bundled module only exports['default', 'fetch', 'query'], so it isundefinedat runtime. Every schema type (Post,Author,AllSanitySchemaTypes, ...) is also importable from every.groqfile. internalGroqTypeReferenceTois a freshunique symbolper virtual module, soPostfroma.groqandPostfromb.groqare different types. Two.groqfiles with identical query text whose result includes a reference object register the sameSanityQuerieskey with non-identical types → TS2717. (GenerateTypesOptions.overloadClientMethodsin codegen documents exactly this failure mode.)- Every
.groqfile, and every generated.d.groq.tson disk for TS 5/6, carries the whole schema. With a real schema that is thousands of lines per query file, regenerated on every schema change.
The PR lists "one shared schema module" as a follow-up, but it changes the public shape of every .groq module, so I'd rather see it before this ships, even as experimental: emit schema types once (a single shared module, supplemental output, or global declarations) and have the virtual module only export default, export {fetch, query}, plus a type-only result export. At minimum add a test asserting that the virtual module's value exports equal the runtime module's exports.
There was a problem hiding this comment.
Fixed, all three. The module's value exports are now exactly default, query, and run (matching the runtime module; evaluate.test.ts asserts the export surface), plus type-only Result and Params. Schema aliases are no longer exported: only the ones the result references (transitively) are inlined as local types, so a projection carries no schema at all. internalGroqTypeReferenceTo is imported from @sanity/groq-files/types (one Symbol.for declaration) instead of being declared per file, so identical queries in two files produce identical types; the tsc test includes a duplicate-query file and compiles without TS2717.
| const generator = options.generator ?? new TypeGenerator() | ||
| const variable = variableNameFromFilename(options.filename) | ||
| const {code} = await generator.generateTypes({ | ||
| overloadClientMethods: true, |
There was a problem hiding this comment.
should-fix — overloadClientMethods: true makes codegen emit declare module "@sanity/client" { interface SanityQueries extends globalThis.SanityQueries {} }. With typescript@7.1.0-beta --runExternalCode, a program where no .ts file imports @sanity/client (I only imported from ./posts.groq; @sanity/client was installed in node_modules) fails with:
posts.groq(45,16): error TS2664: Invalid module name in augmentation, module '@sanity/client' cannot be found.
This location is in virtual code produced by the content mapper '@sanity/groq-files@0.0.0' and has no corresponding location in the original file.
As soon as any file in the program imports @sanity/client, the error disappears; the same text as a .d.groq.ts under TS 6.0.3 is fine either way. Projects that reach the client indirectly (next-sanity, @sanity/sdk, sanity) will see this on every .groq file in the editor. Options: only emit the declare module bridge when @sanity/client resolves from root (the mapper has it), or drop the bridge, rely on the global SanityQueries registry that codegen already emits, and tighten the peer range to the client versions that read it.
There was a problem hiding this comment.
Fixed: dropped the bridge (overloadClientMethods: false) and render the global SanityQueries entry directly. The peer range ^7.27.0 || ^8.5.0 already matches the client versions that read the global (sanity-io/client#1319, #1320). PR B's TS 7.1 smoke test runs --runExternalCode on a program where @sanity/client is only type-imported and passes with no TS2664.
| name: '@sanity/groq-files', | ||
| })) | ||
|
|
||
| export const groqFilesPlugin = { |
There was a problem hiding this comment.
should-fix — typing every factory as () => {name: string} breaks consumers at the type level for three of the five bundlers. Checked with TS 6.0.3 against the real plugin interfaces:
types.ts(5,7): error TS2741: Property 'setup' is missing in type 'GroqBundlerPlugin' but required in type 'Plugin'. // esbuild
types.ts(9,7): error TS2741: Property 'apply' is missing in type 'GroqBundlerPlugin' but required in type 'WebpackPluginInstance'. // webpack; rspack has the same shape
Vite and Rollup only require name, so they happen to work. esbuild/webpack/rspack users will need as any. unplugin ships per-bundler types (UnpluginInstance<...>['esbuild'] etc.); if api-extractor cannot follow the optional rspack types, declare minimal structural return types locally ({name: string; setup(build: ...): void}, {name: string; apply(compiler: ...): void}) rather than erasing them.
Related: ./vite, ./rollup, ./webpack, ./esbuild and ./rspack in package.json all resolve to this same module, so import {webpackPlugin} from '@sanity/groq-files/vite' works. Either make each subpath export only its bundler's plugin (the unplugin convention) or keep a single entry; five identical subpaths are confusing to document.
There was a problem hiding this comment.
Fixed: createUnplugin<undefined, false> plus minimal structural return types per bundler ({name; setup(build)} for esbuild, {apply(compiler)} for webpack/rspack, {name} for Vite/Rollup). plugin.test.ts asserts assignability to Vite's and esbuild's real Plugin types. Each subpath (/vite, /rollup, /webpack, /esbuild, /rspack) is its own entry exporting only that bundler's factory as the default export; the root exports the five as groqFilesPlugin.
|
|
||
| run() | ||
| try { | ||
| watcher = watch(directory, {recursive: true}, (_event, filename) => { |
There was a problem hiding this comment.
should-fix — this watches the whole project directory recursively, including node_modules. On Linux, Node implements recursive: true with one inotify watch per directory: on this checkout that is ~13.5k directories and ~0.9s just to set up, and on a large npm/yarn tree it can exceed fs.inotify.max_user_watches (8192/65536 on common defaults). When that happens the watcher emits 'error', and there is no error listener here, so it becomes an uncaught exception inside tsserver. Related: shouldRegenerate (declarations.ts line 70) only compares the base name, so any schema.json anywhere under the root, including node_modules/**/schema.json, triggers a full regeneration of every .groq file.
Suggest: attach an error handler, exclude SKIP_DIRECTORIES from the watch (watch the directories that actually contain .groq files plus the schema file, instead of the root), and compare the full path to the resolved schema path.
There was a problem hiding this comment.
Addressed in PR C (#2038): no recursive watch. The plugin watches, non-recursively, the directories that contain .groq files (from the generator's output), the project root, and the schema's directory, so node_modules is never touched. Every watcher has an error handler that logs to tsserver, and regeneration triggers only for *.groq or the exact resolved schema path.
| */ | ||
| function init(_modules) { | ||
| /** @type {{close(): void} | undefined} */ | ||
| let session |
There was a problem hiding this comment.
should-fix — session lives in init()'s scope, but tsserver calls create() once per project. With two tsconfigs (tsconfig.json + tsconfig.node.json, or any monorepo) the second create() runs session?.close() and tears down the first project's watcher, and the patched dispose of either project closes whichever session is current. Move session inside create() (one per project), and consider returning a proxy of the language service rather than mutating info.languageService.dispose.
There was a problem hiding this comment.
Addressed in PR C (#2038): the session lives inside create(), one per project, and create() returns a proxy of the language service whose dispose closes that session, instead of mutating info.languageService.dispose. Disposing before the ESM import settles sets a closed flag so no session starts. Covered by a test that requires the built .cjs.
| * Module source a bundler evaluates for a `.groq` import. | ||
| * The default export is the query string. `fetch(client, params)` runs it. | ||
| */ | ||
| export function createRuntimeModule(content: string): string { |
There was a problem hiding this comment.
should-fix (API design) — two things about the per-file API, which is the real public contract of this package and will be hard to change later:
- The named export is
fetch.import {fetch} from './posts.groq'silently shadowsglobalThis.fetchin the importing module; the README has to alias it everywhere (fetch as fetchPosts), which is a hint that the name fights the platform.run/execute/fetchQuerywould avoid the footgun. - There is no way to pass
client.fetch's third argument (perspective,useCdn,filterResponse,stega,next: {revalidate}, ...). Next.js users in particular rely onclient.fetch(query, params, {next}); today they have to drop back toclient.fetch(getPosts, params)and lose the strict params. A passthroughfetch(client, params, options?)here and invirtualModule.ts(typed off the client, e.g.Parameters<C['fetch']>[2]) keeps the typed path usable.
There was a problem hiding this comment.
Fixed both: the named export is run, and run(client, params, options?) forwards the third argument to client.fetch. options is typed from the passed client (GroqRunOptions<Client> in @sanity/groq-files/types reads the last parameter of Client['fetch'] and omits filterResponse/returnQuery, since run always returns the filtered result). The tsc test passes perspective/useCdn/tag and asserts filterResponse is rejected.
| '@sanity/sdk': | ||
| specifier: ^3.8.0 | ||
| version: 3.8.0 | ||
| version: 3.9.1 |
There was a problem hiding this comment.
should-fix — unrelated lockfile churn: @sanity/sdk moves from 3.8.0 to 3.9.1 (and the 3.8.0 snapshot is dropped) because the lockfile was regenerated without --frozen-lockfile. Please keep this PR's lockfile diff to the new packages/@sanity/groq-files importer, unplugin, and webpack-virtual-modules; bump the SDK separately if wanted.
There was a problem hiding this comment.
Fixed: the lockfile is main's plus the new packages/@sanity/groq-files importer and its dependencies (unplugin, webpack-virtual-modules, @sanity/worker-channels, esbuild). @sanity/sdk stays at 3.8.0; pnpm install --frozen-lockfile passes.
| if (buffer.length < bodyEnd) return undefined | ||
| try { | ||
| const request = JSON.parse(buffer.toString('utf8', bodyStart, bodyEnd)) as unknown | ||
| if (!isRequest(request)) return {consumed: bodyEnd, item: {kind: 'parse'}} |
There was a problem hiding this comment.
nit — valid JSON that is not a request (and a bad Content-Length) is answered with -32700 Parse error and id: null; JSON-RPC 2.0 reserves -32700 for unparseable JSON and uses -32600 Invalid Request (echoing the id when present) for this case. For the record, the framing held up in my probes: split mid-header/mid-body, two messages in one chunk, notifications without id, and the process exits cleanly on stdin EOF, which is the shutdown signal the TS host relies on.
There was a problem hiding this comment.
Fixed in PR B (#2037): unparseable JSON is -32700; valid JSON that is not a request, and a bad Content-Length, is -32600 Invalid Request echoing the id when present (null otherwise). Tests cover split/packed frames, notifications, both error kinds, end while a request is in flight, and the error event. Thanks for probing the framing.
| function checkSource(): string { | ||
| return `import {type SanityClient} from '@sanity/client' | ||
|
|
||
| import getBySlug, {fetch as fetchBySlug} from './postBySlug.js' |
There was a problem hiding this comment.
should-fix (tests) — this is the only type-level test and it imports ./posts.js/./postBySlug.js, i.e. the rendered text as ordinary modules. It never exercises import ... from './x.groq' through either real path (allowArbitraryExtensions + declaration file, or the content mapper), which is how the .groq.d.ts naming bug got through. Suggestions:
- Run
tscon a fixture that imports./posts.groqwith the generated declaration (TS 6 is already a devDependency). - Add an opt-in/CI smoke test against
typescript@7.1.0-beta(tsc --runExternalCode -p example). I did this manually and the round-trip works, including pipelinedtransforms and aschema.jsonedit under--watch(newconfigIdentity, re-transform, types update), so it is worth locking in. - Cover the enum param (
role: "admin" | "member"rejecting'nope'),string[]params,unknownparams, andfetch(client)rejecting a second argument on a no-param query. expectTypeOf<SanityClient['fetch']>().toBeFunction()on line 57 asserts nothing about this package.- Coverage from
pnpm test:coverage packages/@sanity/groq-files:schemaTypes.ts58% stmts / 55% branches,params.ts70% / 56%,jsonrpc.ts80% / 67% (parse error, badContent-Length,errorevent,endwhile a request is in flight are untested),schemaFile.ts70% (invalid JSON / non-array branches). CONTRIBUTING asks for maximum coverage on new code. - Only Vite (rolldown under the hood) is exercised in
plugin.test.ts; esbuild is already in the lockfile and a ten-line test covers theonLoadpath (I ran one manually; it works). - Line 23 writes into
packages/@sanity/groq-files/.tmpinside the repo;tmpdir()like the other tests avoids polluting the checkout.
There was a problem hiding this comment.
Done, point by point: declarations.test.ts runs the real tsc over the example importing ./x.groq through generated .d.groq.ts, with assertions for the enum param rejecting 'nope', string[] params, a no-param query rejecting params, filterResponse rejected, and client.fetch(getPosts) equal to run's result. The TS 7.1 smoke test is in PR B (#2037), opt-in via GROQ_FILES_TSC71, and passed against 7.1.0-beta. The toBeFunction() assertion is gone. plugin.test.ts bundles with esbuild as well as Vite and imports the output. Everything writes to tmpdir(). New focused tests for schemaTypes.ts, params.ts, schemaFile.ts, and virtualModule.ts; package coverage is ~95% statements.
|
|
||
| export type ParamInference = {error: unknown; ok: false} | {ok: true; params: GroqParam[]} | ||
|
|
||
| const SLICE_PARAM = /\[\s*(?:\$(\w+)|\d+)\s*\.\.\.?\s*(?:\$(\w+)|\d+)\s*\]/g |
There was a problem hiding this comment.
nit — the slice regex runs over raw query text, so it also fires inside string literals: *[_type == "post" && title == "[0..$x]"] infers a required x: number that the query never reads (and substitutes 0 for it before parsing). Low likelihood, but a param that does not exist is a confusing error for the user; consider deriving slice parameters from the AST after a first parse attempt, or at least skipping matches inside string literals. Also $role != "admin" widens the schema enum to string (widened, line 179) where keeping the literal would preserve the union; your call.
There was a problem hiding this comment.
Fixed the first: the slice regex runs over a copy of the query with string-literal contents blanked out, so title == "[0..$x]" no longer invents $x (test added). Kept != widening to string on purpose: $role != "admin" is commonly used to pass a value outside the enum, and narrowing it would reject valid calls. Happy to flip it if you feel strongly.
| end() {}, | ||
| }, | ||
| }, | ||
| } as unknown as NonNullable<GenerateTypesOptions['reporter']> |
There was a problem hiding this comment.
nit — the hand-rolled reporter cast through unknown will silently break when TypegenWorkerChannel changes. @sanity/worker-channels (already a transitive dependency via codegen) exposes new WorkerChannelReporter<TypegenWorkerChannel>(message => ...), which gives a typed reporter from a postMessage callback; the evaluatedModules stream can be collected from those messages without the cast. Also _content on line 213 is unused.
There was a problem hiding this comment.
Fixed: the reporter is new WorkerChannelReporter<TypegenWorkerChannel>(message => ...) from @sanity/worker-channels, collecting the generatedSchemaTypes event and the evaluatedModules emission from typed messages. The unused _content parameter is gone.
| } | ||
| } | ||
|
|
||
| const root = configFileName.length > 0 ? dirname(configFileName) : process.cwd() |
There was a problem hiding this comment.
nit — per the protocol configFileName is "" for a project without a config file. TS 7.1 spawns the mapper with cwd set to the package's real path (probed: cwd=/.../node_modules/@sanity/groq-files), so this fallback resolves schema.json inside the mapper package and the diagnostic points users at a path under node_modules. Nobody hits it until an editor extension registers an inferred-project contribution, but it is cheap to handle explicitly (require an absolute options.schema, or resolve relative to the transformed file's directory at transform time).
Separately, isDirectRun() (line 211) is a fragile start condition; it happens to work because TS passes the absolute real path as argv[1], but a dedicated always-start entry (dist/bin/contentMapper.js) that imports the handler module removes the dependency on how the host spawns it, and sidesteps symlink/drive-letter-casing mismatches.
There was a problem hiding this comment.
Fixed in PR B (#2037): with configFileName === "" the mapper requires an absolute schema option; otherwise it reports an option diagnostic (code 4) and transforms untyped with a schema diagnostic, never touching process.cwd(). root for such projects is the transformed file's directory. isDirectRun() is replaced by a dedicated entry, dist/bin/contentMapper.js, referenced from typescript.contentMapper.exec.
| ] | ||
| } | ||
| }, | ||
| "engines": { |
There was a problem hiding this comment.
nit — engines is ^22.18.0 || >=24.11.0 while every other package in the repo is >=22.12; nothing here needs the stricter range (Promise.withResolvers landed in Node 22.0), so this only rejects otherwise-fine Node 22.12–22.17 installs. Also the root entry exports createRuntimeModule/isGroqRequest and ./declarations exports shouldRegenerate/schemaBaseName, which read as internals; keeping the public surface to the plugin factories and generateGroqDeclarations makes an eventual 1.0 easier.
There was a problem hiding this comment.
Fixed: engines is >=22.12 like the rest of the repo. The root entry exports only groqFilesPlugin and the four plugin types; ./declarations exports only generateGroqDeclarations (and declarationPath); createRuntimeModule, isGroqRequest, shouldRegenerate, and schemaBaseName are internal now.
| const SCHEMA = new URL('../../example/schema.json', import.meta.url) | ||
|
|
||
| describe('typescript plugin', () => { | ||
| it('writes declarations on startup and when a .groq file is saved', async () => { |
There was a problem hiding this comment.
nit — a real recursive watcher with a 4s ceiling is a flakiness risk on CI runners (recursive-watch setup on Linux scales with directory count). Consider injecting the watcher, or unit-testing shouldRegenerate + the debounce/regenerate path without touching the filesystem watcher.
There was a problem hiding this comment.
Addressed in PR C (#2038): the watcher is injectable, so debounce, filtering, rewatching, run coalescing, and error reporting are tested under fake timers with no filesystem watcher. One remaining real-fs test uses the new non-recursive watcher on a temp directory with two subdirectories, which is a handful of inotify watches rather than a recursive walk.
c087093 to
0d9c5bb
Compare
0d9c5bb to
48508c7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
inunwraps union array types- Added elementOf to unwrap each top-level union arm before stripping []/Array<>, so $x in across multi-doc array fields yields a valid element union.
Or push these changes by commenting:
@cursor push 72abcb62c9
Preview (72abcb62c9)
diff --git a/packages/@sanity/groq-files/src/__tests__/params.test.ts b/packages/@sanity/groq-files/src/__tests__/params.test.ts
--- a/packages/@sanity/groq-files/src/__tests__/params.test.ts
+++ b/packages/@sanity/groq-files/src/__tests__/params.test.ts
@@ -47,6 +47,57 @@
expect(params('*[_type == "post" && title match $term]')).toEqual({term: 'string'})
})
+ it('unwraps each arm when `in` sees a union of array fields across documents', () => {
+ const multi: SchemaType = [
+ ...schema,
+ {
+ attributes: {
+ _type: {type: 'objectAttribute', value: {type: 'string', value: 'page'}},
+ items: {
+ type: 'objectAttribute',
+ value: {
+ of: {
+ attributes: {a: {type: 'objectAttribute', value: {type: 'number'}}},
+ type: 'object',
+ },
+ type: 'array',
+ },
+ },
+ tags: {
+ type: 'objectAttribute',
+ value: {of: {type: 'number'}, type: 'array'},
+ },
+ },
+ name: 'page',
+ type: 'document',
+ },
+ {
+ attributes: {
+ _type: {type: 'objectAttribute', value: {type: 'string', value: 'note'}},
+ items: {
+ type: 'objectAttribute',
+ value: {
+ of: {
+ attributes: {b: {type: 'objectAttribute', value: {type: 'string'}}},
+ type: 'object',
+ },
+ type: 'array',
+ },
+ },
+ },
+ name: 'note',
+ type: 'document',
+ },
+ ]
+ const result = (query: string) => {
+ const inferred = inferGroqParams(query, multi)
+ if (!inferred.ok) throw new Error(syntaxDiagnostic(inferred.error).message)
+ return Object.fromEntries(inferred.params.map((param) => [param.name, param.type]))
+ }
+ expect(result('*[ $tag in tags]')).toEqual({tag: 'number | string'})
+ expect(result('*[ $x in items]')).toEqual({x: '{a: number} | {b: string}'})
+ })
+
it('types dereferences, `in` arrays, slices, and unconstrained placeholders', () => {
expect(params('*[_type == "post" && author->name == $name]')).toEqual({name: 'string'})
expect(params('*[_type == "post" && _id in $ids]')).toEqual({ids: 'string[]'})
diff --git a/packages/@sanity/groq-files/src/__tests__/schemaTypes.test.ts b/packages/@sanity/groq-files/src/__tests__/schemaTypes.test.ts
--- a/packages/@sanity/groq-files/src/__tests__/schemaTypes.test.ts
+++ b/packages/@sanity/groq-files/src/__tests__/schemaTypes.test.ts
@@ -1,7 +1,7 @@
import {type SchemaType} from 'groq-js'
import {describe, expect, it} from 'vitest'
-import {arrayOf, lookupPathType, unionTypeStrings} from '../schemaTypes.js'
+import {arrayOf, elementOf, lookupPathType, unionTypeStrings} from '../schemaTypes.js'
const schema: SchemaType = [
{
@@ -121,4 +121,12 @@
expect(arrayOf('"a" | "b"')).toBe('Array<"a" | "b">')
expect(arrayOf('{x: number}')).toBe('Array<{x: number}>')
})
+
+ it('unwraps each arm of a union of array types', () => {
+ expect(elementOf('string[]')).toBe('string')
+ expect(elementOf('Array<"a" | "b">')).toBe('"a" | "b"')
+ expect(elementOf('number[] | string[]')).toBe('number | string')
+ expect(elementOf('Array<{a: number}> | Array<{b: string}>')).toBe('{a: number} | {b: string}')
+ expect(elementOf('string[] | number')).toBe('number | string')
+ })
})
diff --git a/packages/@sanity/groq-files/src/params.ts b/packages/@sanity/groq-files/src/params.ts
--- a/packages/@sanity/groq-files/src/params.ts
+++ b/packages/@sanity/groq-files/src/params.ts
@@ -1,6 +1,12 @@
import {GroqSyntaxError, parse, type SchemaType} from 'groq-js'
-import {arrayOf, lookupPathType, type PathSegment, unionTypeStrings} from './schemaTypes.js'
+import {
+ arrayOf,
+ elementOf,
+ lookupPathType,
+ type PathSegment,
+ unionTypeStrings,
+} from './schemaTypes.js'
/**
* groq-js 2.0 tags every AST node with `type`, but that union is not exported
@@ -174,10 +180,7 @@
}
const inner = valueType(node, context)
- if (inner === undefined) return undefined
- if (inner.endsWith('[]')) return inner.slice(0, -2)
- if (inner.startsWith('Array<') && inner.endsWith('>')) return inner.slice('Array<'.length, -1)
- return inner
+ return inner === undefined ? undefined : elementOf(inner)
}
function widened(value: unknown): string | undefined {
diff --git a/packages/@sanity/groq-files/src/schemaTypes.ts b/packages/@sanity/groq-files/src/schemaTypes.ts
--- a/packages/@sanity/groq-files/src/schemaTypes.ts
+++ b/packages/@sanity/groq-files/src/schemaTypes.ts
@@ -56,6 +56,21 @@
return element.includes(' ') || element.includes('|') ? `Array<${element}>` : `${element}[]`
}
+/**
+ * Inverse of {@link arrayOf}: strip one array layer from each top-level union arm.
+ * Needed when {@link lookupPathType} joins fields across documents into
+ * `number[] | string[]` or `Array<{a: number}> | Array<{b: string}>`.
+ */
+export function elementOf(arrayType: string): string {
+ return unionTypeStrings(
+ splitTopLevelUnion(arrayType).map((part) => {
+ if (part.endsWith('[]')) return part.slice(0, -2)
+ if (part.startsWith('Array<') && part.endsWith('>')) return part.slice('Array<'.length, -1)
+ return part
+ }),
+ )
+}
+
function printType(type: TypeNode, lookupInline: InlineLookup, seen: Set<string>): string {
switch (type.type) {
case 'array': {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 48508c7. Configure here.
| const inner = valueType(node, context) | ||
| if (inner === undefined) return undefined | ||
| if (inner.endsWith('[]')) return inner.slice(0, -2) | ||
| if (inner.startsWith('Array<') && inner.endsWith('>')) return inner.slice('Array<'.length, -1) |
There was a problem hiding this comment.
in unwraps union array types
Medium Severity
elementType strips one [] or wrapping Array<...> from the whole printed type string. When valueType returns a union of array types, that check treats the union as a single array and emits a wrong or invalid param type, so $x in field across multiple document types can fail typecheck or accept the wrong value.
Reviewed by Cursor Bugbot for commit 48508c7. Configure here.
|
Superseded by sanity-io/codegen#176 (typed |
Description
Adds an experimental, opt-in package
@sanity/groq-filesso a GROQ file can be imported as a typed query. The import is the query string, carrying its result and params types along with it. Nothing else is exported.The reference is Sébastien Lorber on TS 7.1 content mappers importing JSON
as const: the mapper only makes the import carry precise types. The result type, including[0]asT | null, comes from@sanity/codegen'sTypeGenerator.$paramplaceholders become a params object, including string enums and array fields from the schema. Existing CLI commands,defineQuery, and@sanity/clientare unchanged.How the result type is carried
The brand is the query's literal type intersected with two optional phantom properties, the same device
@sanity/sdkuses for__schemaTypeName. The value stays assignable tostringand to its own literal, works in template literals and===, and nothing is declared globally: nodeclare global, noSanityQueriesaugmentation, nodeclare module "@sanity/client". Two files with the same query text have identical types and cannot collide. The brand is keyed by plain strings so a client can read it back structurally without depending on this package.There is no existing branded-query mechanism to reuse:
defineQueryis an identity on the literal, andClientReturn/SanityQueries/SanityQueriesByResourceare all keyed by query text. A brand and the global registry are also mutually exclusive, since indexingSanityQuerieswith an intersection type yieldsunknown, so the registry path was dropped rather than combined.What
@sanity/clientinfers today:client.fetch(getPosts)accepts the value but returnsany, becauseClientReturn<G>only looksGup inSanityQueries. PassGroqResult<typeof getPosts>as the type argument in the meantime. The minimal client-side change (separate concern, not in this stack) is to check the brand before the registry:plus
G extends {readonly __groqParams?: infer P} ? P : QueryParamsonparams.@sanity/sdk'sResolveQueryResultwould need the same brand-first check.This is PR A of a stack of three:
generateGroqDeclarationsfor TypeScript 5 and 6.Changes since the last review
runhelper is gone, and so is the namedqueryexport. The runtime module is exactlyexport default "<query text>".GroqQuery<"<text>", Result, Params>;ResultandParamsremain as type exports.declare global { interface SanityQueries }block,GroqClient,GroqRunOptions, and the@sanity/clientpeer dependency (the package no longer refers to the client at all).client.fetch<GroqResult<typeof q>>(q), with a section on how the type is carried and what the client would need.What to review
packages/@sanity/groq-files/src/types.ts(the brand),virtualModule.ts(module text),params.ts(param inference),evaluateGroqModule.ts(TypeGenerator glue),plugin.ts+ per-bundler entries,declarations.ts.pnpm-workspace.yaml, Vitest project, knip entry, lockfile importer for the new package.Testing
33 unit tests.
declarations.test.tscopiesexample/to a temp dir, generates*.d.groq.ts, and runs the realtscover acheck.tsof type assertions: the default export is assignable tostringand its literal, a different literal is rejected,GroqResult/GroqParamsand the module'sResult/Paramsagree (enum,string[],T | null,Record<string, never>), two files with the same text have equal types,keyof typeof moduleis exactly'default',SanityQuerieshas nothing registered, and today'sclient.fetch(q)isanywhileclient.fetch<GroqResult<typeof q>>(q)is the inferred type.plugin.test.tsbundles with Vite and esbuild and checks the bundled module has onlydefault.Build,
build:types, typecheck, lint (--max-warnings 0), format, publint, and knip pass.Notes for release
N/A