From cec9bb33c5f73a48d8d05c8a02d484c77fc5eabb Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Tue, 10 Nov 2026 12:17:21 -0600 Subject: [PATCH 1/8] Fail the build when a prepare target's variables don't match its run query Add check.js, which Shopify CLI runs before building a JavaScript function when this package has it (Shopify/cli#8829). It exits non-zero, failing the build, when a prepare target returns variables its run query doesn't accept, and warns about variables it can't check. The check is moved here from Shopify/cli#8829: it follows what the prepare export returns, ignoring type annotations and casts, and validates it against the run query's variable definitions. Values it can't follow, such as parsed JSON, are warnings rather than errors, so valid code still builds. --- .changeset/prepare-variables.md | 5 + check.js | 32 +++ prepare-variables.js | 385 +++++++++++++++++++++++++++++++ tests/check.test.cjs | 72 ++++++ tests/prepare-variables.test.cjs | 209 +++++++++++++++++ 5 files changed, 703 insertions(+) create mode 100644 .changeset/prepare-variables.md create mode 100644 check.js create mode 100644 prepare-variables.js create mode 100644 tests/check.test.cjs create mode 100644 tests/prepare-variables.test.cjs diff --git a/.changeset/prepare-variables.md b/.changeset/prepare-variables.md new file mode 100644 index 0000000..587649a --- /dev/null +++ b/.changeset/prepare-variables.md @@ -0,0 +1,5 @@ +--- +"@shopify/shopify_function": minor +--- + +Fail the build when a JavaScript function's prepare target returns variables its run query doesn't accept, and warn about variables it can't check. Needs a Shopify CLI that runs this package's `check.js`. diff --git a/check.js b/check.js new file mode 100644 index 0000000..6d7c1c7 --- /dev/null +++ b/check.js @@ -0,0 +1,32 @@ +#!/usr/bin/env node +// Shopify CLI runs this, when the package has it, before bundling a JavaScript function: +// +// node check.js --function +// +// The function JSON is {handle, directory, entry, schema, targets: [{target, export, inputQuery}]}, where `export` is +// the JavaScript function the CLI bundles. It exits non-zero, failing the build, when a prepare target returns +// variables its run query doesn't accept, and warns about variables it can't check. +const fs = require("node:fs"); +const { parseArgs } = require("node:util"); +const { prepareVariablesProblems } = require("./prepare-variables.js"); + +try { + const { values } = parseArgs({ options: { function: { type: "string" } } }); + if (!values.function) throw new Error("Usage: check.js --function "); + const fn = { schema: "schema.graphql", targets: [], ...JSON.parse(fs.readFileSync(values.function, "utf8")) }; + const problems = prepareVariablesProblems(fn); + const name = fn.handle ? `The function extension "${fn.handle}"` : "The function"; + const list = (warning) => problems.filter((problem) => problem.warning === warning).map((problem) => problem.message); + + const warnings = list(true); + if (warnings.length > 0) { + console.warn(`${name} returns prepare variables that can't be checked against its run query\n${warnings.join("\n")}`); + } + const errors = list(false); + if (errors.length > 0) { + throw new Error(`${name} returns prepare variables its run query doesn't accept\n${errors.join("\n")}`); + } +} catch (error) { + console.error(error.message); + process.exitCode = 1; +} diff --git a/prepare-variables.js b/prepare-variables.js new file mode 100644 index 0000000..761123c --- /dev/null +++ b/prepare-variables.js @@ -0,0 +1,385 @@ +// Checks what each prepare target's export returns against the variables its run target's input query declares. +// Type annotations and casts are ignored, and values are followed back to where they're created. Values that can't be +// followed are warnings rather than passes. +const fs = require("node:fs"); +const path = require("node:path"); +const ts = require("typescript"); +const { + GraphQLInputObjectType, + GraphQLNonNull, + Kind, + buildSchema, + getNamedType, + isEnumType, + isInputObjectType, + isListType, + isNonNullType, + parse, + typeFromAST, +} = require("graphql"); + +// How each scalar's values appear in the variables, as Shopify CLI's codegen config types them. Other custom scalars, +// like `JSON`, accept anything. +const scalarKinds = { + Int: "number", + Float: "number", + String: "string", + Boolean: "boolean", + ID: "string", + Handle: "string", + Decimal: "string", + Date: "string", + DateTime: "string", + DateTimeWithoutTimezone: "string", + TimeWithoutTimezone: "string", + URL: "string", + Void: "null", +}; +const scalarKind = (named) => scalarKinds[named.name] ?? "unknown"; + +/** The prepare targets whose run target has an input query. */ +function preparePairs(targets) { + return targets.flatMap(({ target, export: name }) => { + const run = targets.find((other) => other.target === target.replace(/\.prepare$/, ".run")); + return target.endsWith(".prepare") && name && run?.inputQuery ? [{ target, name, query: run.inputQuery }] : []; + }); +} + +/** + * Returns `{message, warning}` for each problem, where `message` is `file:line:column message`, relative to `directory`. + * Values that can't be checked are warnings, and mismatches are errors. + */ +function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets }) { + const pairs = preparePairs(targets); + if (pairs.length === 0) return []; + if (!entryPath) { + return [{ message: "The function has no entry file, such as src/index.js, to check its prepare targets in", warning: false }]; + } + const entry = path.resolve(directory, entryPath); + + const schema = buildSchema(fs.readFileSync(path.resolve(directory, schemaPath), "utf8")); + const options = { + allowJs: true, + strict: true, + noEmit: true, + skipLibCheck: true, + types: [], + lib: ["lib.es2022.d.ts"], + target: ts.ScriptTarget.ES2022, + module: ts.ModuleKind.ESNext, + moduleResolution: ts.ModuleResolutionKind.Bundler, + }; + const host = ts.createCompilerHost(options, true); + // Annotations and casts are claims nothing checks against the value (`@ts-ignore` silences the compiler), so they're + // blanked out of the function's own files, leaving types to come from the values. Parameter types are kept. Blanking + // keeps positions, so errors point at the original source. + const { getSourceFile } = host; + host.getSourceFile = (fileName, language, ...rest) => { + const file = getSourceFile(fileName, language, ...rest); + if (!file || file.isDeclarationFile || fileName.includes("/node_modules/")) return file; + const text = file.text.split(""); + const blank = (start, end) => { + for (let index = start; index < end; index++) if (text[index] !== "\n" && text[index] !== "\r") text[index] = " "; + }; + const visit = (node) => { + for (const tag of ts.getJSDocTags(node)) { + const typing = + ts.isJSDocTypeTag(tag) || ts.isJSDocReturnTag(tag) || ts.isJSDocSatisfiesTag(tag) || ts.isJSDocOverloadTag(tag); + if (typing && !ts.isParameter(tag.parent.parent)) blank(tag.pos, tag.end); + } + if (ts.isAsExpression(node) || ts.isSatisfiesExpression(node) || ts.isNonNullExpression(node)) { + blank(node.expression.end, node.end); + } else if (ts.isTypeAssertionExpression(node)) { + blank(node.type.pos - 1, node.expression.pos); + } else if ((ts.isCallExpression(node) || ts.isNewExpression(node)) && node.typeArguments) { + blank(node.typeArguments.pos - 1, node.typeArguments.end + 1); + } else if ( + (ts.isVariableDeclaration(node) || + ts.isPropertyDeclaration(node) || + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isArrowFunction(node) || + ts.isMethodDeclaration(node) || + ts.isGetAccessorDeclaration(node)) && + node.type + ) { + blank(node.type.pos - 1, node.type.end); + } + ts.forEachChild(node, visit); + }; + visit(file); + return ts.createSourceFile(fileName, text.join(""), language); + }; + const program = ts.createProgram([entry], options, host); + const checker = program.getTypeChecker(); + const entryFile = program.getSourceFile(entry); + const entrySymbol = entryFile && checker.getSymbolAtLocation(entryFile); + const problems = new Map(); + let muted = false; + + const report = (node, message, warning = false) => { + if (muted) return; + const file = node.getSourceFile(); + const { line, character } = file.getLineAndCharacterOfPosition(node.getStart()); + problems.set(`${path.relative(directory, file.fileName)}:${line + 1}:${character + 1} ${message}`, warning); + }; + const join = (base, key) => (base ? `${base}.${key}` : key); + const subject = (at) => (at ? `\`${at}\`` : "The result"); + const unchecked = (type) => Boolean(type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)); + + const unwrap = (node) => (ts.isParenthesizedExpression(node) ? unwrap(node.expression) : node); + + const functionOf = (node) => { + if (!node) return; + if (ts.isVariableDeclaration(node)) return node.initializer && functionOf(unwrap(node.initializer)); + const local = + !node.getSourceFile().isDeclarationFile && !program.isSourceFileFromExternalLibrary(node.getSourceFile()); + if (local && (ts.isFunctionDeclaration(node) || ts.isFunctionExpression(node) || ts.isArrowFunction(node))) { + return node.body && node; + } + }; + + const symbolOf = (node) => + ts.isShorthandPropertyAssignment(node.parent) && node.parent.name === node + ? checker.getShorthandAssignmentValueSymbol(node.parent) + : checker.getSymbolAtLocation(node); + + // A reference that could change the value after it's created: assigning to it or a member, deleting or + // incrementing a member, calling a method on it, or passing, aliasing or reassigning the binding itself. + const changes = (reference) => { + let top = reference; + while ( + (ts.isPropertyAccessExpression(top.parent) || ts.isElementAccessExpression(top.parent)) && + top.parent.expression === top + ) { + top = top.parent; + } + const { parent } = top; + if (top === reference) { + return !(ts.isSpreadAssignment(parent) || ts.isSpreadElement(parent) || ts.isTypeOfExpression(parent)); + } + return ( + (ts.isBinaryExpression(parent) && + parent.left === top && + parent.operatorToken.kind >= ts.SyntaxKind.FirstAssignment && + parent.operatorToken.kind <= ts.SyntaxKind.LastAssignment) || + ts.isDeleteExpression(parent) || + ((ts.isPrefixUnaryExpression(parent) || ts.isPostfixUnaryExpression(parent)) && + (parent.operator === ts.SyntaxKind.PlusPlusToken || parent.operator === ts.SyntaxKind.MinusMinusToken)) || + (ts.isCallExpression(parent) && parent.expression === top) + ); + }; + + for (const { name, query } of pairs) { + const queryPath = path.resolve(directory, query); + const queryName = path.relative(directory, queryPath); + const operation = parse(fs.readFileSync(queryPath, "utf8")).definitions.find( + (definition) => definition.kind === Kind.OPERATION_DEFINITION, + ); + const variables = new GraphQLInputObjectType({ + name: "Variables", + fields: Object.fromEntries( + (operation?.variableDefinitions ?? []).map((definition) => [ + definition.variable.name.value, + { type: typeFromAST(schema, definition.type), defaultValue: definition.defaultValue }, + ]), + ), + }); + const result = new GraphQLInputObjectType({ + name: "PrepareResult", + fields: { variables: { type: new GraphQLNonNull(variables) } }, + }); + const owner = (type) => (type === variables ? `declared by ${queryName}` : `a field of ${type.name}`); + const followed = new Map(); + const checkedReferences = new Set(); + const visiting = new Set(); + const changed = new Map(); + + // The variable an object literal property is written in, such as `variables` in `const variables = {a: 1}` + const bindingOf = (property) => { + const declaration = property.valueDeclaration; + const assignment = + declaration && (ts.isPropertyAssignment(declaration) || ts.isShorthandPropertyAssignment(declaration)); + const binding = assignment && declaration.parent.parent; + return binding && ts.isVariableDeclaration(binding) && ts.isIdentifier(binding.name) ? binding : undefined; + }; + // A variable changed or passed on after it's created may not hold what it was created with, so it's not checked + const skipChanged = (declaration, at) => { + const changes = changed.get(declaration); + for (const node of changes ?? []) { + report(node, `${subject(at)} is changed or passed on after it's created, so it can't be checked`, true); + } + return Boolean(changes); + }; + + const scalarMatches = (type, named) => { + if (isEnumType(named)) return type.isStringLiteral() && named.getValue(type.value) !== undefined; + switch (scalarKind(named)) { + case "string": + return Boolean(type.flags & ts.TypeFlags.StringLike); + case "number": + return ( + Boolean(type.flags & ts.TypeFlags.NumberLike) && + !(named.name === "Int" && type.isNumberLiteral() && !Number.isInteger(type.value)) + ); + case "boolean": + return Boolean(type.flags & ts.TypeFlags.BooleanLike); + default: + return false; + } + }; + + const typed = (type, expected, at, omittable, node) => { + const named = getNamedType(expected); + if (!isEnumType(named) && !isInputObjectType(named) && scalarKind(named) === "unknown") return; + if (unchecked(type)) return report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); + if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); + if (type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)) { + if (isNonNullType(expected) && !omittable) report(node, `${subject(at)} can be undefined, but ${expected} has no default`); + return; + } + if (type.flags & ts.TypeFlags.Null) { + if (isNonNullType(expected)) report(node, `${subject(at)} can be null, but ${expected} can't`); + return; + } + const inner = isNonNullType(expected) ? expected.ofType : expected; + if (isListType(inner)) { + const element = + (checker.isArrayType(type) || checker.isTupleType(type)) && checker.getIndexTypeOfType(type, ts.IndexKind.Number); + return element ? typed(element, inner.ofType, `${at}[]`, false, node) : typed(type, inner.ofType, at, false, node); + } + if (isInputObjectType(inner) && type.flags & ts.TypeFlags.Object && !checker.isArrayType(type)) { + if (checker.getIndexInfosOfType(type).length > 0) { + return report(node, `${subject(at)} can have any keys, so it can't be checked`, true); + } + const properties = checker.getPropertiesOfType(type); + const bindings = new Set(properties.map(bindingOf).filter(Boolean)); + if ([...bindings].map((binding) => skipChanged(binding, at)).some(Boolean)) return; + const fields = inner.getFields(); + for (const property of properties) { + const field = fields[property.name]; + const key = join(at, property.name); + // A property written in an object literal is followed to its value, keeping literal types (like an enum + // value) that the object's type widens + const declaration = property.valueDeclaration; + const assignment = + declaration && (ts.isPropertyAssignment(declaration) || ts.isShorthandPropertyAssignment(declaration)) + ? declaration + : undefined; + if (!field) { + report(assignment ?? node, `${subject(key)} isn't ${owner(inner)}`); + } else if (assignment) { + const binding = bindingOf(property); + if (binding) followed.set(binding, at); + const initializer = ts.isPropertyAssignment(assignment) ? assignment.initializer : assignment.name; + value(initializer, field.type, key, field.defaultValue !== undefined); + } else { + typed(checker.getTypeOfSymbol(property), field.type, key, field.defaultValue !== undefined, node); + } + } + for (const [key, field] of Object.entries(fields)) { + if (isNonNullType(field.type) && field.defaultValue === undefined && !checker.getPropertyOfType(type, key)) { + report(node, `${subject(join(at, key))} is missing, and ${field.type} has no default`); + } + } + return; + } + if (isInputObjectType(inner) || !scalarMatches(type, inner)) { + report(node, `${subject(at)} must be ${expected}, not ${checker.typeToString(type)}`); + } + }; + + const returns = (fn, expected, at, omittable) => { + const body = fn.body; + if (!body) return; + if (!ts.isBlock(body)) return value(body, expected, at, omittable); + const visit = (node) => { + if (ts.isReturnStatement(node)) { + if (node.expression) value(node.expression, expected, at, omittable); + else typed(checker.getUndefinedType(), expected, at, omittable, node); + } else if (!ts.isFunctionLike(node)) { + ts.forEachChild(node, visit); + } + }; + ts.forEachChild(body, visit); + }; + + const value = (node, expected, at, omittable) => { + const expression = unwrap(node); + const inner = isNonNullType(expected) ? expected.ofType : expected; + + if (ts.isIdentifier(expression)) { + const declaration = symbolOf(expression)?.valueDeclaration; + if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer) { + followed.set(declaration, at); + checkedReferences.add(expression); + if (skipChanged(declaration, at)) return; + return value(declaration.initializer, expected, at, omittable); + } + } else if (ts.isConditionalExpression(expression)) { + value(expression.whenTrue, expected, at, omittable); + return value(expression.whenFalse, expected, at, omittable); + } else if ( + ts.isBinaryExpression(expression) && + (expression.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken || + expression.operatorToken.kind === ts.SyntaxKind.BarBarToken) + ) { + const left = checker.getNonNullableType(checker.getTypeAtLocation(expression.left)); + typed(left, expected, at, omittable, expression.left); + return value(expression.right, expected, at, omittable); + } else if (ts.isCallExpression(expression)) { + const fn = functionOf(checker.getResolvedSignature(expression)?.declaration); + if (fn && !visiting.has(fn)) { + visiting.add(fn); + returns(fn, expected, at, omittable); + visiting.delete(fn); + return; + } + } else if (ts.isArrayLiteralExpression(expression) && isListType(inner)) { + for (const element of expression.elements) { + if (ts.isSpreadElement(element)) typed(checker.getTypeAtLocation(element), expected, at, omittable, element); + else value(element, inner.ofType, `${at}[]`, false); + } + return; + } + typed(checker.getTypeAtLocation(expression), expected, at, omittable, expression); + }; + + const exported = entrySymbol && checker.getExportsOfModule(entrySymbol).find((symbol) => symbol.name === name); + const resolved = exported && (exported.flags & ts.SymbolFlags.Alias ? checker.getAliasedSymbol(exported) : exported); + const fn = functionOf(resolved?.valueDeclaration); + if (!fn) { + problems.set(`${path.relative(directory, entry)} has no \`${name}\` function to check`, false); + continue; + } + // The first pass finds the variables the result is built from, and the second checks it, skipping the ones + // changed after they're created + muted = true; + returns(fn, new GraphQLNonNull(result), "", false); + muted = false; + for (const declaration of followed.keys()) { + const symbol = checker.getSymbolAtLocation(declaration.name); + const nodes = []; + const visit = (node) => { + if ( + ts.isIdentifier(node) && + node !== declaration.name && + !checkedReferences.has(node) && + symbolOf(node) === symbol && + changes(node) + ) { + nodes.push(node); + } + ts.forEachChild(node, visit); + }; + visit(declaration.getSourceFile()); + if (nodes.length > 0) changed.set(declaration, nodes); + } + visiting.clear(); + returns(fn, new GraphQLNonNull(result), "", false); + } + + return [...problems].map(([message, warning]) => ({ message, warning })); +} + +module.exports = { prepareVariablesProblems }; diff --git a/tests/check.test.cjs b/tests/check.test.cjs new file mode 100644 index 0000000..b22ed9d --- /dev/null +++ b/tests/check.test.cjs @@ -0,0 +1,72 @@ +const { test } = require("node:test"); +const assert = require("node:assert/strict"); +const { spawnSync } = require("node:child_process"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); + +const check = path.join(__dirname, "..", "check.js"); + +const schema = ` +scalar JSON +input LineSelector @oneOf { id: ID, handle: String } +type CartLine { id: ID! } +type Query { id: ID!, lines(selector: LineSelector!, first: Int): [CartLine!]! } +`; + +const targets = [ + { target: "test.target.prepare", export: "targetPrepare" }, + { target: "test.target.run", export: "targetRun", inputQuery: "src/run.graphql" }, +]; + +// Runs check.js for a function whose prepare target returns `variables`, as Shopify CLI does. +function runCheck(t, { variables = "{}", fn = {} } = {}) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "function-")); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + fs.mkdirSync(path.join(dir, "src")); + fs.writeFileSync(path.join(dir, "schema.graphql"), schema); + fs.writeFileSync(path.join(dir, "src/run.graphql"), "query Run($selector: LineSelector!, $first: Int) { lines(selector: $selector, first: $first) { id } }"); + fs.writeFileSync(path.join(dir, "src/index.js"), `export function targetPrepare() {\n return { variables: ${variables} };\n}\n`); + const json = path.join(dir, "function.json"); + fs.writeFileSync(json, JSON.stringify({ handle: "prepare-test", directory: dir, entry: "src/index.js", schema: "schema.graphql", targets, ...fn })); + return spawnSync(process.execPath, [check, "--function", json], { cwd: dir, encoding: "utf8" }); +} + +test("passes variables the run query accepts", (t) => { + const result = runCheck(t, { variables: '{ selector: { id: "gid://shopify/CartLine/1" }, first: 5 }' }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stderr, ""); +}); + +test("fails when a prepare target returns variables its run query doesn't accept", (t) => { + const result = runCheck(t, { variables: '{ selectr: { id: "gid://shopify/CartLine/1" }, first: "5" }' }); + assert.equal(result.status, 1); + assert.equal( + result.stderr, + [ + `The function extension "prepare-test" returns prepare variables its run query doesn't accept`, + "src/index.js:2:25 `variables.selectr` isn't declared by src/run.graphql", + 'src/index.js:2:77 `variables.first` must be Int, not "5"', + "src/index.js:2:23 `variables.selector` is missing, and LineSelector! has no default", + "", + ].join("\n"), + ); +}); + +test("warns, without failing, about variables it can't check", (t) => { + const result = runCheck(t, { variables: "{ selector: JSON.parse(\"{}\"), first: 5 }" }); + assert.equal(result.status, 0, result.stderr); + assert.equal( + result.stderr, + [ + `The function extension "prepare-test" returns prepare variables that can't be checked against its run query`, + "src/index.js:2:35 `variables.selector` is typed any, so it can't be checked", + "", + ].join("\n"), + ); +}); + +test("passes a function without prepare targets", (t) => { + const result = runCheck(t, { fn: { entry: undefined, targets: [targets[1]] } }); + assert.equal(result.status, 0, result.stderr); +}); diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs new file mode 100644 index 0000000..c8da88b --- /dev/null +++ b/tests/prepare-variables.test.cjs @@ -0,0 +1,209 @@ +const { describe, test } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const { prepareVariablesProblems } = require("../prepare-variables.js"); + +const schema = ` + scalar JSON + enum Mode { FAST SLOW } + input Rule { handle: String!, weight: Int } + type Query { shop: String } +`; +const runQuery = "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON) { shop }"; + +const paired = [ + { target: "cart.validations.generate.prepare", export: "cartValidationsGeneratePrepare" }, + { target: "cart.validations.generate.run", inputQuery: "src/run.graphql", export: "cartValidationsGenerateRun" }, +]; + +// Writes a function with `files` in src/ and returns its problems, without their locations. Warnings start with "warning: ". +function check(files, targets = paired) { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "prepare-")); + try { + fs.mkdirSync(path.join(directory, "src")); + fs.writeFileSync(path.join(directory, "schema.graphql"), schema); + fs.writeFileSync(path.join(directory, "src/run.graphql"), runQuery); + for (const [file, content] of Object.entries(files)) fs.writeFileSync(path.join(directory, "src", file), content); + return prepareVariablesProblems({ directory, entry: "src/index.js", schema: "schema.graphql", targets }).map( + ({ message, warning }) => `${warning ? "warning: " : ""}${message.replace(/^\S+:\d+:\d+ /, "")}`, + ); + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +} + +const prepare = (body) => ({ "index.js": `export function cartValidationsGeneratePrepare(input) {\n${body}\n}` }); + +describe("prepareVariablesProblems", () => { + test("accepts variables the run query declares", () => { + assert.deepEqual( + check( + prepare(` + const region = String(input.region) + return { + variables: { + handle: region ? \`rule-\${region.toLowerCase()}\` : 'default', + limit: undefined, + mode: region === 'eu' ? 'FAST' : null, + rules: [{handle: 'a', weight: 1}], + data: JSON.parse(input.data), + }, + }`), + ), + [], + ); + }); + + test("ignores functions without a prepare target", () => { + assert.deepEqual( + check({}, [{ target: "cart.validations.generate.run", inputQuery: "src/run.graphql", export: "run" }]), + [], + ); + }); + + test("rejects keys the run query does not declare", () => { + assert.deepEqual(check(prepare(`return {variables: {handel: 'a'}}`)), [ + "`variables.handel` isn't declared by src/run.graphql", + "`variables.handle` is missing, and String! has no default", + ]); + }); + + test("rejects values of the wrong type", () => { + assert.deepEqual( + check(prepare(`return {variables: {handle: 1, limit: 1.5, mode: 'MEDIUM', rules: [{handle: 'a', weight: '1'}]}}`)), + [ + "`variables.handle` must be String!, not 1", + "`variables.limit` must be Int, not 1.5", + '`variables.mode` must be Mode, not "MEDIUM"', + '`variables.rules[].weight` must be Int, not "1"', + ], + ); + // Each branch is checked on its own, since TypeScript widens 'FAST' and 'MEDIUM' to string and merges the branches + assert.deepEqual( + check( + prepare(`return input.a ? {variables: {handle: 'a', mode: 'FAST'}} : {variables: {handle: 'a', mode: 'MEDIUM'}}`), + ), + ['`variables.mode` must be Mode, not "MEDIUM"'], + ); + }); + + test("rejects null and undefined where the variable needs a value", () => { + assert.deepEqual(check(prepare(`return input.a ? {variables: {handle: null}} : {variables: {handle: undefined}}`)), [ + "`variables.handle` can be null, but String! can't", + "`variables.handle` can be undefined, but String! has no default", + ]); + assert.deepEqual(check(prepare(`return {}`)), ["`variables` is missing, and Variables! has no default"]); + }); + + test("checks what is built, not what the return type declares", () => { + assert.deepEqual( + check({ + "index.js": ` + /** @returns {{variables: {handle: string}}} */ + export function cartValidationsGeneratePrepare() { + const variables = {handle: 'a', region: 'eu'} + return {variables} + }`, + }), + ["`variables.region` isn't declared by src/run.graphql"], + ); + }); + + test("sees through casts, @ts-ignore and helpers", () => { + assert.deepEqual( + check({ + "index.js": ` + export function cartValidationsGeneratePrepare(input) { + // @ts-ignore + if (input.a) return {variables: /** @type {{handle: string}} */ (JSON.parse(input.raw))} + return {variables: build()} + } + function build() { + return {handle: 2} + }`, + }), + ["warning: `variables` is typed any, so it can't be checked", "`variables.handle` must be String!, not 2"], + ); + }); + + test("warns about, rather than checks, variables changed or passed on after they are created", () => { + assert.deepEqual( + check( + prepare(` + const variables = {handle: 'a'} + variables.limit = 'many' + tweak(variables) + return {variables}`), + ), + [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ], + ); + assert.deepEqual( + check( + prepare(` + const base = {handle: 'a'} + base.handle = 1 + return {variables: {...base}}`), + ), + ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], + ); + // A variable set after the object is created isn't reported missing + assert.deepEqual( + check( + prepare(` + const variables = {} + variables.handle = 'a' + return {variables}`), + ), + ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], + ); + }); + + test("ignores what annotations and casts claim", () => { + assert.deepEqual( + check( + prepare(` + const raw = /** @type {{handle: string}} */ (JSON.parse(input.raw)) + const {limit} = /** @type {{limit: number}} */ (JSON.parse(input.raw)) + return {variables: {handle: raw.handle, limit}}`), + ), + ["warning: `variables.handle` is typed any, so it can't be checked", "warning: `variables.limit` is typed any, so it can't be checked"], + ); + assert.deepEqual( + check({ + "index.js": `export * from './prepare.js'`, + "prepare.ts": ` + type Variables = {handle: string} + export function cartValidationsGeneratePrepare(input: {name?: string; mode: string}): {variables: Variables} { + if (input.mode) return {variables: {handle: input.name!, mode: <'FAST'>input.mode}} + return {variables: new Map().get(input.mode)! as Variables} + }`, + }), + [ + "`variables.handle` can be undefined, but String! has no default", + "`variables.mode` must be Mode, not string", + "warning: `variables` is typed any, so it can't be checked", + ], + ); + }); + + test("follows re-exports", () => { + assert.deepEqual( + check({ + "index.js": `export * from './prepare.js'`, + "prepare.js": `export const cartValidationsGeneratePrepare = () => ({variables: {handle: 1}})`, + }), + ["`variables.handle` must be String!, not 1"], + ); + }); + + test("rejects a missing prepare export", () => { + assert.deepEqual(check({ "index.js": "export function other() {}" }), [ + "src/index.js has no `cartValidationsGeneratePrepare` function to check", + ]); + }); +}); From c7b71046c50e1284c7e3100dbbe3352c0a2383b5 Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Thu, 12 Nov 2026 09:18:41 -0600 Subject: [PATCH 2/8] Only fail the build on prepare variables that are definitely wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Values the check can't follow are warnings, and only provable mismatches fail the build: - `||` skips falsy values on its left, an `ID` takes integers, and a plain string for an enum is a warning - an export it can't follow, or a problem in the check itself, is a warning - missing exports and syntax errors are left to the bundler, and run query types the schema doesn't have to codegen - variables assigned by name after the object is created, `variables.x = …`, are checked - built-in scalars and enums use graphql-js's own coercion, so an Int over 32 bits fails --- check.js | 9 ++- prepare-variables.js | 133 +++++++++++++++++++++---------- tests/check.test.cjs | 6 ++ tests/prepare-variables.test.cjs | 81 +++++++++++++++---- 4 files changed, 168 insertions(+), 61 deletions(-) diff --git a/check.js b/check.js index 6d7c1c7..9ad1bd3 100644 --- a/check.js +++ b/check.js @@ -14,8 +14,15 @@ try { const { values } = parseArgs({ options: { function: { type: "string" } } }); if (!values.function) throw new Error("Usage: check.js --function "); const fn = { schema: "schema.graphql", targets: [], ...JSON.parse(fs.readFileSync(values.function, "utf8")) }; - const problems = prepareVariablesProblems(fn); const name = fn.handle ? `The function extension "${fn.handle}"` : "The function"; + let problems; + try { + problems = prepareVariablesProblems(fn); + } catch (error) { + // A problem in the check itself shouldn't stop the build + console.warn(`${name}'s prepare variables couldn't be checked: ${error.message}`); + problems = []; + } const list = (warning) => problems.filter((problem) => problem.warning === warning).map((problem) => problem.message); const warnings = list(true); diff --git a/prepare-variables.js b/prepare-variables.js index 761123c..46d8421 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -7,6 +7,7 @@ const ts = require("typescript"); const { GraphQLInputObjectType, GraphQLNonNull, + GraphQLScalarType, Kind, buildSchema, getNamedType, @@ -14,28 +15,13 @@ const { isInputObjectType, isListType, isNonNullType, + isScalarType, + isSpecifiedScalarType, parse, typeFromAST, } = require("graphql"); -// How each scalar's values appear in the variables, as Shopify CLI's codegen config types them. Other custom scalars, -// like `JSON`, accept anything. -const scalarKinds = { - Int: "number", - Float: "number", - String: "string", - Boolean: "boolean", - ID: "string", - Handle: "string", - Decimal: "string", - Date: "string", - DateTime: "string", - DateTimeWithoutTimezone: "string", - TimeWithoutTimezone: "string", - URL: "string", - Void: "null", -}; -const scalarKind = (named) => scalarKinds[named.name] ?? "unknown"; +const unknownScalar = new GraphQLScalarType({ name: "Unknown" }); /** The prepare targets whose run target has an input query. */ function preparePairs(targets) { @@ -111,6 +97,9 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return ts.createSourceFile(fileName, text.join(""), language); }; const program = ts.createProgram([entry], options, host); + // The bundler reports syntax errors better, after this check + const local = program.getSourceFiles().filter((file) => !file.isDeclarationFile && !program.isSourceFileFromExternalLibrary(file)); + if (local.some((file) => program.getSyntacticDiagnostics(file).length > 0)) return []; const checker = program.getTypeChecker(); const entryFile = program.getSourceFile(entry); const entrySymbol = entryFile && checker.getSymbolAtLocation(entryFile); @@ -146,6 +135,18 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // A reference that could change the value after it's created: assigning to it or a member, deleting or // incrementing a member, calling a method on it, or passing, aliasing or reassigning the binding itself. + // An assignment with a written-out key, `object.key = value` or `object["key"] = value`, to a reference to the object + const assignmentOf = (reference) => { + const access = reference.parent; + const assignment = access.parent; + const direct = (ts.isPropertyAccessExpression(access) || ts.isElementAccessExpression(access)) && access.expression === reference; + if (!direct || !ts.isBinaryExpression(assignment) || assignment.left !== access) return; + if (assignment.operatorToken.kind !== ts.SyntaxKind.EqualsToken) return; + const name = ts.isPropertyAccessExpression(access) ? access.name : access.argumentExpression; + const key = ts.isIdentifier(name) || ts.isStringLiteralLike(name) ? name.text : undefined; + return key === undefined ? undefined : { key, name, value: assignment.right }; + }; + const changes = (reference) => { let top = reference; while ( @@ -181,7 +182,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP fields: Object.fromEntries( (operation?.variableDefinitions ?? []).map((definition) => [ definition.variable.name.value, - { type: typeFromAST(schema, definition.type), defaultValue: definition.defaultValue }, + // A type the schema doesn't have is left to codegen, which reports it + { type: typeFromAST(schema, definition.type) ?? unknownScalar, defaultValue: definition.defaultValue }, ]), ), }); @@ -194,6 +196,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const checkedReferences = new Set(); const visiting = new Set(); const changed = new Map(); + const assigned = new Map(); // The variable an object literal property is written in, such as `variables` in `const variables = {a: 1}` const bindingOf = (property) => { @@ -204,34 +207,37 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return binding && ts.isVariableDeclaration(binding) && ts.isIdentifier(binding.name) ? binding : undefined; }; // A variable changed or passed on after it's created may not hold what it was created with, so it's not checked - const skipChanged = (declaration, at) => { - const changes = changed.get(declaration); + // An object assigned to by name is checked with its assignments, but only where it's created + const skipChanged = (declaration, at, node) => { + const assignments = assigned.get(declaration); + const changes = changed.get(declaration) ?? (assignments && assignments.object !== node ? assignments.nodes : undefined); for (const node of changes ?? []) { report(node, `${subject(at)} is changed or passed on after it's created, so it can't be checked`, true); } return Boolean(changes); }; - const scalarMatches = (type, named) => { - if (isEnumType(named)) return type.isStringLiteral() && named.getValue(type.value) !== undefined; - switch (scalarKind(named)) { - case "string": - return Boolean(type.flags & ts.TypeFlags.StringLike); - case "number": - return ( - Boolean(type.flags & ts.TypeFlags.NumberLike) && - !(named.name === "Int" && type.isNumberLiteral() && !Number.isInteger(type.value)) - ); - case "boolean": - return Boolean(type.flags & ts.TypeFlags.BooleanLike); - default: + // Whether a built-in scalar or an enum accepts a value, by GraphQL's own coercion: "yes", "no", or "maybe" when it + // depends on which of its type's values it is. A type like `number` is tried with one of its values. + const scalarMatch = (type, named) => { + const accepts = (value) => { + try { + return named.parseValue(value) !== undefined; + } catch { return false; - } + } + }; + if (type.isStringLiteral() || type.isNumberLiteral()) return accepts(type.value) ? "yes" : "no"; + if (type.flags & ts.TypeFlags.BooleanLiteral) return accepts(type === checker.getTrueType()) ? "yes" : "no"; + if (type.flags & ts.TypeFlags.StringLike) return isEnumType(named) ? "maybe" : accepts("") ? "yes" : "no"; + if (type.flags & ts.TypeFlags.NumberLike) return accepts(0) ? "yes" : "no"; + return "no"; }; const typed = (type, expected, at, omittable, node) => { const named = getNamedType(expected); - if (!isEnumType(named) && !isInputObjectType(named) && scalarKind(named) === "unknown") return; + // The server decides what a custom scalar, such as JSON or Decimal, accepts + if (isScalarType(named) && !isSpecifiedScalarType(named)) return; if (unchecked(type)) return report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); if (type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)) { @@ -254,9 +260,22 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } const properties = checker.getPropertiesOfType(type); const bindings = new Set(properties.map(bindingOf).filter(Boolean)); - if ([...bindings].map((binding) => skipChanged(binding, at)).some(Boolean)) return; + // A spread object adds what it holds when it's spread, which may not be what it was created with + for (const spread of ts.isObjectLiteralExpression(node) ? node.properties.filter(ts.isSpreadAssignment) : []) { + const expression = unwrap(spread.expression); + const declaration = ts.isIdentifier(expression) && symbolOf(expression)?.valueDeclaration; + if (declaration && ts.isVariableDeclaration(declaration)) { + followed.set(declaration, at); + bindings.add(declaration); + } + } + if ([...bindings].map((binding) => skipChanged(binding, at, node)).some(Boolean)) return; const fields = inner.getFields(); + const assignments = [...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []; + const assignedKeys = new Set(assignments.map((assignment) => assignment.key)); for (const property of properties) { + // A property assigned after the object is created is checked with its assignments + if (assignedKeys.has(property.name)) continue; const field = fields[property.name]; const key = join(at, property.name); // A property written in an object literal is followed to its value, keeping literal types (like an enum @@ -277,15 +296,24 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP typed(checker.getTypeOfSymbol(property), field.type, key, field.defaultValue !== undefined, node); } } + for (const { key, name, value: assignedValue } of assignments) { + const field = fields[key]; + if (!field) report(name, `${subject(join(at, key))} isn't ${owner(inner)}`); + else value(assignedValue, field.type, join(at, key), field.defaultValue !== undefined); + } for (const [key, field] of Object.entries(fields)) { - if (isNonNullType(field.type) && field.defaultValue === undefined && !checker.getPropertyOfType(type, key)) { + const set = checker.getPropertyOfType(type, key) || assignedKeys.has(key); + if (isNonNullType(field.type) && field.defaultValue === undefined && !set) { report(node, `${subject(join(at, key))} is missing, and ${field.type} has no default`); } } return; } - if (isInputObjectType(inner) || !scalarMatches(type, inner)) { + const match = isInputObjectType(inner) ? "no" : scalarMatch(type, inner); + if (match === "no") { report(node, `${subject(at)} must be ${expected}, not ${checker.typeToString(type)}`); + } else if (match === "maybe") { + report(node, `${subject(at)} is ${checker.typeToString(type)}, which ${inner} may not accept, so it can't be checked`, true); } }; @@ -313,7 +341,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer) { followed.set(declaration, at); checkedReferences.add(expression); - if (skipChanged(declaration, at)) return; + if (skipChanged(declaration, at, unwrap(declaration.initializer))) return; return value(declaration.initializer, expected, at, omittable); } } else if (ts.isConditionalExpression(expression)) { @@ -324,8 +352,14 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP (expression.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken || expression.operatorToken.kind === ts.SyntaxKind.BarBarToken) ) { - const left = checker.getNonNullableType(checker.getTypeAtLocation(expression.left)); - typed(left, expected, at, omittable, expression.left); + // `??` uses the left side unless it's null or undefined, and `||` only when it's truthy + const or = expression.operatorToken.kind === ts.SyntaxKind.BarBarToken; + const left = checker.getTypeAtLocation(expression.left); + for (const member of left.isUnion() ? left.types : [left]) { + if (member.flags & (ts.TypeFlags.Null | ts.TypeFlags.Undefined | ts.TypeFlags.Void)) continue; + if (or && (member === checker.getFalseType() || ((member.isStringLiteral() || member.isNumberLiteral()) && !member.value))) continue; + typed(member, expected, at, omittable, expression.left); + } return value(expression.right, expected, at, omittable); } else if (ts.isCallExpression(expression)) { const fn = functionOf(checker.getResolvedSignature(expression)?.declaration); @@ -349,7 +383,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const resolved = exported && (exported.flags & ts.SymbolFlags.Alias ? checker.getAliasedSymbol(exported) : exported); const fn = functionOf(resolved?.valueDeclaration); if (!fn) { - problems.set(`${path.relative(directory, entry)} has no \`${name}\` function to check`, false); + // The bundler reports a missing export + if (resolved?.valueDeclaration) { + report(resolved.valueDeclaration, `\`${name}\` isn't a function it can follow, so it can't be checked`, true); + } continue; } // The first pass finds the variables the result is built from, and the second checks it, skipping the ones @@ -373,9 +410,17 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ts.forEachChild(node, visit); }; visit(declaration.getSourceFile()); - if (nodes.length > 0) changed.set(declaration, nodes); + if (nodes.length === 0) continue; + const object = unwrap(declaration.initializer); + const assignments = nodes.map(assignmentOf); + if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean)) { + assigned.set(declaration, { object, nodes, assignments }); + } else { + changed.set(declaration, nodes); + } } visiting.clear(); + followed.clear(); returns(fn, new GraphQLNonNull(result), "", false); } diff --git a/tests/check.test.cjs b/tests/check.test.cjs index b22ed9d..6a1952b 100644 --- a/tests/check.test.cjs +++ b/tests/check.test.cjs @@ -66,6 +66,12 @@ test("warns, without failing, about variables it can't check", (t) => { ); }); +test("warns, without failing, when the check itself fails", (t) => { + const result = runCheck(t, { fn: { schema: "missing.graphql" } }); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stderr, /The function extension "prepare-test"'s prepare variables couldn't be checked: ENOENT/); +}); + test("passes a function without prepare targets", (t) => { const result = runCheck(t, { fn: { entry: undefined, targets: [targets[1]] } }); assert.equal(result.status, 0, result.stderr); diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index c8da88b..7d2b761 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -7,11 +7,13 @@ const { prepareVariablesProblems } = require("../prepare-variables.js"); const schema = ` scalar JSON + scalar Decimal enum Mode { FAST SLOW } input Rule { handle: String!, weight: Int } - type Query { shop: String } + type Query { shop: String, id: ID } `; -const runQuery = "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON) { shop }"; +const runQuery = + "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON, $id: ID, $price: Decimal) { shop }"; const paired = [ { target: "cart.validations.generate.prepare", export: "cartValidationsGeneratePrepare" }, @@ -151,16 +153,6 @@ describe("prepareVariablesProblems", () => { ), ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], ); - // A variable set after the object is created isn't reported missing - assert.deepEqual( - check( - prepare(` - const variables = {} - variables.handle = 'a' - return {variables}`), - ), - ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], - ); }); test("ignores what annotations and casts claim", () => { @@ -185,7 +177,7 @@ describe("prepareVariablesProblems", () => { }), [ "`variables.handle` can be undefined, but String! has no default", - "`variables.mode` must be Mode, not string", + "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", "warning: `variables` is typed any, so it can't be checked", ], ); @@ -201,9 +193,66 @@ describe("prepareVariablesProblems", () => { ); }); - test("rejects a missing prepare export", () => { - assert.deepEqual(check({ "index.js": "export function other() {}" }), [ - "src/index.js has no `cartValidationsGeneratePrepare` function to check", + test("checks variables assigned by name after the object is created", () => { + const assigned = (lines) => check(prepare(`const variables = ${lines}\nreturn {variables}`)); + assert.deepEqual(assigned(`{}\nvariables.handle = 'a'\nvariables['limit'] = 5`), []); + assert.deepEqual(assigned(`{handle: null}\nvariables.handle = 'a'`), []); + assert.deepEqual(assigned(`{handle: 'a'}\nif (input.a) variables.region = 'eu'`), [ + "`variables.region` isn't declared by src/run.graphql", + ]); + assert.deepEqual(assigned(`{handle: 'a'}\nvariables.limit = 'many'`), ['`variables.limit` must be Int, not "many"']); + assert.deepEqual(assigned(`{}\nvariables.limit = 5`), ["`variables.handle` is missing, and String! has no default"]); + // Keys that aren't written out, and objects spread after they're assigned to, still can't be checked + assert.deepEqual(assigned(`{handle: 'a'}\nvariables[input.key] = 5`), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ]); + assert.deepEqual(check(prepare(`const base = {}\nbase.handle = 'a'\nreturn {variables: {...base}}`)), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", ]); }); + + test("only fails on values that are definitely wrong", () => { + assert.deepEqual( + check({ + "index.js": ` + const MODES = ['FAST', 'SLOW'] + /** @param {{a: boolean, m: string}} input */ + export function cartValidationsGeneratePrepare(input) { + return {variables: {handle: (input.a && 'x') || 'y', id: 5, mode: input.a ? input.m : MODES[0], price: 1.5}} + }`, + }), + [ + // One for `input.m`, and one for `MODES[0]` + "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", + "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", + ], + ); + assert.deepEqual(check(prepare(`return {variables: {handle: (input.a && false) || true, id: 1.5}}`)), [ + "warning: `variables.handle` is typed any, so it can't be checked", + "`variables.handle` must be String!, not true", + "`variables.id` must be ID, not 1.5", + ]); + }); + + test("warns about an export it can't follow, and leaves missing exports and syntax errors to the bundler", () => { + assert.deepEqual( + check({ + "index.js": ` + const wrap = (fn) => fn + export const cartValidationsGeneratePrepare = wrap(() => ({variables: {handle: 'a'}}))`, + }), + ["warning: `cartValidationsGeneratePrepare` isn't a function it can follow, so it can't be checked"], + ); + assert.deepEqual( + check({ + "index.js": ` + const handlers = {cartValidationsGeneratePrepare() { return {variables: {handle: 'a'}} }} + export const cartValidationsGeneratePrepare = handlers.cartValidationsGeneratePrepare`, + }), + ["warning: `cartValidationsGeneratePrepare` isn't a function it can follow, so it can't be checked"], + ); + assert.deepEqual(check({ "index.js": "export function other() {}" }), []); + // A function missing its closing brace + assert.deepEqual(check({ "index.js": "export function cartValidationsGeneratePrepare() {\n return {variables: {handle: 1}}" }), []); + }); }); From 4ff38bda8030625bbf626e1351a73e8be9540010 Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Thu, 12 Nov 2026 14:42:51 -0600 Subject: [PATCH 3/8] Check more ways of building prepare variables - trust non-null assertions, as Rust trusts `unwrap`, and treat an array element as possibly undefined - check objects combined with Object.assign, and the last value of a key written twice - warn about spreading an object that can have any keys, and about a required variable only set on some paths - reject an async prepare target, whose result is a promise - apply oneOf rules - follow the arguments a helper is called with - report paths with forward slashes on Windows --- prepare-variables.js | 68 ++++++++++++++++++------ tests/prepare-variables.test.cjs | 90 +++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/prepare-variables.js b/prepare-variables.js index 46d8421..7fb4077 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -42,6 +42,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return [{ message: "The function has no entry file, such as src/index.js, to check its prepare targets in", warning: false }]; } const entry = path.resolve(directory, entryPath); + // Paths in messages use forward slashes, as Shopify CLI's do, on Windows too + const relative = (file) => path.relative(directory, file).split(path.sep).join("/"); const schema = buildSchema(fs.readFileSync(path.resolve(directory, schemaPath), "utf8")); const options = { @@ -54,11 +56,13 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP target: ts.ScriptTarget.ES2022, module: ts.ModuleKind.ESNext, moduleResolution: ts.ModuleResolutionKind.Bundler, + // An element of an array, like `list[0]`, can be undefined + noUncheckedIndexedAccess: true, }; const host = ts.createCompilerHost(options, true); // Annotations and casts are claims nothing checks against the value (`@ts-ignore` silences the compiler), so they're - // blanked out of the function's own files, leaving types to come from the values. Parameter types are kept. Blanking - // keeps positions, so errors point at the original source. + // blanked out of the function's own files, leaving types to come from the values. Parameter types and non-null + // assertions (`!`, like Rust's `unwrap`) are kept. Blanking keeps positions, so errors point at the original source. const { getSourceFile } = host; host.getSourceFile = (fileName, language, ...rest) => { const file = getSourceFile(fileName, language, ...rest); @@ -73,7 +77,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ts.isJSDocTypeTag(tag) || ts.isJSDocReturnTag(tag) || ts.isJSDocSatisfiesTag(tag) || ts.isJSDocOverloadTag(tag); if (typing && !ts.isParameter(tag.parent.parent)) blank(tag.pos, tag.end); } - if (ts.isAsExpression(node) || ts.isSatisfiesExpression(node) || ts.isNonNullExpression(node)) { + if (ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { blank(node.expression.end, node.end); } else if (ts.isTypeAssertionExpression(node)) { blank(node.type.pos - 1, node.expression.pos); @@ -110,7 +114,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (muted) return; const file = node.getSourceFile(); const { line, character } = file.getLineAndCharacterOfPosition(node.getStart()); - problems.set(`${path.relative(directory, file.fileName)}:${line + 1}:${character + 1} ${message}`, warning); + problems.set(`${relative(file.fileName)}:${line + 1}:${character + 1} ${message}`, warning); }; const join = (base, key) => (base ? `${base}.${key}` : key); const subject = (at) => (at ? `\`${at}\`` : "The result"); @@ -144,7 +148,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (assignment.operatorToken.kind !== ts.SyntaxKind.EqualsToken) return; const name = ts.isPropertyAccessExpression(access) ? access.name : access.argumentExpression; const key = ts.isIdentifier(name) || ts.isStringLiteralLike(name) ? name.text : undefined; - return key === undefined ? undefined : { key, name, value: assignment.right }; + return key === undefined ? undefined : { key, name, value: assignment.right, node: assignment }; }; const changes = (reference) => { @@ -173,7 +177,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP for (const { name, query } of pairs) { const queryPath = path.resolve(directory, query); - const queryName = path.relative(directory, queryPath); + const queryName = relative(queryPath); const operation = parse(fs.readFileSync(queryPath, "utf8")).definitions.find( (definition) => definition.kind === Kind.OPERATION_DEFINITION, ); @@ -197,6 +201,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const visiting = new Set(); const changed = new Map(); const assigned = new Map(); + const passed = new Map(); // The variable an object literal property is written in, such as `variables` in `const variables = {a: 1}` const bindingOf = (property) => { @@ -254,7 +259,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP (checker.isArrayType(type) || checker.isTupleType(type)) && checker.getIndexTypeOfType(type, ts.IndexKind.Number); return element ? typed(element, inner.ofType, `${at}[]`, false, node) : typed(type, inner.ofType, at, false, node); } - if (isInputObjectType(inner) && type.flags & ts.TypeFlags.Object && !checker.isArrayType(type)) { + // An intersection, such as what Object.assign returns, is checked like an object + if (isInputObjectType(inner) && (type.flags & ts.TypeFlags.Object || type.isIntersection()) && !checker.isArrayType(type)) { if (checker.getIndexInfosOfType(type).length > 0) { return report(node, `${subject(at)} can have any keys, so it can't be checked`, true); } @@ -268,9 +274,15 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP followed.set(declaration, at); bindings.add(declaration); } + const spreadType = checker.getTypeAtLocation(spread.expression); + if (unchecked(spreadType) || checker.getIndexInfosOfType(spreadType).length > 0) { + return report(spread, `${subject(at)} spreads an object that can have any keys, so it can't be checked`, true); + } } if ([...bindings].map((binding) => skipChanged(binding, at, node)).some(Boolean)) return; const fields = inner.getFields(); + // A oneOf input object takes exactly one of its fields, which can't be null + const fieldType = (field) => (inner.isOneOf ? new GraphQLNonNull(field.type) : field.type); const assignments = [...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []; const assignedKeys = new Set(assignments.map((assignment) => assignment.key)); for (const property of properties) { @@ -279,8 +291,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const field = fields[property.name]; const key = join(at, property.name); // A property written in an object literal is followed to its value, keeping literal types (like an enum - // value) that the object's type widens - const declaration = property.valueDeclaration; + // value) that the object's type widens. A key written twice has its last value. + const declaration = property.declarations?.at(-1) ?? property.valueDeclaration; const assignment = declaration && (ts.isPropertyAssignment(declaration) || ts.isShorthandPropertyAssignment(declaration)) ? declaration @@ -291,21 +303,28 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const binding = bindingOf(property); if (binding) followed.set(binding, at); const initializer = ts.isPropertyAssignment(assignment) ? assignment.initializer : assignment.name; - value(initializer, field.type, key, field.defaultValue !== undefined); + value(initializer, fieldType(field), key, field.defaultValue !== undefined); } else { - typed(checker.getTypeOfSymbol(property), field.type, key, field.defaultValue !== undefined, node); + typed(checker.getTypeOfSymbol(property), fieldType(field), key, field.defaultValue !== undefined, node); } } for (const { key, name, value: assignedValue } of assignments) { const field = fields[key]; if (!field) report(name, `${subject(join(at, key))} isn't ${owner(inner)}`); - else value(assignedValue, field.type, join(at, key), field.defaultValue !== undefined); + else value(assignedValue, fieldType(field), join(at, key), field.defaultValue !== undefined); + } + const keys = new Set([...properties.map((property) => property.name), ...assignedKeys]); + if (inner.isOneOf && keys.size !== 1) { + const optional = properties.some((property) => property.flags & ts.SymbolFlags.Optional); + if (optional && keys.size > 1) report(node, `${subject(at)} may set more than one field of ${inner}, so it can't be checked`, true); + else report(node, `${subject(at)} must set exactly one field of ${inner}`); } for (const [key, field] of Object.entries(fields)) { - const set = checker.getPropertyOfType(type, key) || assignedKeys.has(key); - if (isNonNullType(field.type) && field.defaultValue === undefined && !set) { - report(node, `${subject(join(at, key))} is missing, and ${field.type} has no default`); - } + if (!isNonNullType(field.type) || field.defaultValue !== undefined || checker.getPropertyOfType(type, key)) continue; + const setBy = assignments.filter((assignment) => assignment.key === key); + if (setBy.some((assignment) => assignment.always)) continue; + if (setBy.length > 0) report(setBy[0].name, `${subject(join(at, key))} is only set on some paths, so it can't be checked`, true); + else report(node, `${subject(join(at, key))} is missing, and ${field.type} has no default`); } return; } @@ -338,6 +357,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (ts.isIdentifier(expression)) { const declaration = symbolOf(expression)?.valueDeclaration; + // A helper's parameter holds what the call passes for it + if (declaration && passed.has(declaration)) return value(passed.get(declaration), expected, at, omittable); if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer) { followed.set(declaration, at); checkedReferences.add(expression); @@ -365,7 +386,13 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const fn = functionOf(checker.getResolvedSignature(expression)?.declaration); if (fn && !visiting.has(fn)) { visiting.add(fn); + const spread = expression.arguments.some(ts.isSpreadElement); + fn.parameters.forEach((parameter, index) => { + const argument = expression.arguments[index] ?? parameter.initializer; + if (!spread && argument && ts.isIdentifier(parameter.name) && !parameter.dotDotDotToken) passed.set(parameter, argument); + }); returns(fn, expected, at, omittable); + fn.parameters.forEach((parameter) => passed.delete(parameter)); visiting.delete(fn); return; } @@ -382,6 +409,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const exported = entrySymbol && checker.getExportsOfModule(entrySymbol).find((symbol) => symbol.name === name); const resolved = exported && (exported.flags & ts.SymbolFlags.Alias ? checker.getAliasedSymbol(exported) : exported); const fn = functionOf(resolved?.valueDeclaration); + if (fn && fn.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.AsyncKeyword)) { + report(fn, `\`${name}\` is async, so it returns a promise instead of its result`); + continue; + } if (!fn) { // The bundler reports a missing export if (resolved?.valueDeclaration) { @@ -414,6 +445,11 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const object = unwrap(declaration.initializer); const assignments = nodes.map(assignmentOf); if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean)) { + // An assignment in the block the object is created in always runs, unlike one in an `if`, loop or callback + const block = declaration.parent.parent.parent; + for (const assignment of assignments) { + assignment.always = ts.isExpressionStatement(assignment.node.parent) && assignment.node.parent.parent === block; + } assigned.set(declaration, { object, nodes, assignments }); } else { changed.set(declaration, nodes); diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index 7d2b761..c3f20ff 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -9,11 +9,12 @@ const schema = ` scalar JSON scalar Decimal enum Mode { FAST SLOW } + input Selector @oneOf { id: ID, handle: String } input Rule { handle: String!, weight: Int } type Query { shop: String, id: ID } `; const runQuery = - "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON, $id: ID, $price: Decimal) { shop }"; + "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON, $id: ID, $price: Decimal, $selector: Selector) { shop }"; const paired = [ { target: "cart.validations.generate.prepare", export: "cartValidationsGeneratePrepare" }, @@ -176,13 +177,98 @@ describe("prepareVariablesProblems", () => { }`, }), [ - "`variables.handle` can be undefined, but String! has no default", "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", "warning: `variables` is typed any, so it can't be checked", ], ); }); + test("follows the arguments a helper is called with", () => { + assert.deepEqual( + check({ + "index.js": `/** @param {{n: number}} input */ + export function cartValidationsGeneratePrepare(input) { return {variables: build(input.n)} } + /** @param {string} handle */ + function build(handle) { return {handle} }`, + }), + ["`variables.handle` must be String!, not number"], + ); + assert.deepEqual( + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": "function id(x: T): T { return x }\nexport function cartValidationsGeneratePrepare() { return {variables: id({handle: 'a'})} }", + }), + [], + ); + }); + + test("warns about a required variable that's only set on some paths", () => { + const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); + assert.deepEqual(assigned(`variables.handle = 'a'`), []); + for (const lines of [ + `if (input.a) variables.handle = 'a'`, + `for (const x of input.list) variables.handle = 'a'`, + `input.list.forEach(() => { variables.handle = 'a' })`, + ]) { + assert.deepEqual(assigned(lines), ["warning: `variables.handle` is only set on some paths, so it can't be checked"], lines); + } + }); + + test("applies oneOf input object rules", () => { + const selector = (value) => check(prepare(`return {variables: {handle: 'a', selector: ${value}}}`)); + assert.deepEqual(selector(`{id: '1'}`), []); + assert.deepEqual(selector(`{id: '1', handle: 'x'}`), ["`variables.selector` must set exactly one field of Selector"]); + assert.deepEqual(selector(`{}`), ["`variables.selector` must set exactly one field of Selector"]); + assert.deepEqual(selector(`{id: null}`), ["`variables.selector.id` can be null, but ID! can't"]); + }); + + test("rejects an async prepare target", () => { + assert.deepEqual(check({ "index.js": "export async function cartValidationsGeneratePrepare() { return {variables: {handle: 'a'}} }" }), [ + "`cartValidationsGeneratePrepare` is async, so it returns a promise instead of its result", + ]); + }); + + test("warns about spreading an object that can have any keys", () => { + assert.deepEqual( + check({ + "index.js": `/** @param {{json: Record}} input */ + export function cartValidationsGeneratePrepare(input) { return {variables: {handle: 'a', ...input.json}} }`, + }), + ["warning: `variables` spreads an object that can have any keys, so it can't be checked"], + ); + }); + + test("checks the last value of a key written twice", () => { + assert.deepEqual(check(prepare(`return {variables: {handle: 'a', handle: 5}}`)), ["`variables.handle` must be String!, not 5"]); + }); + + test("checks objects combined with Object.assign", () => { + assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {limit: 2})}`)), []); + assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {region: 'eu'})}`)), [ + "`variables.region` isn't declared by src/run.graphql", + ]); + }); + + test("treats an array element as possibly undefined", () => { + assert.deepEqual( + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list[0]}} }", + }), + ["`variables.handle` can be undefined, but String! has no default"], + ); + }); + + test("trusts non-null assertions", () => { + assert.deepEqual( + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list.find((x) => x)!}} }", + }), + [], + ); + }); + test("follows re-exports", () => { assert.deepEqual( check({ From 39e01a0092d4d045953f0033def61fa78d1dc56c Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Thu, 12 Nov 2026 15:29:16 -0600 Subject: [PATCH 4/8] Check custom scalars, the whole prepare result and syntax errors - a custom scalar is checked as the type codegen gives it, from Shopify CLI's defaults and the function's own config, so a `Handle` is a string and `Void` only takes null. Null and lists are checked for every custom scalar. - the prepare result's other fields come from the schema, with only `variables` replaced by the run query's variables, as the Rust override does - a generic helper is checked by what its call returns - only assignments that can change what's returned are checked - a syntax error that stops the check is a warning --- check.js | 12 ++++- prepare-variables.js | 91 +++++++++++++++++++++++++++----- tests/check.test.cjs | 13 ++++- tests/prepare-variables.test.cjs | 80 +++++++++++++++++++++++++--- 4 files changed, 172 insertions(+), 24 deletions(-) diff --git a/check.js b/check.js index 9ad1bd3..b83aa61 100644 --- a/check.js +++ b/check.js @@ -7,6 +7,7 @@ // the JavaScript function the CLI bundles. It exits non-zero, failing the build, when a prepare target returns // variables its run query doesn't accept, and warns about variables it can't check. const fs = require("node:fs"); +const path = require("node:path"); const { parseArgs } = require("node:util"); const { prepareVariablesProblems } = require("./prepare-variables.js"); @@ -17,7 +18,7 @@ try { const name = fn.handle ? `The function extension "${fn.handle}"` : "The function"; let problems; try { - problems = prepareVariablesProblems(fn); + problems = prepareVariablesProblems({ ...fn, scalars: codegenScalars(fn.directory) }); } catch (error) { // A problem in the check itself shouldn't stop the build console.warn(`${name}'s prepare variables couldn't be checked: ${error.message}`); @@ -37,3 +38,12 @@ try { console.error(error.message); process.exitCode = 1; } + +// The scalars in the function's codegen config, which Shopify CLI merges with its own +function codegenScalars(directory) { + try { + return JSON.parse(fs.readFileSync(path.join(directory, "package.json"), "utf8")).codegen?.config?.scalars ?? {}; + } catch { + return {}; + } +} diff --git a/prepare-variables.js b/prepare-variables.js index 7fb4077..db33fed 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -6,8 +6,11 @@ const path = require("node:path"); const ts = require("typescript"); const { GraphQLInputObjectType, + GraphQLBoolean, + GraphQLFloat, GraphQLNonNull, GraphQLScalarType, + GraphQLString, Kind, buildSchema, getNamedType, @@ -23,6 +26,20 @@ const { const unknownScalar = new GraphQLScalarType({ name: "Unknown" }); +// The TypeScript type Shopify CLI's codegen config gives each custom scalar, which a function's config can change +const codegenScalars = { + Date: "string", + DateTime: "string", + DateTimeWithoutTimezone: "string", + Decimal: "string", + Handle: "string", + JSON: "unknown", + TimeWithoutTimezone: "string", + URL: "string", + Void: "null", +}; +const scalarsByType = { string: GraphQLString, number: GraphQLFloat, boolean: GraphQLBoolean }; + /** The prepare targets whose run target has an input query. */ function preparePairs(targets) { return targets.flatMap(({ target, export: name }) => { @@ -33,9 +50,11 @@ function preparePairs(targets) { /** * Returns `{message, warning}` for each problem, where `message` is `file:line:column message`, relative to `directory`. - * Values that can't be checked are warnings, and mismatches are errors. + * Values that can't be checked are warnings, and mismatches are errors. `scalars` is the function's codegen config. */ -function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets }) { +function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets, scalars = {} }) { + const scalarTypes = { ...codegenScalars }; + for (const [name, type] of Object.entries(scalars)) scalarTypes[name] = typeof type === "object" ? type?.input : type; const pairs = preparePairs(targets); if (pairs.length === 0) return []; if (!entryPath) { @@ -101,9 +120,17 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return ts.createSourceFile(fileName, text.join(""), language); }; const program = ts.createProgram([entry], options, host); - // The bundler reports syntax errors better, after this check + // A syntax error stops the check. The bundler reports it too, unless the bundle leaves the file out, such as one + // only imported with `import type`. const local = program.getSourceFiles().filter((file) => !file.isDeclarationFile && !program.isSourceFileFromExternalLibrary(file)); - if (local.some((file) => program.getSyntacticDiagnostics(file).length > 0)) return []; + const syntaxErrors = local.flatMap((file) => program.getSyntacticDiagnostics(file).slice(0, 1)); + if (syntaxErrors.length > 0) { + return syntaxErrors.map(({ file, start, messageText }) => { + const { line, character } = file.getLineAndCharacterOfPosition(start); + const error = ts.flattenDiagnosticMessageText(messageText, " "); + return { message: `${relative(file.fileName)}:${line + 1}:${character + 1} can't be checked because of a syntax error: ${error}`, warning: true }; + }); + } const checker = program.getTypeChecker(); const entryFile = program.getSourceFile(entry); const entrySymbol = entryFile && checker.getSymbolAtLocation(entryFile); @@ -118,7 +145,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP }; const join = (base, key) => (base ? `${base}.${key}` : key); const subject = (at) => (at ? `\`${at}\`` : "The result"); - const unchecked = (type) => Boolean(type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)); + // A type parameter, such as `T` in a generic helper, can be anything + const generic = (type) => + Boolean(type.flags & ts.TypeFlags.InstantiableNonPrimitive) || (type.isIntersection() && type.types.some(generic)); + const unchecked = (type) => Boolean(type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) || generic(type); const unwrap = (node) => (ts.isParenthesizedExpression(node) ? unwrap(node.expression) : node); @@ -140,6 +170,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // A reference that could change the value after it's created: assigning to it or a member, deleting or // incrementing a member, calling a method on it, or passing, aliasing or reassigning the binding itself. // An assignment with a written-out key, `object.key = value` or `object["key"] = value`, to a reference to the object + const functionAround = (node) => ts.findAncestor(node.parent, ts.isFunctionLike); + const assignmentOf = (reference) => { const access = reference.parent; const assignment = access.parent; @@ -175,7 +207,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ); }; - for (const { name, query } of pairs) { + for (const { target, name, query } of pairs) { const queryPath = path.resolve(directory, query); const queryName = relative(queryPath); const operation = parse(fs.readFileSync(queryPath, "utf8")).definitions.find( @@ -191,9 +223,16 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ]), ), }); + // The prepare target's result type, from its mutation field such as `cartValidationsGeneratePrepare(result: …)`, with + // `variables` typed as the run query's variables, as the Rust override does. Without one, it only has `variables`. + const field = schema.getMutationType()?.getFields()[target.replace(/[.-](\w)/g, (_, letter) => letter.toUpperCase())]; + const schemaResult = field && getNamedType(field.args.find((argument) => argument.name === "result")?.type); const result = new GraphQLInputObjectType({ - name: "PrepareResult", - fields: { variables: { type: new GraphQLNonNull(variables) } }, + name: isInputObjectType(schemaResult) ? schemaResult.name : "PrepareResult", + fields: { + ...(isInputObjectType(schemaResult) ? schemaResult.toConfig().fields : {}), + variables: { type: new GraphQLNonNull(variables) }, + }, }); const owner = (type) => (type === variables ? `declared by ${queryName}` : `a field of ${type.name}`); const followed = new Map(); @@ -202,6 +241,15 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const changed = new Map(); const assigned = new Map(); const passed = new Map(); + const returning = []; + // An assignment after the return being checked doesn't change what it returns, unless a loop around both runs it first + const beforeReturn = ({ node }) => { + const fn = functionAround(node); + const statement = returning.findLast((candidate) => functionAround(candidate) === fn); + if (!statement || node.pos < statement.end) return true; + const loop = ts.findAncestor(statement, (ancestor) => (ancestor === fn ? "quit" : ts.isIterationStatement(ancestor, false))); + return Boolean(loop && node.end <= loop.end); + }; // The variable an object literal property is written in, such as `variables` in `const variables = {a: 1}` const bindingOf = (property) => { @@ -241,9 +289,15 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const typed = (type, expected, at, omittable, node) => { const named = getNamedType(expected); - // The server decides what a custom scalar, such as JSON or Decimal, accepts - if (isScalarType(named) && !isSpecifiedScalarType(named)) return; - if (unchecked(type)) return report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); + // A custom scalar is checked as the TypeScript type codegen gives it, so a `Handle` is a string. The server + // decides what other ones, such as JSON, accept, but not whether they can be null or a list. + const custom = isScalarType(named) && !isSpecifiedScalarType(named); + const codegenType = custom ? scalarTypes[named.name] : undefined; + const loose = custom && codegenType !== "null" && !scalarsByType[codegenType]; + if (unchecked(type)) { + if (!loose) report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); + return; + } if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); if (type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)) { if (isNonNullType(expected) && !omittable) report(node, `${subject(at)} can be undefined, but ${expected} has no default`); @@ -283,7 +337,9 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const fields = inner.getFields(); // A oneOf input object takes exactly one of its fields, which can't be null const fieldType = (field) => (inner.isOneOf ? new GraphQLNonNull(field.type) : field.type); - const assignments = [...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []; + const assignments = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( + beforeReturn, + ); const assignedKeys = new Set(assignments.map((assignment) => assignment.key)); for (const property of properties) { // A property assigned after the object is created is checked with its assignments @@ -328,7 +384,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } return; } - const match = isInputObjectType(inner) ? "no" : scalarMatch(type, inner); + if (loose) return; + const match = isInputObjectType(inner) || codegenType === "null" ? "no" : scalarMatch(type, scalarsByType[codegenType] ?? inner); if (match === "no") { report(node, `${subject(at)} must be ${expected}, not ${checker.typeToString(type)}`); } else if (match === "maybe") { @@ -342,8 +399,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (!ts.isBlock(body)) return value(body, expected, at, omittable); const visit = (node) => { if (ts.isReturnStatement(node)) { + returning.push(node); if (node.expression) value(node.expression, expected, at, omittable); else typed(checker.getUndefinedType(), expected, at, omittable, node); + returning.pop(); } else if (!ts.isFunctionLike(node)) { ts.forEachChild(node, visit); } @@ -384,6 +443,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return value(expression.right, expected, at, omittable); } else if (ts.isCallExpression(expression)) { const fn = functionOf(checker.getResolvedSignature(expression)?.declaration); + // A generic helper is checked by what its call returns, where its type parameters are known + if (fn?.typeParameters) return typed(checker.getTypeAtLocation(expression), expected, at, omittable, expression); if (fn && !visiting.has(fn)) { visiting.add(fn); const spread = expression.arguments.some(ts.isSpreadElement); @@ -444,7 +505,9 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (nodes.length === 0) continue; const object = unwrap(declaration.initializer); const assignments = nodes.map(assignmentOf); - if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean)) { + // An assignment in a nested function, such as a callback, may run at any time or not at all + const nested = nodes.some((node) => functionAround(node) !== functionAround(declaration)); + if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean) && !nested) { // An assignment in the block the object is created in always runs, unlike one in an `if`, loop or callback const block = declaration.parent.parent.parent; for (const assignment of assignments) { diff --git a/tests/check.test.cjs b/tests/check.test.cjs index 6a1952b..7ee46c1 100644 --- a/tests/check.test.cjs +++ b/tests/check.test.cjs @@ -9,6 +9,7 @@ const check = path.join(__dirname, "..", "check.js"); const schema = ` scalar JSON +scalar Decimal input LineSelector @oneOf { id: ID, handle: String } type CartLine { id: ID! } type Query { id: ID!, lines(selector: LineSelector!, first: Int): [CartLine!]! } @@ -20,12 +21,13 @@ const targets = [ ]; // Runs check.js for a function whose prepare target returns `variables`, as Shopify CLI does. -function runCheck(t, { variables = "{}", fn = {} } = {}) { +function runCheck(t, { variables = "{}", fn = {}, packageJson } = {}) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "function-")); t.after(() => fs.rmSync(dir, { recursive: true, force: true })); fs.mkdirSync(path.join(dir, "src")); fs.writeFileSync(path.join(dir, "schema.graphql"), schema); - fs.writeFileSync(path.join(dir, "src/run.graphql"), "query Run($selector: LineSelector!, $first: Int) { lines(selector: $selector, first: $first) { id } }"); + fs.writeFileSync(path.join(dir, "src/run.graphql"), "query Run($selector: LineSelector!, $first: Int, $price: Decimal) { lines(selector: $selector, first: $first) { id } }"); + if (packageJson) fs.writeFileSync(path.join(dir, "package.json"), JSON.stringify(packageJson)); fs.writeFileSync(path.join(dir, "src/index.js"), `export function targetPrepare() {\n return { variables: ${variables} };\n}\n`); const json = path.join(dir, "function.json"); fs.writeFileSync(json, JSON.stringify({ handle: "prepare-test", directory: dir, entry: "src/index.js", schema: "schema.graphql", targets, ...fn })); @@ -66,6 +68,13 @@ test("warns, without failing, about variables it can't check", (t) => { ); }); +test("checks custom scalars with the function's codegen config", (t) => { + const variables = '{ selector: { id: "1" }, price: 1.5 }'; + assert.match(runCheck(t, { variables }).stderr, /`variables.price` must be Decimal, not 1.5/); + const packageJson = { codegen: { config: { scalars: { Decimal: "number" } } } }; + assert.equal(runCheck(t, { variables, packageJson }).status, 0); +}); + test("warns, without failing, when the check itself fails", (t) => { const result = runCheck(t, { fn: { schema: "missing.graphql" } }); assert.equal(result.status, 0, result.stderr); diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index c3f20ff..510d716 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -8,13 +8,17 @@ const { prepareVariablesProblems } = require("../prepare-variables.js"); const schema = ` scalar JSON scalar Decimal + scalar Handle + scalar Void enum Mode { FAST SLOW } input Selector @oneOf { id: ID, handle: String } input Rule { handle: String!, weight: Int } type Query { shop: String, id: ID } + input CartValidationsGeneratePrepareResult { variables: JSON!, cacheKey: String } + type Mutation { cartValidationsGeneratePrepare(result: CartValidationsGeneratePrepareResult!): Void! } `; const runQuery = - "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON, $id: ID, $price: Decimal, $selector: Selector) { shop }"; + "query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON, $id: ID, $price: Decimal, $selector: Selector, $handles: [Handle!], $nothing: Void) { shop }"; const paired = [ { target: "cart.validations.generate.prepare", export: "cartValidationsGeneratePrepare" }, @@ -22,14 +26,14 @@ const paired = [ ]; // Writes a function with `files` in src/ and returns its problems, without their locations. Warnings start with "warning: ". -function check(files, targets = paired) { +function check(files, targets = paired, scalars) { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "prepare-")); try { fs.mkdirSync(path.join(directory, "src")); fs.writeFileSync(path.join(directory, "schema.graphql"), schema); fs.writeFileSync(path.join(directory, "src/run.graphql"), runQuery); for (const [file, content] of Object.entries(files)) fs.writeFileSync(path.join(directory, "src", file), content); - return prepareVariablesProblems({ directory, entry: "src/index.js", schema: "schema.graphql", targets }).map( + return prepareVariablesProblems({ directory, entry: "src/index.js", schema: "schema.graphql", targets, scalars }).map( ({ message, warning }) => `${warning ? "warning: " : ""}${message.replace(/^\S+:\d+:\d+ /, "")}`, ); } finally { @@ -183,6 +187,17 @@ describe("prepareVariablesProblems", () => { ); }); + test("checks a generic helper by what its call returns", () => { + const generic = (variables) => + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": `function first(...xs: T[]): T { return xs[0]! }\nexport function cartValidationsGeneratePrepare(input: {t: T}) { return {variables: ${variables}} }`, + }); + assert.deepEqual(generic(`{handle: first(...['a'])}`), []); + assert.deepEqual(generic(`{handle: 'a', limit: first('x')}`), ["`variables.limit` must be Int, not \"x\""]); + assert.deepEqual(generic(`{handle: input.t}`), ["warning: `variables.handle` is typed T, so it can't be checked"]); + }); + test("follows the arguments a helper is called with", () => { assert.deepEqual( check({ @@ -202,13 +217,24 @@ describe("prepareVariablesProblems", () => { ); }); + test("only checks assignments that can change what's returned", () => { + const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}`)); + assert.deepEqual(built(`const later = () => { variables.limit = 'x' }\nreturn {variables}`), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ]); + assert.deepEqual(built(`if (input.a) return {variables}\nvariables.limit = 'x'\nreturn {variables: {handle: 'b'}}`), []); + assert.deepEqual(built(`const result = {variables}\nvariables.limit = 'x'\nreturn result`), ["`variables.limit` must be Int, not \"x\""]); + assert.deepEqual(built(`for (const x of input.list) { if (x) return {variables}; variables.limit = 'x' }\nreturn {variables: {handle: 'b'}}`), [ + "`variables.limit` must be Int, not \"x\"", + ]); + }); + test("warns about a required variable that's only set on some paths", () => { const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); assert.deepEqual(assigned(`variables.handle = 'a'`), []); for (const lines of [ `if (input.a) variables.handle = 'a'`, `for (const x of input.list) variables.handle = 'a'`, - `input.list.forEach(() => { variables.handle = 'a' })`, ]) { assert.deepEqual(assigned(lines), ["warning: `variables.handle` is only set on some paths, so it can't be checked"], lines); } @@ -259,6 +285,33 @@ describe("prepareVariablesProblems", () => { ); }); + test("checks the prepare result's other fields from the schema", () => { + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 'k'}`)), []); + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 5}`)), ["`cacheKey` must be String, not 5"]); + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKy: 'k'}`)), [ + "`cacheKy` isn't a field of CartValidationsGeneratePrepareResult", + ]); + }); + + test("checks custom scalars as the type codegen gives them", () => { + const vars = (value, scalars) => check(prepare(`return {variables: {handle: 'a', ${value}}}`), undefined, scalars); + assert.deepEqual(vars(`handles: ['h']`), []); + assert.deepEqual(vars(`handles: [5]`), ["`variables.handles[]` must be Handle!, not 5"]); + assert.deepEqual(vars(`price: '1.5'`), []); + assert.deepEqual(vars(`price: 1.5`), ["`variables.price` must be Decimal, not 1.5"]); + assert.deepEqual(vars(`price: 1.5`, { Decimal: "number" }), []); + assert.deepEqual(vars(`price: 1.5`, { Decimal: { input: "unknown", output: "string" } }), []); + assert.deepEqual(vars(`data: {a: [1]}`), []); + assert.deepEqual(vars(`nothing: null`), []); + assert.deepEqual(vars(`nothing: 'x'`), ["`variables.nothing` must be Void, not \"x\""]); + }); + + test("checks null and lists for custom scalars", () => { + assert.deepEqual(check(prepare(`return {variables: {handle: 'a', handles: ['h', null]}}`)), [ + "`variables.handles[]` can be null, but Handle! can't", + ]); + }); + test("trusts non-null assertions", () => { assert.deepEqual( check({ @@ -304,7 +357,7 @@ describe("prepareVariablesProblems", () => { const MODES = ['FAST', 'SLOW'] /** @param {{a: boolean, m: string}} input */ export function cartValidationsGeneratePrepare(input) { - return {variables: {handle: (input.a && 'x') || 'y', id: 5, mode: input.a ? input.m : MODES[0], price: 1.5}} + return {variables: {handle: (input.a && 'x') || 'y', id: 5, mode: input.a ? input.m : MODES[0], price: '1.5'}} }`, }), [ @@ -320,7 +373,7 @@ describe("prepareVariablesProblems", () => { ]); }); - test("warns about an export it can't follow, and leaves missing exports and syntax errors to the bundler", () => { + test("warns about an export it can't follow, and leaves missing exports to the bundler", () => { assert.deepEqual( check({ "index.js": ` @@ -338,7 +391,20 @@ describe("prepareVariablesProblems", () => { ["warning: `cartValidationsGeneratePrepare` isn't a function it can follow, so it can't be checked"], ); assert.deepEqual(check({ "index.js": "export function other() {}" }), []); + }); + + test("warns about a syntax error, even in a file the bundle leaves out", () => { // A function missing its closing brace - assert.deepEqual(check({ "index.js": "export function cartValidationsGeneratePrepare() {\n return {variables: {handle: 1}}" }), []); + assert.deepEqual(check({ "index.js": "export function cartValidationsGeneratePrepare() {\n return {variables: {handle: 1}}" }), [ + "warning: can't be checked because of a syntax error: '}' expected.", + ]); + assert.deepEqual( + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": "import type { X } from './types.ts'\nexport function cartValidationsGeneratePrepare() { return {variables: {handle: 1}} }", + "types.ts": "export type X = { a: ; }", + }), + ["warning: can't be checked because of a syntax error: Type expected."], + ); }); }); From 7ccff4ded0a9bb2279fa6fb69ca469dabb7745e8 Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Thu, 12 Nov 2026 16:59:53 -0600 Subject: [PATCH 5/8] Tidy the prepare variables check, and check toJSON values - the check reads the codegen scalars from package.json itself, so check.js only runs it - each custom scalar is checked as one built-in scalar - smaller tidying, with no change in what's checked, and the tests grouped by topic - a value with toJSON, such as a Date, is checked as what toJSON returns, since Shopify's output writer calls it --- check.js | 12 +- prepare-variables.js | 72 ++++--- tests/prepare-variables.test.cjs | 345 ++++++++++++++++--------------- 3 files changed, 219 insertions(+), 210 deletions(-) diff --git a/check.js b/check.js index b83aa61..9ad1bd3 100644 --- a/check.js +++ b/check.js @@ -7,7 +7,6 @@ // the JavaScript function the CLI bundles. It exits non-zero, failing the build, when a prepare target returns // variables its run query doesn't accept, and warns about variables it can't check. const fs = require("node:fs"); -const path = require("node:path"); const { parseArgs } = require("node:util"); const { prepareVariablesProblems } = require("./prepare-variables.js"); @@ -18,7 +17,7 @@ try { const name = fn.handle ? `The function extension "${fn.handle}"` : "The function"; let problems; try { - problems = prepareVariablesProblems({ ...fn, scalars: codegenScalars(fn.directory) }); + problems = prepareVariablesProblems(fn); } catch (error) { // A problem in the check itself shouldn't stop the build console.warn(`${name}'s prepare variables couldn't be checked: ${error.message}`); @@ -38,12 +37,3 @@ try { console.error(error.message); process.exitCode = 1; } - -// The scalars in the function's codegen config, which Shopify CLI merges with its own -function codegenScalars(directory) { - try { - return JSON.parse(fs.readFileSync(path.join(directory, "package.json"), "utf8")).codegen?.config?.scalars ?? {}; - } catch { - return {}; - } -} diff --git a/prepare-variables.js b/prepare-variables.js index db33fed..241b7a8 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -38,7 +38,13 @@ const codegenScalars = { URL: "string", Void: "null", }; -const scalarsByType = { string: GraphQLString, number: GraphQLFloat, boolean: GraphQLBoolean }; +// The built-in scalar each of those TypeScript types is checked as. `null` is for Void, which only takes null. +const scalarsByType = { + string: GraphQLString, + number: GraphQLFloat, + boolean: GraphQLBoolean, + null: new GraphQLScalarType({ name: "Null", parseValue: () => undefined }), +}; /** The prepare targets whose run target has an input query. */ function preparePairs(targets) { @@ -50,13 +56,18 @@ function preparePairs(targets) { /** * Returns `{message, warning}` for each problem, where `message` is `file:line:column message`, relative to `directory`. - * Values that can't be checked are warnings, and mismatches are errors. `scalars` is the function's codegen config. + * Values that can't be checked are warnings, and mismatches are errors. */ -function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets, scalars = {} }) { - const scalarTypes = { ...codegenScalars }; - for (const [name, type] of Object.entries(scalars)) scalarTypes[name] = typeof type === "object" ? type?.input : type; +function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets }) { const pairs = preparePairs(targets); if (pairs.length === 0) return []; + // Shopify CLI merges the codegen config in the function's package.json with its own + const packageJson = path.join(directory, "package.json"); + const { codegen } = fs.existsSync(packageJson) ? JSON.parse(fs.readFileSync(packageJson, "utf8")) : {}; + const scalarTypes = { ...codegenScalars }; + for (const [name, type] of Object.entries(codegen?.config?.scalars ?? {})) { + scalarTypes[name] = typeof type === "object" ? type?.input : type; + } if (!entryPath) { return [{ message: "The function has no entry file, such as src/index.js, to check its prepare targets in", warning: false }]; } @@ -120,10 +131,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return ts.createSourceFile(fileName, text.join(""), language); }; const program = ts.createProgram([entry], options, host); + const isLocal = (file) => !file.isDeclarationFile && !program.isSourceFileFromExternalLibrary(file); // A syntax error stops the check. The bundler reports it too, unless the bundle leaves the file out, such as one // only imported with `import type`. - const local = program.getSourceFiles().filter((file) => !file.isDeclarationFile && !program.isSourceFileFromExternalLibrary(file)); - const syntaxErrors = local.flatMap((file) => program.getSyntacticDiagnostics(file).slice(0, 1)); + const syntaxErrors = program.getSourceFiles().filter(isLocal).flatMap((file) => program.getSyntacticDiagnostics(file).slice(0, 1)); if (syntaxErrors.length > 0) { return syntaxErrors.map(({ file, start, messageText }) => { const { line, character } = file.getLineAndCharacterOfPosition(start); @@ -155,9 +166,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const functionOf = (node) => { if (!node) return; if (ts.isVariableDeclaration(node)) return node.initializer && functionOf(unwrap(node.initializer)); - const local = - !node.getSourceFile().isDeclarationFile && !program.isSourceFileFromExternalLibrary(node.getSourceFile()); - if (local && (ts.isFunctionDeclaration(node) || ts.isFunctionExpression(node) || ts.isArrowFunction(node))) { + if (isLocal(node.getSourceFile()) && (ts.isFunctionDeclaration(node) || ts.isFunctionExpression(node) || ts.isArrowFunction(node))) { return node.body && node; } }; @@ -167,11 +176,9 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ? checker.getShorthandAssignmentValueSymbol(node.parent) : checker.getSymbolAtLocation(node); - // A reference that could change the value after it's created: assigning to it or a member, deleting or - // incrementing a member, calling a method on it, or passing, aliasing or reassigning the binding itself. - // An assignment with a written-out key, `object.key = value` or `object["key"] = value`, to a reference to the object const functionAround = (node) => ts.findAncestor(node.parent, ts.isFunctionLike); + // An assignment with a written-out key, `object.key = value` or `object["key"] = value`, to a reference to the object const assignmentOf = (reference) => { const access = reference.parent; const assignment = access.parent; @@ -183,6 +190,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP return key === undefined ? undefined : { key, name, value: assignment.right, node: assignment }; }; + // A reference that could change the value after it's created: assigning to it or a member, deleting or + // incrementing a member, calling a method on it, or passing, aliasing or reassigning the binding itself. const changes = (reference) => { let top = reference; while ( @@ -235,7 +244,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP }, }); const owner = (type) => (type === variables ? `declared by ${queryName}` : `a field of ${type.name}`); - const followed = new Map(); + const followed = new Set(); const checkedReferences = new Set(); const visiting = new Set(); const changed = new Map(); @@ -261,11 +270,11 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP }; // A variable changed or passed on after it's created may not hold what it was created with, so it's not checked // An object assigned to by name is checked with its assignments, but only where it's created - const skipChanged = (declaration, at, node) => { + const skipChanged = (declaration, at, object) => { const assignments = assigned.get(declaration); - const changes = changed.get(declaration) ?? (assignments && assignments.object !== node ? assignments.nodes : undefined); - for (const node of changes ?? []) { - report(node, `${subject(at)} is changed or passed on after it's created, so it can't be checked`, true); + const changes = changed.get(declaration) ?? (assignments && assignments.object !== object ? assignments.nodes : undefined); + for (const change of changes ?? []) { + report(change, `${subject(at)} is changed or passed on after it's created, so it can't be checked`, true); } return Boolean(changes); }; @@ -289,13 +298,11 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const typed = (type, expected, at, omittable, node) => { const named = getNamedType(expected); - // A custom scalar is checked as the TypeScript type codegen gives it, so a `Handle` is a string. The server + // A custom scalar is checked as the TypeScript type codegen gives it, so a `Handle` is a String. The server // decides what other ones, such as JSON, accept, but not whether they can be null or a list. - const custom = isScalarType(named) && !isSpecifiedScalarType(named); - const codegenType = custom ? scalarTypes[named.name] : undefined; - const loose = custom && codegenType !== "null" && !scalarsByType[codegenType]; + const checkedAs = isScalarType(named) && !isSpecifiedScalarType(named) ? scalarsByType[scalarTypes[named.name]] : named; if (unchecked(type)) { - if (!loose) report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); + if (checkedAs) report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); return; } if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); @@ -307,6 +314,11 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (isNonNullType(expected)) report(node, `${subject(at)} can be null, but ${expected} can't`); return; } + // A value with a toJSON method, such as a Date, is written out as what it returns, as JSON.stringify does + const toJSON = checker.getPropertyOfType(type, "toJSON"); + const [write] = toJSON ? checker.getSignaturesOfType(checker.getTypeOfSymbol(toJSON), ts.SignatureKind.Call) : []; + const written = write && checker.getReturnTypeOfSignature(write); + if (written && written !== type) return typed(written, expected, at, omittable, node); const inner = isNonNullType(expected) ? expected.ofType : expected; if (isListType(inner)) { const element = @@ -325,7 +337,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const expression = unwrap(spread.expression); const declaration = ts.isIdentifier(expression) && symbolOf(expression)?.valueDeclaration; if (declaration && ts.isVariableDeclaration(declaration)) { - followed.set(declaration, at); + followed.add(declaration); bindings.add(declaration); } const spreadType = checker.getTypeAtLocation(spread.expression); @@ -357,7 +369,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP report(assignment ?? node, `${subject(key)} isn't ${owner(inner)}`); } else if (assignment) { const binding = bindingOf(property); - if (binding) followed.set(binding, at); + if (binding) followed.add(binding); const initializer = ts.isPropertyAssignment(assignment) ? assignment.initializer : assignment.name; value(initializer, fieldType(field), key, field.defaultValue !== undefined); } else { @@ -384,8 +396,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } return; } - if (loose) return; - const match = isInputObjectType(inner) || codegenType === "null" ? "no" : scalarMatch(type, scalarsByType[codegenType] ?? inner); + if (!checkedAs) return; + const match = isInputObjectType(checkedAs) ? "no" : scalarMatch(type, checkedAs); if (match === "no") { report(node, `${subject(at)} must be ${expected}, not ${checker.typeToString(type)}`); } else if (match === "maybe") { @@ -419,7 +431,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // A helper's parameter holds what the call passes for it if (declaration && passed.has(declaration)) return value(passed.get(declaration), expected, at, omittable); if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer) { - followed.set(declaration, at); + followed.add(declaration); checkedReferences.add(expression); if (skipChanged(declaration, at, unwrap(declaration.initializer))) return; return value(declaration.initializer, expected, at, omittable); @@ -486,7 +498,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP muted = true; returns(fn, new GraphQLNonNull(result), "", false); muted = false; - for (const declaration of followed.keys()) { + for (const declaration of followed) { const symbol = checker.getSymbolAtLocation(declaration.name); const nodes = []; const visit = (node) => { @@ -508,7 +520,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // An assignment in a nested function, such as a callback, may run at any time or not at all const nested = nodes.some((node) => functionAround(node) !== functionAround(declaration)); if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean) && !nested) { - // An assignment in the block the object is created in always runs, unlike one in an `if`, loop or callback + // An assignment in the block the object is created in always runs, unlike one in an `if` or a loop const block = declaration.parent.parent.parent; for (const assignment of assignments) { assignment.always = ts.isExpressionStatement(assignment.node.parent) && assignment.node.parent.parent === block; diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index 510d716..cc9dd9c 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -26,14 +26,14 @@ const paired = [ ]; // Writes a function with `files` in src/ and returns its problems, without their locations. Warnings start with "warning: ". -function check(files, targets = paired, scalars) { +function check(files, targets = paired) { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "prepare-")); try { fs.mkdirSync(path.join(directory, "src")); fs.writeFileSync(path.join(directory, "schema.graphql"), schema); fs.writeFileSync(path.join(directory, "src/run.graphql"), runQuery); for (const [file, content] of Object.entries(files)) fs.writeFileSync(path.join(directory, "src", file), content); - return prepareVariablesProblems({ directory, entry: "src/index.js", schema: "schema.graphql", targets, scalars }).map( + return prepareVariablesProblems({ directory, entry: "src/index.js", schema: "schema.graphql", targets }).map( ({ message, warning }) => `${warning ? "warning: " : ""}${message.replace(/^\S+:\d+:\d+ /, "")}`, ); } finally { @@ -104,59 +104,108 @@ describe("prepareVariablesProblems", () => { assert.deepEqual(check(prepare(`return {}`)), ["`variables` is missing, and Variables! has no default"]); }); - test("checks what is built, not what the return type declares", () => { + test("only fails on values that are definitely wrong", () => { assert.deepEqual( check({ "index.js": ` - /** @returns {{variables: {handle: string}}} */ - export function cartValidationsGeneratePrepare() { - const variables = {handle: 'a', region: 'eu'} - return {variables} + const MODES = ['FAST', 'SLOW'] + /** @param {{a: boolean, m: string}} input */ + export function cartValidationsGeneratePrepare(input) { + return {variables: {handle: (input.a && 'x') || 'y', id: 5, mode: input.a ? input.m : MODES[0], price: '1.5'}} }`, }), - ["`variables.region` isn't declared by src/run.graphql"], + [ + // One for `input.m`, and one for `MODES[0]` + "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", + "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", + ], ); + assert.deepEqual(check(prepare(`return {variables: {handle: (input.a && false) || true, id: 1.5}}`)), [ + "warning: `variables.handle` is typed any, so it can't be checked", + "`variables.handle` must be String!, not true", + "`variables.id` must be ID, not 1.5", + ]); }); - test("sees through casts, @ts-ignore and helpers", () => { + test("checks the prepare result's other fields from the schema", () => { + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 'k'}`)), []); + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 5}`)), ["`cacheKey` must be String, not 5"]); + assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKy: 'k'}`)), [ + "`cacheKy` isn't a field of CartValidationsGeneratePrepareResult", + ]); + }); + + test("rejects an async prepare target", () => { + assert.deepEqual(check({ "index.js": "export async function cartValidationsGeneratePrepare() { return {variables: {handle: 'a'}} }" }), [ + "`cartValidationsGeneratePrepare` is async, so it returns a promise instead of its result", + ]); + }); + + test("checks custom scalars as the type codegen gives them", () => { + const vars = (value, scalars) => + check({ + ...prepare(`return {variables: {handle: 'a', ${value}}}`), + "../package.json": JSON.stringify({ codegen: { config: { scalars } } }), + }); + assert.deepEqual(vars(`handles: ['h']`), []); + assert.deepEqual(vars(`handles: ['h', null]`), ["`variables.handles[]` can be null, but Handle! can't"]); + assert.deepEqual(vars(`handles: [5]`), ["`variables.handles[]` must be Handle!, not 5"]); + assert.deepEqual(vars(`price: '1.5'`), []); + assert.deepEqual(vars(`price: 1.5`), ["`variables.price` must be Decimal, not 1.5"]); + assert.deepEqual(vars(`price: 1.5`, { Decimal: "number" }), []); + assert.deepEqual(vars(`price: 1.5`, { Decimal: { input: "unknown", output: "string" } }), []); + assert.deepEqual(vars(`price: 1.5`, null), ["`variables.price` must be Decimal, not 1.5"]); + assert.deepEqual(vars(`data: {a: [1]}`), []); + assert.deepEqual(vars(`nothing: null`), []); + assert.deepEqual(vars(`nothing: 'x'`), ["`variables.nothing` must be Void, not \"x\""]); + }); + + test("checks a value with toJSON as what it returns", () => { + const vars = (value) => check(prepare(`return {variables: {handle: ${value}}}`)); + assert.deepEqual(vars(`new Date()`), []); + assert.deepEqual(vars(`{toJSON: () => 'a'}`), []); + assert.deepEqual(vars(`{toJSON: () => 5}`), ["`variables.handle` must be String!, not number"]); + }); + + test("applies oneOf input object rules", () => { + const selector = (value) => check(prepare(`return {variables: {handle: 'a', selector: ${value}}}`)); + assert.deepEqual(selector(`{id: '1'}`), []); + assert.deepEqual(selector(`{id: '1', handle: 'x'}`), ["`variables.selector` must set exactly one field of Selector"]); + assert.deepEqual(selector(`{}`), ["`variables.selector` must set exactly one field of Selector"]); + assert.deepEqual(selector(`{id: null}`), ["`variables.selector.id` can be null, but ID! can't"]); + }); + + test("treats an array element as possibly undefined", () => { assert.deepEqual( check({ - "index.js": ` - export function cartValidationsGeneratePrepare(input) { - // @ts-ignore - if (input.a) return {variables: /** @type {{handle: string}} */ (JSON.parse(input.raw))} - return {variables: build()} - } - function build() { - return {handle: 2} - }`, + "index.js": "export * from './prepare.ts'", + "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list[0]}} }", }), - ["warning: `variables` is typed any, so it can't be checked", "`variables.handle` must be String!, not 2"], + ["`variables.handle` can be undefined, but String! has no default"], ); }); - test("warns about, rather than checks, variables changed or passed on after they are created", () => { + test("trusts non-null assertions", () => { assert.deepEqual( - check( - prepare(` - const variables = {handle: 'a'} - variables.limit = 'many' - tweak(variables) - return {variables}`), - ), - [ - "warning: `variables` is changed or passed on after it's created, so it can't be checked", - "warning: `variables` is changed or passed on after it's created, so it can't be checked", - ], + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list.find((x) => x)!}} }", + }), + [], ); + }); + + test("checks what is built, not what the return type declares", () => { assert.deepEqual( - check( - prepare(` - const base = {handle: 'a'} - base.handle = 1 - return {variables: {...base}}`), - ), - ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], + check({ + "index.js": ` + /** @returns {{variables: {handle: string}}} */ + export function cartValidationsGeneratePrepare() { + const variables = {handle: 'a', region: 'eu'} + return {variables} + }`, + }), + ["`variables.region` isn't declared by src/run.graphql"], ); }); @@ -187,45 +236,84 @@ describe("prepareVariablesProblems", () => { ); }); - test("checks a generic helper by what its call returns", () => { - const generic = (variables) => + test("sees through casts, @ts-ignore and helpers", () => { + assert.deepEqual( check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": `function first(...xs: T[]): T { return xs[0]! }\nexport function cartValidationsGeneratePrepare(input: {t: T}) { return {variables: ${variables}} }`, - }); - assert.deepEqual(generic(`{handle: first(...['a'])}`), []); - assert.deepEqual(generic(`{handle: 'a', limit: first('x')}`), ["`variables.limit` must be Int, not \"x\""]); - assert.deepEqual(generic(`{handle: input.t}`), ["warning: `variables.handle` is typed T, so it can't be checked"]); + "index.js": ` + export function cartValidationsGeneratePrepare(input) { + // @ts-ignore + if (input.a) return {variables: /** @type {{handle: string}} */ (JSON.parse(input.raw))} + return {variables: build()} + } + function build() { + return {handle: 2} + }`, + }), + ["warning: `variables` is typed any, so it can't be checked", "`variables.handle` must be String!, not 2"], + ); }); - test("follows the arguments a helper is called with", () => { + test("checks the last value of a key written twice", () => { + assert.deepEqual(check(prepare(`return {variables: {handle: 'a', handle: 5}}`)), ["`variables.handle` must be String!, not 5"]); + }); + + test("checks objects combined with Object.assign", () => { + assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {limit: 2})}`)), []); + assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {region: 'eu'})}`)), [ + "`variables.region` isn't declared by src/run.graphql", + ]); + }); + + test("warns about spreading an object that can have any keys", () => { assert.deepEqual( check({ - "index.js": `/** @param {{n: number}} input */ - export function cartValidationsGeneratePrepare(input) { return {variables: build(input.n)} } - /** @param {string} handle */ - function build(handle) { return {handle} }`, + "index.js": `/** @param {{json: Record}} input */ + export function cartValidationsGeneratePrepare(input) { return {variables: {handle: 'a', ...input.json}} }`, }), - ["`variables.handle` must be String!, not number"], + ["warning: `variables` spreads an object that can have any keys, so it can't be checked"], ); + }); + + test("warns about, rather than checks, variables changed or passed on after they are created", () => { assert.deepEqual( - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": "function id(x: T): T { return x }\nexport function cartValidationsGeneratePrepare() { return {variables: id({handle: 'a'})} }", - }), - [], + check( + prepare(` + const variables = {handle: 'a'} + variables.limit = 'many' + tweak(variables) + return {variables}`), + ), + [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ], + ); + assert.deepEqual( + check( + prepare(` + const base = {handle: 'a'} + base.handle = 1 + return {variables: {...base}}`), + ), + ["warning: `variables` is changed or passed on after it's created, so it can't be checked"], ); }); - test("only checks assignments that can change what's returned", () => { - const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}`)); - assert.deepEqual(built(`const later = () => { variables.limit = 'x' }\nreturn {variables}`), [ + test("checks variables assigned by name after the object is created", () => { + const assigned = (lines) => check(prepare(`const variables = ${lines}\nreturn {variables}`)); + assert.deepEqual(assigned(`{}\nvariables.handle = 'a'\nvariables['limit'] = 5`), []); + assert.deepEqual(assigned(`{handle: null}\nvariables.handle = 'a'`), []); + assert.deepEqual(assigned(`{handle: 'a'}\nif (input.a) variables.region = 'eu'`), [ + "`variables.region` isn't declared by src/run.graphql", + ]); + assert.deepEqual(assigned(`{handle: 'a'}\nvariables.limit = 'many'`), ['`variables.limit` must be Int, not "many"']); + assert.deepEqual(assigned(`{}\nvariables.limit = 5`), ["`variables.handle` is missing, and String! has no default"]); + // Keys that aren't written out, and objects spread after they're assigned to, still can't be checked + assert.deepEqual(assigned(`{handle: 'a'}\nvariables[input.key] = 5`), [ "warning: `variables` is changed or passed on after it's created, so it can't be checked", ]); - assert.deepEqual(built(`if (input.a) return {variables}\nvariables.limit = 'x'\nreturn {variables: {handle: 'b'}}`), []); - assert.deepEqual(built(`const result = {variables}\nvariables.limit = 'x'\nreturn result`), ["`variables.limit` must be Int, not \"x\""]); - assert.deepEqual(built(`for (const x of input.list) { if (x) return {variables}; variables.limit = 'x' }\nreturn {variables: {handle: 'b'}}`), [ - "`variables.limit` must be Int, not \"x\"", + assert.deepEqual(check(prepare(`const base = {}\nbase.handle = 'a'\nreturn {variables: {...base}}`)), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", ]); }); @@ -240,86 +328,46 @@ describe("prepareVariablesProblems", () => { } }); - test("applies oneOf input object rules", () => { - const selector = (value) => check(prepare(`return {variables: {handle: 'a', selector: ${value}}}`)); - assert.deepEqual(selector(`{id: '1'}`), []); - assert.deepEqual(selector(`{id: '1', handle: 'x'}`), ["`variables.selector` must set exactly one field of Selector"]); - assert.deepEqual(selector(`{}`), ["`variables.selector` must set exactly one field of Selector"]); - assert.deepEqual(selector(`{id: null}`), ["`variables.selector.id` can be null, but ID! can't"]); - }); - - test("rejects an async prepare target", () => { - assert.deepEqual(check({ "index.js": "export async function cartValidationsGeneratePrepare() { return {variables: {handle: 'a'}} }" }), [ - "`cartValidationsGeneratePrepare` is async, so it returns a promise instead of its result", + test("only checks assignments that can change what's returned", () => { + const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}`)); + assert.deepEqual(built(`const later = () => { variables.limit = 'x' }\nreturn {variables}`), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ]); + assert.deepEqual(built(`if (input.a) return {variables}\nvariables.limit = 'x'\nreturn {variables: {handle: 'b'}}`), []); + assert.deepEqual(built(`const result = {variables}\nvariables.limit = 'x'\nreturn result`), ["`variables.limit` must be Int, not \"x\""]); + assert.deepEqual(built(`for (const x of input.list) { if (x) return {variables}; variables.limit = 'x' }\nreturn {variables: {handle: 'b'}}`), [ + "`variables.limit` must be Int, not \"x\"", ]); }); - test("warns about spreading an object that can have any keys", () => { + test("follows the arguments a helper is called with", () => { assert.deepEqual( check({ - "index.js": `/** @param {{json: Record}} input */ - export function cartValidationsGeneratePrepare(input) { return {variables: {handle: 'a', ...input.json}} }`, + "index.js": `/** @param {{n: number}} input */ + export function cartValidationsGeneratePrepare(input) { return {variables: build(input.n)} } + /** @param {string} handle */ + function build(handle) { return {handle} }`, }), - ["warning: `variables` spreads an object that can have any keys, so it can't be checked"], + ["`variables.handle` must be String!, not number"], ); - }); - - test("checks the last value of a key written twice", () => { - assert.deepEqual(check(prepare(`return {variables: {handle: 'a', handle: 5}}`)), ["`variables.handle` must be String!, not 5"]); - }); - - test("checks objects combined with Object.assign", () => { - assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {limit: 2})}`)), []); - assert.deepEqual(check(prepare(`return {variables: Object.assign({}, {handle: 'a'}, {region: 'eu'})}`)), [ - "`variables.region` isn't declared by src/run.graphql", - ]); - }); - - test("treats an array element as possibly undefined", () => { assert.deepEqual( check({ "index.js": "export * from './prepare.ts'", - "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list[0]}} }", + "prepare.ts": "function id(x: T): T { return x }\nexport function cartValidationsGeneratePrepare() { return {variables: id({handle: 'a'})} }", }), - ["`variables.handle` can be undefined, but String! has no default"], + [], ); }); - test("checks the prepare result's other fields from the schema", () => { - assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 'k'}`)), []); - assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKey: 5}`)), ["`cacheKey` must be String, not 5"]); - assert.deepEqual(check(prepare(`return {variables: {handle: 'a'}, cacheKy: 'k'}`)), [ - "`cacheKy` isn't a field of CartValidationsGeneratePrepareResult", - ]); - }); - - test("checks custom scalars as the type codegen gives them", () => { - const vars = (value, scalars) => check(prepare(`return {variables: {handle: 'a', ${value}}}`), undefined, scalars); - assert.deepEqual(vars(`handles: ['h']`), []); - assert.deepEqual(vars(`handles: [5]`), ["`variables.handles[]` must be Handle!, not 5"]); - assert.deepEqual(vars(`price: '1.5'`), []); - assert.deepEqual(vars(`price: 1.5`), ["`variables.price` must be Decimal, not 1.5"]); - assert.deepEqual(vars(`price: 1.5`, { Decimal: "number" }), []); - assert.deepEqual(vars(`price: 1.5`, { Decimal: { input: "unknown", output: "string" } }), []); - assert.deepEqual(vars(`data: {a: [1]}`), []); - assert.deepEqual(vars(`nothing: null`), []); - assert.deepEqual(vars(`nothing: 'x'`), ["`variables.nothing` must be Void, not \"x\""]); - }); - - test("checks null and lists for custom scalars", () => { - assert.deepEqual(check(prepare(`return {variables: {handle: 'a', handles: ['h', null]}}`)), [ - "`variables.handles[]` can be null, but Handle! can't", - ]); - }); - - test("trusts non-null assertions", () => { - assert.deepEqual( + test("checks a generic helper by what its call returns", () => { + const generic = (variables) => check({ "index.js": "export * from './prepare.ts'", - "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list.find((x) => x)!}} }", - }), - [], - ); + "prepare.ts": `function first(...xs: T[]): T { return xs[0]! }\nexport function cartValidationsGeneratePrepare(input: {t: T}) { return {variables: ${variables}} }`, + }); + assert.deepEqual(generic(`{handle: first(...['a'])}`), []); + assert.deepEqual(generic(`{handle: 'a', limit: first('x')}`), ["`variables.limit` must be Int, not \"x\""]); + assert.deepEqual(generic(`{handle: input.t}`), ["warning: `variables.handle` is typed T, so it can't be checked"]); }); test("follows re-exports", () => { @@ -332,47 +380,6 @@ describe("prepareVariablesProblems", () => { ); }); - test("checks variables assigned by name after the object is created", () => { - const assigned = (lines) => check(prepare(`const variables = ${lines}\nreturn {variables}`)); - assert.deepEqual(assigned(`{}\nvariables.handle = 'a'\nvariables['limit'] = 5`), []); - assert.deepEqual(assigned(`{handle: null}\nvariables.handle = 'a'`), []); - assert.deepEqual(assigned(`{handle: 'a'}\nif (input.a) variables.region = 'eu'`), [ - "`variables.region` isn't declared by src/run.graphql", - ]); - assert.deepEqual(assigned(`{handle: 'a'}\nvariables.limit = 'many'`), ['`variables.limit` must be Int, not "many"']); - assert.deepEqual(assigned(`{}\nvariables.limit = 5`), ["`variables.handle` is missing, and String! has no default"]); - // Keys that aren't written out, and objects spread after they're assigned to, still can't be checked - assert.deepEqual(assigned(`{handle: 'a'}\nvariables[input.key] = 5`), [ - "warning: `variables` is changed or passed on after it's created, so it can't be checked", - ]); - assert.deepEqual(check(prepare(`const base = {}\nbase.handle = 'a'\nreturn {variables: {...base}}`)), [ - "warning: `variables` is changed or passed on after it's created, so it can't be checked", - ]); - }); - - test("only fails on values that are definitely wrong", () => { - assert.deepEqual( - check({ - "index.js": ` - const MODES = ['FAST', 'SLOW'] - /** @param {{a: boolean, m: string}} input */ - export function cartValidationsGeneratePrepare(input) { - return {variables: {handle: (input.a && 'x') || 'y', id: 5, mode: input.a ? input.m : MODES[0], price: '1.5'}} - }`, - }), - [ - // One for `input.m`, and one for `MODES[0]` - "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", - "warning: `variables.mode` is string, which Mode may not accept, so it can't be checked", - ], - ); - assert.deepEqual(check(prepare(`return {variables: {handle: (input.a && false) || true, id: 1.5}}`)), [ - "warning: `variables.handle` is typed any, so it can't be checked", - "`variables.handle` must be String!, not true", - "`variables.id` must be ID, not 1.5", - ]); - }); - test("warns about an export it can't follow, and leaves missing exports to the bundler", () => { assert.deepEqual( check({ From 8ab6ba864b0316dbfdfa90d809d673abd1bef891 Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Fri, 13 Nov 2026 08:56:02 -0600 Subject: [PATCH 6/8] Accept more valid prepare variables - an empty list typed `never[]` - a key in brackets that a `const` holds - a variable narrowed where it's used - a helper's default where its argument is undefined - a write that a later one replaces, which also catches a property that's only replaced inside an `if` - undefined fields of a oneOf input, which JSON leaves out --- prepare-variables.js | 96 +++++++++++++++++++++++--------- tests/prepare-variables.test.cjs | 49 ++++++++++++++++ 2 files changed, 118 insertions(+), 27 deletions(-) diff --git a/prepare-variables.js b/prepare-variables.js index 241b7a8..ca1de59 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -160,6 +160,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const generic = (type) => Boolean(type.flags & ts.TypeFlags.InstantiableNonPrimitive) || (type.isIntersection() && type.types.some(generic)); const unchecked = (type) => Boolean(type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) || generic(type); + const members = (type) => (type.isUnion() ? type.types : [type]); + const isUndefined = (type) => Boolean(type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)); const unwrap = (node) => (ts.isParenthesizedExpression(node) ? unwrap(node.expression) : node); @@ -186,7 +188,9 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (!direct || !ts.isBinaryExpression(assignment) || assignment.left !== access) return; if (assignment.operatorToken.kind !== ts.SyntaxKind.EqualsToken) return; const name = ts.isPropertyAccessExpression(access) ? access.name : access.argumentExpression; - const key = ts.isIdentifier(name) || ts.isStringLiteralLike(name) ? name.text : undefined; + // The key in brackets is its value, when TypeScript knows it, such as `"key"` or a `const` holding it + const keyType = ts.isElementAccessExpression(access) && checker.getTypeAtLocation(name); + const key = keyType ? (keyType.isStringLiteral() ? keyType.value : undefined) : name.text; return key === undefined ? undefined : { key, name, value: assignment.right, node: assignment }; }; @@ -305,6 +309,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (checkedAs) report(node, `${subject(at)} is typed ${checker.typeToString(type)}, so it can't be checked`, true); return; } + // No value has the type never, such as an element of `[].slice()` + if (type.flags & ts.TypeFlags.Never) return; if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); if (type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)) { if (isNonNullType(expected) && !omittable) report(node, `${subject(at)} can be undefined, but ${expected} has no default`); @@ -347,15 +353,20 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } if ([...bindings].map((binding) => skipChanged(binding, at, node)).some(Boolean)) return; const fields = inner.getFields(); - // A oneOf input object takes exactly one of its fields, which can't be null + // A oneOf input object takes exactly one of its fields, which can't be null. The others are left out, or + // undefined, which JSON leaves out. const fieldType = (field) => (inner.isOneOf ? new GraphQLNonNull(field.type) : field.type); - const assignments = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( + const canOmit = (field) => inner.isOneOf || field.defaultValue !== undefined; + const written = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( beforeReturn, ); - const assignedKeys = new Set(assignments.map((assignment) => assignment.key)); + // A write that a later one to the same key always replaces isn't returned, and neither is a property one replaces + const assignments = written.filter( + (assignment, index) => !written.slice(index + 1).some((later) => later.key === assignment.key && later.always), + ); + const replaced = new Set(assignments.filter((assignment) => assignment.always).map((assignment) => assignment.key)); for (const property of properties) { - // A property assigned after the object is created is checked with its assignments - if (assignedKeys.has(property.name)) continue; + if (replaced.has(property.name)) continue; const field = fields[property.name]; const key = join(at, property.name); // A property written in an object literal is followed to its value, keeping literal types (like an enum @@ -371,21 +382,35 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const binding = bindingOf(property); if (binding) followed.add(binding); const initializer = ts.isPropertyAssignment(assignment) ? assignment.initializer : assignment.name; - value(initializer, fieldType(field), key, field.defaultValue !== undefined); + value(initializer, fieldType(field), key, canOmit(field)); } else { - typed(checker.getTypeOfSymbol(property), fieldType(field), key, field.defaultValue !== undefined, node); + typed(checker.getTypeOfSymbol(property), fieldType(field), key, canOmit(field), node); } } for (const { key, name, value: assignedValue } of assignments) { const field = fields[key]; if (!field) report(name, `${subject(join(at, key))} isn't ${owner(inner)}`); - else value(assignedValue, fieldType(field), join(at, key), field.defaultValue !== undefined); + else value(assignedValue, fieldType(field), join(at, key), canOmit(field)); } - const keys = new Set([...properties.map((property) => property.name), ...assignedKeys]); - if (inner.isOneOf && keys.size !== 1) { - const optional = properties.some((property) => property.flags & ts.SymbolFlags.Optional); - if (optional && keys.size > 1) report(node, `${subject(at)} may set more than one field of ${inner}, so it can't be checked`, true); - else report(node, `${subject(at)} must set exactly one field of ${inner}`); + if (inner.isOneOf) { + // A field is set where it can't be undefined, and maybe set where it can be + const presence = (type, optional) => + members(type).every(isUndefined) ? "no" : optional || members(type).some(isUndefined) ? "maybe" : "yes"; + const keys = new Set([...properties.map((property) => property.name), ...assignments.map(({ key }) => key)]); + const set = [...keys].map((key) => { + const writes = assignments.filter((assignment) => assignment.key === key); + const possible = writes.map((assignment) => presence(checker.getTypeAtLocation(assignment.value), false)); + const property = !replaced.has(key) && properties.find((candidate) => candidate.name === key); + if (property) possible.push(presence(checker.getTypeOfSymbol(property), property.flags & ts.SymbolFlags.Optional)); + else if (!replaced.has(key)) possible.push("no"); + return possible.every((kind) => kind === possible[0]) ? possible[0] : "maybe"; + }); + const count = (kind) => set.filter((presence) => presence === kind).length; + if (count("yes") > 1 || count("yes") + count("maybe") === 0) { + report(node, `${subject(at)} must set exactly one field of ${inner}`); + } else if (count("maybe") > 0) { + report(node, `${subject(at)} may not set exactly one field of ${inner}, so it can't be checked`, true); + } } for (const [key, field] of Object.entries(fields)) { if (!isNonNullType(field.type) || field.defaultValue !== undefined || checker.getPropertyOfType(type, key)) continue; @@ -422,15 +447,33 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ts.forEachChild(body, visit); }; + // Checks the types `left` can have, apart from the ones `skip` matches, which use `right` instead + const orElse = (left, right, skip, expected, at, omittable) => { + for (const member of members(checker.getTypeAtLocation(left))) { + if (!skip(member)) typed(member, expected, at, omittable, left); + } + return value(right, expected, at, omittable); + }; + const value = (node, expected, at, omittable) => { const expression = unwrap(node); const inner = isNonNullType(expected) ? expected.ofType : expected; if (ts.isIdentifier(expression)) { - const declaration = symbolOf(expression)?.valueDeclaration; - // A helper's parameter holds what the call passes for it - if (declaration && passed.has(declaration)) return value(passed.get(declaration), expected, at, omittable); - if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer) { + const symbol = symbolOf(expression); + const declaration = symbol?.valueDeclaration; + // A helper's parameter holds what the call passes for it, or its default where that's undefined + if (declaration && passed.has(declaration)) { + const { argument, initializer } = passed.get(declaration); + if (!argument) return value(initializer, expected, at, omittable); + if (!initializer || !members(checker.getTypeAtLocation(argument)).some(isUndefined)) { + return value(argument, expected, at, omittable); + } + return orElse(argument, initializer, isUndefined, expected, at, omittable); + } + // A use a check narrows, such as `x` in `typeof x === "string" ? x : …`, is checked as its narrowed type + const narrowed = symbol && checker.getTypeAtLocation(expression) !== checker.getTypeOfSymbol(symbol); + if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer && !narrowed) { followed.add(declaration); checkedReferences.add(expression); if (skipChanged(declaration, at, unwrap(declaration.initializer))) return; @@ -446,13 +489,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP ) { // `??` uses the left side unless it's null or undefined, and `||` only when it's truthy const or = expression.operatorToken.kind === ts.SyntaxKind.BarBarToken; - const left = checker.getTypeAtLocation(expression.left); - for (const member of left.isUnion() ? left.types : [left]) { - if (member.flags & (ts.TypeFlags.Null | ts.TypeFlags.Undefined | ts.TypeFlags.Void)) continue; - if (or && (member === checker.getFalseType() || ((member.isStringLiteral() || member.isNumberLiteral()) && !member.value))) continue; - typed(member, expected, at, omittable, expression.left); - } - return value(expression.right, expected, at, omittable); + const falsy = (member) => + member === checker.getFalseType() || ((member.isStringLiteral() || member.isNumberLiteral()) && !member.value); + const skip = (member) => isUndefined(member) || Boolean(member.flags & ts.TypeFlags.Null) || (or && falsy(member)); + return orElse(expression.left, expression.right, skip, expected, at, omittable); } else if (ts.isCallExpression(expression)) { const fn = functionOf(checker.getResolvedSignature(expression)?.declaration); // A generic helper is checked by what its call returns, where its type parameters are known @@ -461,8 +501,10 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP visiting.add(fn); const spread = expression.arguments.some(ts.isSpreadElement); fn.parameters.forEach((parameter, index) => { - const argument = expression.arguments[index] ?? parameter.initializer; - if (!spread && argument && ts.isIdentifier(parameter.name) && !parameter.dotDotDotToken) passed.set(parameter, argument); + const argument = expression.arguments[index]; + const { initializer } = parameter; + if (spread || !ts.isIdentifier(parameter.name) || parameter.dotDotDotToken || !(argument || initializer)) return; + passed.set(parameter, { argument, initializer }); }); returns(fn, expected, at, omittable); fn.parameters.forEach((parameter) => passed.delete(parameter)); diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index cc9dd9c..9f59ad4 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -173,6 +173,11 @@ describe("prepareVariablesProblems", () => { assert.deepEqual(selector(`{id: '1', handle: 'x'}`), ["`variables.selector` must set exactly one field of Selector"]); assert.deepEqual(selector(`{}`), ["`variables.selector` must set exactly one field of Selector"]); assert.deepEqual(selector(`{id: null}`), ["`variables.selector.id` can be null, but ID! can't"]); + assert.deepEqual(selector(`{id: '1', handle: undefined}`), []); + assert.deepEqual(selector(`{id: undefined, handle: undefined}`), ["`variables.selector` must set exactly one field of Selector"]); + assert.deepEqual(selector(`{id: input.a ? '1' : undefined, handle: 'x'}`), [ + "warning: `variables.selector` may not set exactly one field of Selector, so it can't be checked", + ]); }); test("treats an array element as possibly undefined", () => { @@ -185,6 +190,23 @@ describe("prepareVariablesProblems", () => { ); }); + test("keeps the narrowing of a variable where it's used", () => { + const narrowed = (body) => + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": `export function cartValidationsGeneratePrepare(input: {handle?: string}) {\nconst handle = input.handle\n${body}\n}`, + }); + assert.deepEqual(narrowed(`return {variables: {handle: typeof handle === 'string' ? handle : 'fallback'}}`), []); + assert.deepEqual(narrowed(`if (handle) return {variables: {handle}}\nreturn {variables: {handle: 'b'}}`), []); + assert.deepEqual(narrowed(`return {variables: {handle}}`), ["`variables.handle` can be undefined, but String! has no default"]); + assert.deepEqual(narrowed(`return {variables: {handle: notDeclared}}`), ["warning: `variables.handle` is typed any, so it can't be checked"]); + }); + + test("accepts an empty list typed never[]", () => { + const files = { "index.js": "export * from './prepare.ts'", "prepare.ts": "export function cartValidationsGeneratePrepare() { return {variables: {handle: 'a', rules: [].slice()}} }" }; + assert.deepEqual(check(files), []); + }); + test("trusts non-null assertions", () => { assert.deepEqual( check({ @@ -317,6 +339,14 @@ describe("prepareVariablesProblems", () => { ]); }); + test("uses the value of a key in brackets", () => { + const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); + assert.deepEqual(assigned(`const key = 'handle'\nvariables[key] = 'a'`), []); + assert.deepEqual(assigned(`let key = 'handle'\nvariables[key] = 'a'`), [ + "warning: `variables` is changed or passed on after it's created, so it can't be checked", + ]); + }); + test("warns about a required variable that's only set on some paths", () => { const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); assert.deepEqual(assigned(`variables.handle = 'a'`), []); @@ -328,6 +358,14 @@ describe("prepareVariablesProblems", () => { } }); + test("checks the writes to a key that can be returned", () => { + const built = (lines) => check(prepare(`${lines}\nreturn {variables}`)); + assert.deepEqual(built(`const variables = {}\nvariables.handle = null\nvariables.handle = 'a'`), []); + assert.deepEqual(built(`const variables = {}\nif (input.a) variables.handle = null\nvariables.handle = 'a'`), []); + assert.deepEqual(built(`const variables = {}\nvariables.handle = 'a'\nvariables.handle = null`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(built(`const variables = {handle: null}\nif (input.a) variables.handle = 'a'`), ["`variables.handle` can be null, but String! can't"]); + }); + test("only checks assignments that can change what's returned", () => { const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}`)); assert.deepEqual(built(`const later = () => { variables.limit = 'x' }\nreturn {variables}`), [ @@ -359,6 +397,17 @@ describe("prepareVariablesProblems", () => { ); }); + test("uses a helper's default where its argument is undefined", () => { + const build = (call) => + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": `function build(handle = 'a') { return {handle} }\nexport function cartValidationsGeneratePrepare(input: {h?: string, n?: number}) { return {variables: ${call}} }`, + }); + assert.deepEqual(build(`build(undefined)`), []); + assert.deepEqual(build(`build(input.h)`), []); + assert.deepEqual(build(`build(input.n)`), ["`variables.handle` must be String!, not number"]); + }); + test("checks a generic helper by what its call returns", () => { const generic = (variables) => check({ From a36dc963257ab18109a8b4342cb4ff9839d676b4 Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Fri, 13 Nov 2026 10:13:09 -0600 Subject: [PATCH 7/8] Follow prepare variables through more helpers, casts and branches - keep what a return returns when removing a `` cast followed by a line break - follow a variable reached only through an assignment's value - check a parameter a helper narrows or reassigns as its type there - skip a write in a block that returns or throws before the return being checked --- prepare-variables.js | 122 +++++++++++++++++++------------ tests/prepare-variables.test.cjs | 39 ++++++++++ 2 files changed, 113 insertions(+), 48 deletions(-) diff --git a/prepare-variables.js b/prepare-variables.js index ca1de59..cec6e6d 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -110,7 +110,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { blank(node.expression.end, node.end); } else if (ts.isTypeAssertionExpression(node)) { - blank(node.type.pos - 1, node.expression.pos); + // Unless a line break follows it, where blanking it would end a `return ` before what it returns + if (!/[\r\n]/.test(file.text.slice(node.expression.pos, node.expression.getStart()))) blank(node.type.pos - 1, node.expression.pos); } else if ((ts.isCallExpression(node) || ts.isNewExpression(node)) && node.typeArguments) { blank(node.typeArguments.pos - 1, node.typeArguments.end + 1); } else if ( @@ -163,7 +164,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const members = (type) => (type.isUnion() ? type.types : [type]); const isUndefined = (type) => Boolean(type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)); - const unwrap = (node) => (ts.isParenthesizedExpression(node) ? unwrap(node.expression) : node); + const unwrap = (node) => + ts.isParenthesizedExpression(node) || ts.isTypeAssertionExpression(node) ? unwrap(node.expression) : node; const functionOf = (node) => { if (!node) return; @@ -208,17 +210,27 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (top === reference) { return !(ts.isSpreadAssignment(parent) || ts.isSpreadElement(parent) || ts.isTypeOfExpression(parent)); } + return written(top) || ts.isDeleteExpression(parent) || (ts.isCallExpression(parent) && parent.expression === top); + }; + + // Whether `node` is given a new value, as in `node = …`, `node += …` or `node++` + const written = (node) => { + const { parent } = node; return ( (ts.isBinaryExpression(parent) && - parent.left === top && + parent.left === node && parent.operatorToken.kind >= ts.SyntaxKind.FirstAssignment && parent.operatorToken.kind <= ts.SyntaxKind.LastAssignment) || - ts.isDeleteExpression(parent) || ((ts.isPrefixUnaryExpression(parent) || ts.isPostfixUnaryExpression(parent)) && - (parent.operator === ts.SyntaxKind.PlusPlusToken || parent.operator === ts.SyntaxKind.MinusMinusToken)) || - (ts.isCallExpression(parent) && parent.expression === top) + (parent.operator === ts.SyntaxKind.PlusPlusToken || parent.operator === ts.SyntaxKind.MinusMinusToken)) ); }; + // Whether a helper gives its parameter a new value + const reassigned = (parameter) => { + const symbol = checker.getSymbolAtLocation(parameter.name); + const visit = (node) => (ts.isIdentifier(node) && symbolOf(node) === symbol && written(node)) || ts.forEachChild(node, visit); + return Boolean(visit(parameter.parent)); + }; for (const { target, name, query } of pairs) { const queryPath = path.resolve(directory, query); @@ -255,11 +267,19 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const assigned = new Map(); const passed = new Map(); const returning = []; - // An assignment after the return being checked doesn't change what it returns, unless a loop around both runs it first - const beforeReturn = ({ node }) => { + // An assignment can't change what the return being checked returns if a return or throw follows it in a block + // the return isn't in, as in `if (…) { x.a = 1; return … }`, or if it comes after the return, unless a loop around + // both runs it first + const reachesReturn = ({ node }) => { const fn = functionAround(node); const statement = returning.findLast((candidate) => functionAround(candidate) === fn); - if (!statement || node.pos < statement.end) return true; + if (!statement) return true; + for (let child = node; !(child.parent.pos <= statement.pos && statement.end <= child.parent.end); child = child.parent) { + const statements = ts.isBlock(child.parent) || ts.isCaseOrDefaultClause(child.parent) ? child.parent.statements : []; + const later = statements.includes(child) ? statements.slice(statements.indexOf(child) + 1) : []; + if (later.some((exit) => ts.isReturnStatement(exit) || ts.isThrowStatement(exit))) return false; + } + if (node.pos < statement.end) return true; const loop = ts.findAncestor(statement, (ancestor) => (ancestor === fn ? "quit" : ts.isIterationStatement(ancestor, false))); return Boolean(loop && node.end <= loop.end); }; @@ -358,7 +378,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const fieldType = (field) => (inner.isOneOf ? new GraphQLNonNull(field.type) : field.type); const canOmit = (field) => inner.isOneOf || field.defaultValue !== undefined; const written = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( - beforeReturn, + reachesReturn, ); // A write that a later one to the same key always replaces isn't returned, and neither is a property one replaces const assignments = written.filter( @@ -462,8 +482,11 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP if (ts.isIdentifier(expression)) { const symbol = symbolOf(expression); const declaration = symbol?.valueDeclaration; - // A helper's parameter holds what the call passes for it, or its default where that's undefined - if (declaration && passed.has(declaration)) { + // A use a check narrows, such as `x` in `typeof x === "string" ? x : …`, is checked as its narrowed type + const narrowed = symbol && checker.getTypeAtLocation(expression) !== checker.getTypeOfSymbol(symbol); + // A helper's parameter holds what the call passes for it, or its default where that's undefined, unless the + // helper narrows it or gives it a new value + if (declaration && passed.has(declaration) && !narrowed && !reassigned(declaration)) { const { argument, initializer } = passed.get(declaration); if (!argument) return value(initializer, expected, at, omittable); if (!initializer || !members(checker.getTypeAtLocation(argument)).some(isUndefined)) { @@ -471,8 +494,6 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } return orElse(argument, initializer, isUndefined, expected, at, omittable); } - // A use a check narrows, such as `x` in `typeof x === "string" ? x : …`, is checked as its narrowed type - const narrowed = symbol && checker.getTypeAtLocation(expression) !== checker.getTypeOfSymbol(symbol); if (declaration && ts.isVariableDeclaration(declaration) && declaration.initializer && !narrowed) { followed.add(declaration); checkedReferences.add(expression); @@ -535,45 +556,50 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP } continue; } - // The first pass finds the variables the result is built from, and the second checks it, skipping the ones - // changed after they're created + // Muted passes find the variables the result is built from and what changes them, until a pass finds nothing new, + // such as a variable only reached through an assignment's value. The last pass checks the result, skipping the + // variables changed after they're created. muted = true; - returns(fn, new GraphQLNonNull(result), "", false); - muted = false; - for (const declaration of followed) { - const symbol = checker.getSymbolAtLocation(declaration.name); - const nodes = []; - const visit = (node) => { - if ( - ts.isIdentifier(node) && - node !== declaration.name && - !checkedReferences.has(node) && - symbolOf(node) === symbol && - changes(node) - ) { - nodes.push(node); - } - ts.forEachChild(node, visit); - }; - visit(declaration.getSourceFile()); - if (nodes.length === 0) continue; - const object = unwrap(declaration.initializer); - const assignments = nodes.map(assignmentOf); - // An assignment in a nested function, such as a callback, may run at any time or not at all - const nested = nodes.some((node) => functionAround(node) !== functionAround(declaration)); - if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean) && !nested) { - // An assignment in the block the object is created in always runs, unlike one in an `if` or a loop - const block = declaration.parent.parent.parent; - for (const assignment of assignments) { - assignment.always = ts.isExpressionStatement(assignment.node.parent) && assignment.node.parent.parent === block; + for (let found = -1; found < followed.size + checkedReferences.size; ) { + found = followed.size + checkedReferences.size; + returns(fn, new GraphQLNonNull(result), "", false); + changed.clear(); + assigned.clear(); + for (const declaration of followed) { + const symbol = checker.getSymbolAtLocation(declaration.name); + const nodes = []; + const visit = (node) => { + if ( + ts.isIdentifier(node) && + node !== declaration.name && + !checkedReferences.has(node) && + symbolOf(node) === symbol && + changes(node) + ) { + nodes.push(node); + } + ts.forEachChild(node, visit); + }; + visit(declaration.getSourceFile()); + if (nodes.length === 0) continue; + const object = unwrap(declaration.initializer); + const assignments = nodes.map(assignmentOf); + // An assignment in a nested function, such as a callback, may run at any time or not at all + const nested = nodes.some((node) => functionAround(node) !== functionAround(declaration)); + if (ts.isObjectLiteralExpression(object) && assignments.every(Boolean) && !nested) { + // An assignment in the block the object is created in always runs, unlike one in an `if` or a loop + const block = declaration.parent.parent.parent; + for (const assignment of assignments) { + assignment.always = ts.isExpressionStatement(assignment.node.parent) && assignment.node.parent.parent === block; + } + assigned.set(declaration, { object, nodes, assignments }); + } else { + changed.set(declaration, nodes); } - assigned.set(declaration, { object, nodes, assignments }); - } else { - changed.set(declaration, nodes); } } + muted = false; visiting.clear(); - followed.clear(); returns(fn, new GraphQLNonNull(result), "", false); } diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index 9f59ad4..f80ccdc 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -258,6 +258,14 @@ describe("prepareVariablesProblems", () => { ); }); + test("keeps what a return returns when it removes a cast", () => { + const files = { + "index.js": "export * from './prepare.ts'", + "prepare.ts": "export function cartValidationsGeneratePrepare() {\n return \n ({variables: {handle: 5}});\n}", + }; + assert.deepEqual(check(files), ["`variables.handle` must be String!, not 5"]); + }); + test("sees through casts, @ts-ignore and helpers", () => { assert.deepEqual( check({ @@ -358,6 +366,12 @@ describe("prepareVariablesProblems", () => { } }); + test("follows a variable reached through an assignment's value", () => { + const body = `const variables = {}\nvariables.handle = ${"VALUE"}\nconst result = {}\nresult.variables = variables\nreturn result`; + assert.deepEqual(check(prepare(body.replace("VALUE", "'a'"))), []); + assert.deepEqual(check(prepare(body.replace("VALUE", "5"))), ["`variables.handle` must be String!, not 5"]); + }); + test("checks the writes to a key that can be returned", () => { const built = (lines) => check(prepare(`${lines}\nreturn {variables}`)); assert.deepEqual(built(`const variables = {}\nvariables.handle = null\nvariables.handle = 'a'`), []); @@ -366,6 +380,14 @@ describe("prepareVariablesProblems", () => { assert.deepEqual(built(`const variables = {handle: null}\nif (input.a) variables.handle = 'a'`), ["`variables.handle` can be null, but String! can't"]); }); + test("skips a write in a block that returns or throws first", () => { + const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}\nreturn {variables}`)); + assert.deepEqual(built(`if (input.a) { variables.handle = null; return {variables: {handle: 'b'}} }`), []); + assert.deepEqual(built(`if (input.a) { variables.handle = null; throw new Error('x') }`), []); + assert.deepEqual(built(`if (input.a) { variables.handle = null; return {variables} }`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(built(`if (input.a) { variables.handle = null }`), ["`variables.handle` can be null, but String! can't"]); + }); + test("only checks assignments that can change what's returned", () => { const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}`)); assert.deepEqual(built(`const later = () => { variables.limit = 'x' }\nreturn {variables}`), [ @@ -397,6 +419,23 @@ describe("prepareVariablesProblems", () => { ); }); + test("checks a parameter a helper narrows or changes as its type there", () => { + const helper = (source) => + check({ + "index.js": "export * from './prepare.ts'", + "prepare.ts": `${source}\nexport function cartValidationsGeneratePrepare() { return {variables: build(7)} }`, + }); + assert.deepEqual(helper("function build(handle: string | number) { handle = String(handle); return {handle} }"), []); + assert.deepEqual(helper("function build(handle: unknown) { if (typeof handle === 'string') return {handle}; return {handle: 'x'} }"), []); + assert.deepEqual(helper("function build(handle: string | number) { handle = 5; return {handle} }"), ["`variables.handle` must be String!, not number"]); + assert.deepEqual(helper("function build(handle: unknown) { handle = String(handle); return {handle} }"), [ + "warning: `variables.handle` is typed unknown, so it can't be checked", + ]); + assert.deepEqual(check(prepare("function build(handle) { handle = String(handle); return {handle} }\nreturn {variables: build(7)}")), [ + "warning: `variables.handle` is typed any, so it can't be checked", + ]); + }); + test("uses a helper's default where its argument is undefined", () => { const build = (call) => check({ From 68b56d48b36c2729ed0f6658a9f683e6615f9dfe Mon Sep 17 00:00:00 2001 From: Sagarika Dasgupta Date: Fri, 13 Nov 2026 11:36:54 -0600 Subject: [PATCH 8/8] Tidy the prepare variables check and its tests No change in what's checked: - names in typed() no longer reuse the `written` helper's name - one helper builds the file:line:column of a problem - typed() uses the isUndefined helper, and the package.json is read after the check for an entry file - the tests share a helper for a TypeScript prepare target, and one for a `variables` object that's assigned to before it's returned --- prepare-variables.js | 34 ++++++------ tests/prepare-variables.test.cjs | 93 +++++++++++--------------------- 2 files changed, 49 insertions(+), 78 deletions(-) diff --git a/prepare-variables.js b/prepare-variables.js index cec6e6d..97f3e4f 100644 --- a/prepare-variables.js +++ b/prepare-variables.js @@ -61,6 +61,9 @@ function preparePairs(targets) { function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaPath, targets }) { const pairs = preparePairs(targets); if (pairs.length === 0) return []; + if (!entryPath) { + return [{ message: "The function has no entry file, such as src/index.js, to check its prepare targets in", warning: false }]; + } // Shopify CLI merges the codegen config in the function's package.json with its own const packageJson = path.join(directory, "package.json"); const { codegen } = fs.existsSync(packageJson) ? JSON.parse(fs.readFileSync(packageJson, "utf8")) : {}; @@ -68,12 +71,13 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP for (const [name, type] of Object.entries(codegen?.config?.scalars ?? {})) { scalarTypes[name] = typeof type === "object" ? type?.input : type; } - if (!entryPath) { - return [{ message: "The function has no entry file, such as src/index.js, to check its prepare targets in", warning: false }]; - } const entry = path.resolve(directory, entryPath); // Paths in messages use forward slashes, as Shopify CLI's do, on Windows too const relative = (file) => path.relative(directory, file).split(path.sep).join("/"); + const locate = (file, position) => { + const { line, character } = file.getLineAndCharacterOfPosition(position); + return `${relative(file.fileName)}:${line + 1}:${character + 1}`; + }; const schema = buildSchema(fs.readFileSync(path.resolve(directory, schemaPath), "utf8")); const options = { @@ -138,9 +142,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const syntaxErrors = program.getSourceFiles().filter(isLocal).flatMap((file) => program.getSyntacticDiagnostics(file).slice(0, 1)); if (syntaxErrors.length > 0) { return syntaxErrors.map(({ file, start, messageText }) => { - const { line, character } = file.getLineAndCharacterOfPosition(start); const error = ts.flattenDiagnosticMessageText(messageText, " "); - return { message: `${relative(file.fileName)}:${line + 1}:${character + 1} can't be checked because of a syntax error: ${error}`, warning: true }; + return { message: `${locate(file, start)} can't be checked because of a syntax error: ${error}`, warning: true }; }); } const checker = program.getTypeChecker(); @@ -150,10 +153,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP let muted = false; const report = (node, message, warning = false) => { - if (muted) return; - const file = node.getSourceFile(); - const { line, character } = file.getLineAndCharacterOfPosition(node.getStart()); - problems.set(`${relative(file.fileName)}:${line + 1}:${character + 1} ${message}`, warning); + if (!muted) problems.set(`${locate(node.getSourceFile(), node.getStart())} ${message}`, warning); }; const join = (base, key) => (base ? `${base}.${key}` : key); const subject = (at) => (at ? `\`${at}\`` : "The result"); @@ -292,8 +292,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP const binding = assignment && declaration.parent.parent; return binding && ts.isVariableDeclaration(binding) && ts.isIdentifier(binding.name) ? binding : undefined; }; - // A variable changed or passed on after it's created may not hold what it was created with, so it's not checked - // An object assigned to by name is checked with its assignments, but only where it's created + // A variable changed or passed on after it's created may not hold what it was created with, so it's not checked, + // except an object assigned to by name, which is checked with its assignments where it's created const skipChanged = (declaration, at, object) => { const assignments = assigned.get(declaration); const changes = changed.get(declaration) ?? (assignments && assignments.object !== object ? assignments.nodes : undefined); @@ -332,7 +332,7 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // No value has the type never, such as an element of `[].slice()` if (type.flags & ts.TypeFlags.Never) return; if (type.isUnion()) return type.types.forEach((member) => typed(member, expected, at, omittable, node)); - if (type.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Void)) { + if (isUndefined(type)) { if (isNonNullType(expected) && !omittable) report(node, `${subject(at)} can be undefined, but ${expected} has no default`); return; } @@ -343,8 +343,8 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // A value with a toJSON method, such as a Date, is written out as what it returns, as JSON.stringify does const toJSON = checker.getPropertyOfType(type, "toJSON"); const [write] = toJSON ? checker.getSignaturesOfType(checker.getTypeOfSymbol(toJSON), ts.SignatureKind.Call) : []; - const written = write && checker.getReturnTypeOfSignature(write); - if (written && written !== type) return typed(written, expected, at, omittable, node); + const json = write && checker.getReturnTypeOfSignature(write); + if (json && json !== type) return typed(json, expected, at, omittable, node); const inner = isNonNullType(expected) ? expected.ofType : expected; if (isListType(inner)) { const element = @@ -377,12 +377,12 @@ function prepareVariablesProblems({ directory, entry: entryPath, schema: schemaP // undefined, which JSON leaves out. const fieldType = (field) => (inner.isOneOf ? new GraphQLNonNull(field.type) : field.type); const canOmit = (field) => inner.isOneOf || field.defaultValue !== undefined; - const written = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( + const reaching = ([...assigned.values()].find((candidate) => candidate.object === node)?.assignments ?? []).filter( reachesReturn, ); // A write that a later one to the same key always replaces isn't returned, and neither is a property one replaces - const assignments = written.filter( - (assignment, index) => !written.slice(index + 1).some((later) => later.key === assignment.key && later.always), + const assignments = reaching.filter( + (assignment, index) => !reaching.slice(index + 1).some((later) => later.key === assignment.key && later.always), ); const replaced = new Set(assignments.filter((assignment) => assignment.always).map((assignment) => assignment.key)); for (const property of properties) { diff --git a/tests/prepare-variables.test.cjs b/tests/prepare-variables.test.cjs index f80ccdc..b037ef9 100644 --- a/tests/prepare-variables.test.cjs +++ b/tests/prepare-variables.test.cjs @@ -42,6 +42,10 @@ function check(files, targets = paired) { } const prepare = (body) => ({ "index.js": `export function cartValidationsGeneratePrepare(input) {\n${body}\n}` }); +// A function whose prepare target is written in TypeScript, in src/prepare.ts +const typescript = (source) => ({ "index.js": "export * from './prepare.ts'", "prepare.ts": source }); +// A prepare target that creates `variables` from `object`, then runs `lines` before returning it +const assigned = (object, lines) => check(prepare(`const variables = ${object}\n${lines}\nreturn {variables}`)); describe("prepareVariablesProblems", () => { test("accepts variables the run query declares", () => { @@ -182,20 +186,14 @@ describe("prepareVariablesProblems", () => { test("treats an array element as possibly undefined", () => { assert.deepEqual( - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list[0]}} }", - }), + check(typescript("export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list[0]}} }")), ["`variables.handle` can be undefined, but String! has no default"], ); }); test("keeps the narrowing of a variable where it's used", () => { const narrowed = (body) => - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": `export function cartValidationsGeneratePrepare(input: {handle?: string}) {\nconst handle = input.handle\n${body}\n}`, - }); + check(typescript(`export function cartValidationsGeneratePrepare(input: {handle?: string}) {\nconst handle = input.handle\n${body}\n}`)); assert.deepEqual(narrowed(`return {variables: {handle: typeof handle === 'string' ? handle : 'fallback'}}`), []); assert.deepEqual(narrowed(`if (handle) return {variables: {handle}}\nreturn {variables: {handle: 'b'}}`), []); assert.deepEqual(narrowed(`return {variables: {handle}}`), ["`variables.handle` can be undefined, but String! has no default"]); @@ -203,16 +201,13 @@ describe("prepareVariablesProblems", () => { }); test("accepts an empty list typed never[]", () => { - const files = { "index.js": "export * from './prepare.ts'", "prepare.ts": "export function cartValidationsGeneratePrepare() { return {variables: {handle: 'a', rules: [].slice()}} }" }; + const files = typescript("export function cartValidationsGeneratePrepare() { return {variables: {handle: 'a', rules: [].slice()}} }"); assert.deepEqual(check(files), []); }); test("trusts non-null assertions", () => { assert.deepEqual( - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": "export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list.find((x) => x)!}} }", - }), + check(typescript("export function cartValidationsGeneratePrepare(input: {list: string[]}) { return {variables: {handle: input.list.find((x) => x)!}} }")), [], ); }); @@ -259,10 +254,7 @@ describe("prepareVariablesProblems", () => { }); test("keeps what a return returns when it removes a cast", () => { - const files = { - "index.js": "export * from './prepare.ts'", - "prepare.ts": "export function cartValidationsGeneratePrepare() {\n return \n ({variables: {handle: 5}});\n}", - }; + const files = typescript("export function cartValidationsGeneratePrepare() {\n return \n ({variables: {handle: 5}});\n}"); assert.deepEqual(check(files), ["`variables.handle` must be String!, not 5"]); }); @@ -330,16 +322,15 @@ describe("prepareVariablesProblems", () => { }); test("checks variables assigned by name after the object is created", () => { - const assigned = (lines) => check(prepare(`const variables = ${lines}\nreturn {variables}`)); - assert.deepEqual(assigned(`{}\nvariables.handle = 'a'\nvariables['limit'] = 5`), []); - assert.deepEqual(assigned(`{handle: null}\nvariables.handle = 'a'`), []); - assert.deepEqual(assigned(`{handle: 'a'}\nif (input.a) variables.region = 'eu'`), [ + assert.deepEqual(assigned(`{}`, `variables.handle = 'a'\nvariables['limit'] = 5`), []); + assert.deepEqual(assigned(`{handle: null}`, `variables.handle = 'a'`), []); + assert.deepEqual(assigned(`{handle: 'a'}`, `if (input.a) variables.region = 'eu'`), [ "`variables.region` isn't declared by src/run.graphql", ]); - assert.deepEqual(assigned(`{handle: 'a'}\nvariables.limit = 'many'`), ['`variables.limit` must be Int, not "many"']); - assert.deepEqual(assigned(`{}\nvariables.limit = 5`), ["`variables.handle` is missing, and String! has no default"]); + assert.deepEqual(assigned(`{handle: 'a'}`, `variables.limit = 'many'`), ['`variables.limit` must be Int, not "many"']); + assert.deepEqual(assigned(`{}`, `variables.limit = 5`), ["`variables.handle` is missing, and String! has no default"]); // Keys that aren't written out, and objects spread after they're assigned to, still can't be checked - assert.deepEqual(assigned(`{handle: 'a'}\nvariables[input.key] = 5`), [ + assert.deepEqual(assigned(`{handle: 'a'}`, `variables[input.key] = 5`), [ "warning: `variables` is changed or passed on after it's created, so it can't be checked", ]); assert.deepEqual(check(prepare(`const base = {}\nbase.handle = 'a'\nreturn {variables: {...base}}`)), [ @@ -348,21 +339,19 @@ describe("prepareVariablesProblems", () => { }); test("uses the value of a key in brackets", () => { - const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); - assert.deepEqual(assigned(`const key = 'handle'\nvariables[key] = 'a'`), []); - assert.deepEqual(assigned(`let key = 'handle'\nvariables[key] = 'a'`), [ + assert.deepEqual(assigned(`{}`, `const key = 'handle'\nvariables[key] = 'a'`), []); + assert.deepEqual(assigned(`{}`, `let key = 'handle'\nvariables[key] = 'a'`), [ "warning: `variables` is changed or passed on after it's created, so it can't be checked", ]); }); test("warns about a required variable that's only set on some paths", () => { - const assigned = (lines) => check(prepare(`const variables = {}\n${lines}\nreturn {variables}`)); - assert.deepEqual(assigned(`variables.handle = 'a'`), []); + assert.deepEqual(assigned(`{}`, `variables.handle = 'a'`), []); for (const lines of [ `if (input.a) variables.handle = 'a'`, `for (const x of input.list) variables.handle = 'a'`, ]) { - assert.deepEqual(assigned(lines), ["warning: `variables.handle` is only set on some paths, so it can't be checked"], lines); + assert.deepEqual(assigned(`{}`, lines), ["warning: `variables.handle` is only set on some paths, so it can't be checked"], lines); } }); @@ -373,19 +362,17 @@ describe("prepareVariablesProblems", () => { }); test("checks the writes to a key that can be returned", () => { - const built = (lines) => check(prepare(`${lines}\nreturn {variables}`)); - assert.deepEqual(built(`const variables = {}\nvariables.handle = null\nvariables.handle = 'a'`), []); - assert.deepEqual(built(`const variables = {}\nif (input.a) variables.handle = null\nvariables.handle = 'a'`), []); - assert.deepEqual(built(`const variables = {}\nvariables.handle = 'a'\nvariables.handle = null`), ["`variables.handle` can be null, but String! can't"]); - assert.deepEqual(built(`const variables = {handle: null}\nif (input.a) variables.handle = 'a'`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(assigned(`{}`, `variables.handle = null\nvariables.handle = 'a'`), []); + assert.deepEqual(assigned(`{}`, `if (input.a) variables.handle = null\nvariables.handle = 'a'`), []); + assert.deepEqual(assigned(`{}`, `variables.handle = 'a'\nvariables.handle = null`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(assigned(`{handle: null}`, `if (input.a) variables.handle = 'a'`), ["`variables.handle` can be null, but String! can't"]); }); test("skips a write in a block that returns or throws first", () => { - const built = (lines) => check(prepare(`const variables = {handle: 'a'}\n${lines}\nreturn {variables}`)); - assert.deepEqual(built(`if (input.a) { variables.handle = null; return {variables: {handle: 'b'}} }`), []); - assert.deepEqual(built(`if (input.a) { variables.handle = null; throw new Error('x') }`), []); - assert.deepEqual(built(`if (input.a) { variables.handle = null; return {variables} }`), ["`variables.handle` can be null, but String! can't"]); - assert.deepEqual(built(`if (input.a) { variables.handle = null }`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(assigned(`{handle: 'a'}`, `if (input.a) { variables.handle = null; return {variables: {handle: 'b'}} }`), []); + assert.deepEqual(assigned(`{handle: 'a'}`, `if (input.a) { variables.handle = null; throw new Error('x') }`), []); + assert.deepEqual(assigned(`{handle: 'a'}`, `if (input.a) { variables.handle = null; return {variables} }`), ["`variables.handle` can be null, but String! can't"]); + assert.deepEqual(assigned(`{handle: 'a'}`, `if (input.a) { variables.handle = null }`), ["`variables.handle` can be null, but String! can't"]); }); test("only checks assignments that can change what's returned", () => { @@ -411,20 +398,14 @@ describe("prepareVariablesProblems", () => { ["`variables.handle` must be String!, not number"], ); assert.deepEqual( - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": "function id(x: T): T { return x }\nexport function cartValidationsGeneratePrepare() { return {variables: id({handle: 'a'})} }", - }), + check(typescript("function id(x: T): T { return x }\nexport function cartValidationsGeneratePrepare() { return {variables: id({handle: 'a'})} }")), [], ); }); test("checks a parameter a helper narrows or changes as its type there", () => { const helper = (source) => - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": `${source}\nexport function cartValidationsGeneratePrepare() { return {variables: build(7)} }`, - }); + check(typescript(`${source}\nexport function cartValidationsGeneratePrepare() { return {variables: build(7)} }`)); assert.deepEqual(helper("function build(handle: string | number) { handle = String(handle); return {handle} }"), []); assert.deepEqual(helper("function build(handle: unknown) { if (typeof handle === 'string') return {handle}; return {handle: 'x'} }"), []); assert.deepEqual(helper("function build(handle: string | number) { handle = 5; return {handle} }"), ["`variables.handle` must be String!, not number"]); @@ -438,10 +419,7 @@ describe("prepareVariablesProblems", () => { test("uses a helper's default where its argument is undefined", () => { const build = (call) => - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": `function build(handle = 'a') { return {handle} }\nexport function cartValidationsGeneratePrepare(input: {h?: string, n?: number}) { return {variables: ${call}} }`, - }); + check(typescript(`function build(handle = 'a') { return {handle} }\nexport function cartValidationsGeneratePrepare(input: {h?: string, n?: number}) { return {variables: ${call}} }`)); assert.deepEqual(build(`build(undefined)`), []); assert.deepEqual(build(`build(input.h)`), []); assert.deepEqual(build(`build(input.n)`), ["`variables.handle` must be String!, not number"]); @@ -449,10 +427,7 @@ describe("prepareVariablesProblems", () => { test("checks a generic helper by what its call returns", () => { const generic = (variables) => - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": `function first(...xs: T[]): T { return xs[0]! }\nexport function cartValidationsGeneratePrepare(input: {t: T}) { return {variables: ${variables}} }`, - }); + check(typescript(`function first(...xs: T[]): T { return xs[0]! }\nexport function cartValidationsGeneratePrepare(input: {t: T}) { return {variables: ${variables}} }`)); assert.deepEqual(generic(`{handle: first(...['a'])}`), []); assert.deepEqual(generic(`{handle: 'a', limit: first('x')}`), ["`variables.limit` must be Int, not \"x\""]); assert.deepEqual(generic(`{handle: input.t}`), ["warning: `variables.handle` is typed T, so it can't be checked"]); @@ -494,11 +469,7 @@ describe("prepareVariablesProblems", () => { "warning: can't be checked because of a syntax error: '}' expected.", ]); assert.deepEqual( - check({ - "index.js": "export * from './prepare.ts'", - "prepare.ts": "import type { X } from './types.ts'\nexport function cartValidationsGeneratePrepare() { return {variables: {handle: 1}} }", - "types.ts": "export type X = { a: ; }", - }), + check({ ...typescript("import type { X } from './types.ts'\nexport function cartValidationsGeneratePrepare() { return {variables: {handle: 1}} }"), "types.ts": "export type X = { a: ; }" }), ["warning: can't be checked because of a syntax error: Type expected."], ); });