Fail the build when a prepare target's variables don't match its run query by saga-dasgupta · Pull Request #140 · Shopify/shopify-function-javascript

saga-dasgupta · 2026-11-10T14:48:18Z

Why

A prepare target returns the variables for its run target's input query as untyped JSON, and nothing checks them. A renamed, missing or mistyped variable deploys fine, and the run query then resolves nothing at checkout. In Rust, Shopify/shopify-function-rust#252 makes these mistakes fail to compile. This PR makes them fail the build for JavaScript functions. Developers get it by updating this package, with a Shopify CLI that includes Shopify/cli#8829.

What

Adds check.js, which Shopify CLI runs before building a JavaScript function when the package has it. It fails the build when a prepare target returns variables its run query doesn't accept:

The function extension "delivery-customization" returns prepare variables its run query doesn't accept
src/cart_delivery_options_transform_prepare.js:26:23 `variables.ruleQueri` isn't declared by src/cart_delivery_options_transform_run.graphql

The check is moved here from Shopify/cli#8829. It ignores type annotations and casts, and also checks variables assigned by name after the object is created (variables.region = …). Values it can't follow, such as parsed JSON or keys worked out at run time, get a warning instead of failing the build, so valid code still builds. Code generation is unchanged.

Tested

tests/prepare-variables.test.cjs: the checker's tests from Run @shopify/shopify_function's check before building JavaScript functions cli#8829, with the can't-be-checked cases now warnings.
tests/check.test.cjs: runs check.js as the CLI does, for passing, failing and uncheckable variables, and for a function without prepare targets.
A pnpm app with a prepare and run function, using the CLI from Run @shopify/shopify_function's check before building JavaScript functions cli#8829 and the function's unchanged config. The function builds, and its 4 test fixtures pass. A misspelled, mistyped or null variable, a renamed run query variable, and a new required variable each fail the build, with or without type annotations. Shopify CLI 5.0.0 with this package, and the new CLI with 2.0.1, build as before.

Shopify/cli#8829 can merge first: it changes nothing until the installed package has check.js.

…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.

jacobsteves · 2026-11-12T14:02:51Z

You said valid code should still build so I got an agent to try to fail something here and it found some cases.


 ┌───────────────────────┬───────────┬──────────────┐
 │ Code                  │ Result    │ Why it's     │
 │                       │           │ wrong        │
 ├───────────────────────┼───────────┼──────────────┤
 │ handle: input.a &&    │ error:    │ For ||, it   │
 │ 'x' || 'y'            │ must be   │ only strips  │
 │                       │ String!,  │ null/undefin │
 │                       │ not false │ ed from the  │
 │                       │           │ left side,   │
 │                       │           │ not falsy    │
 │                       │           │ values like  │
 │                       │           │ false        │
 ├───────────────────────┼───────────┼──────────────┤
 │ mode: input.m where   │ error:    │ The value    │
 │ m: string             │ must be   │ isn't        │
 │                       │ Mode      │ provably     │
 │                       │           │ wrong; this  │
 │                       │           │ should be a  │
 │                       │           │ warning      │
 ├───────────────────────┼───────────┼──────────────┤
 │ const                 │ error     │ Same as      │
 │ MODES=['FAST','SLOW'] │           │ above        │
 │ ; mode: MODES[0]      │           │              │
 ├───────────────────────┼───────────┼──────────────┤
 │ id: 5 for an ID       │ error     │ The GraphQL  │
 │ variable              │           │ spec accepts │
 │                       │           │ Int input    │
 │                       │           │ for ID       │
 ├───────────────────────┼───────────┼──────────────┤
 │ price: 1.5 for        │ error     │ scalarKinds  │
 │ Decimal               │           │ hard-codes   │
 │                       │           │ Decimal as   │
 │                       │           │ string, but  │
 │                       │           │ the CLI lets │
 │                       │           │ users        │
 │                       │           │ override     │
 │                       │           │ codegen      │
 │                       │           │ scalars (its │
 │                       │           │ own test     │
 │                       │           │ uses         │
 │                       │           │ Decimal:     │
 │                       │           │ 'number').   │
 │                       │           │ The check    │
 │                       │           │ ignores      │
 │                       │           │ those        │
 │                       │           │ overrides    │
 ├───────────────────────┼───────────┼──────────────┤
 │ export const aPrepare │ error:    │ Code it      │
 │ = wrap(() => …), or   │ "has no   │ can't follow │
 │ an object method      │ aPrepare  │ should be a  │
 │                       │ function  │ warning      │
 │                       │ to check" │              │
 ├───────────────────────┼───────────┼──────────────┤
 │ A syntax error in the │ the same  │ Since the    │
 │ entry file            │ misleadin │ check runs   │
 │                       │ g error   │ before       │
 │                       │           │ esbuild, it  │
 │                       │           │ hides        │
 │                       │           │ esbuild's    │
 │                       │           │ real syntax  │
 │                       │           │ error        │
 └───────────────────────┴───────────┴──────────────┘

jacobsteves · 2026-11-12T14:09:47Z

+// followed are warnings rather than passes.
+const fs = require("node:fs");
+const path = require("node:path");
+const ts = require("typescript");


If we're requiring typescript anyway, could we utilize some type coercion/validation from it instead of reimplementing graphql rules? Theres checker.isTypeAssignableTo(actual, expected)

Good suggestion, it looks like there are some cases isTypeAssignableTo doesn't work for like a plain string going to an enum which is your case 2 and 3 above. It also wouldn't catch extra keys like a misspelled variable, and GraphQL's rules don't exatly match TypeScript's (enums are strings, ID takes numbers).

Here are some options I explored with my agent:

1. TypeScript ┌───────────────────────────────────────────────────────────────────────────────────────────────┬──────────────────────────────────────────────────────────────┬───────────────────────────────────────────────────────────────────────────────┬───────────────────────────────┐ │ Option │ What TypeScript would do for us │ What it gets wrong │ Verdict │ ├───────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼───────────────────────────────┤ │ checker.isTypeAssignableTo (public in TS 5.8): compare each value's type with a type built │ The scalar and enum comparisons │ It asks whether every possible value fits, so a plain string going to an enum │ Not on its own │ │ from the run query │ │ fails (Jacob's cases 2 and 3). It doesn't catch extra keys. It can't tell 1.5 │ │ │ │ │ from an Int. │ │ ├───────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼───────────────────────────────┤ │ Have TypeScript check the returned value against a type built from the run query, using the │ Literal values, nested objects, missing keys, and extra keys │ The same false failures for a plain string going to an enum. Extra keys in an │ Interesting, but it needs the │ │ in-memory copy of the code the checker already makes, then use TypeScript's error messages │ written directly in the returned object, with exact │ object built in a const go unnoticed. any values pass silently. Int vs Float │ same special cases back │ │ │ positions │ again. │ │ ├───────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────────┼───────────────────────────────┤ │ TypeScript's own errors, for TypeScript prepare functions that declare the generated │ Everything, including step-by-step assignments │ Only works when the developer writes TypeScript with declared types │ A good follow-up for │ │ variables type │ │ │ TypeScript users │ └───────────────────────────────────────────────────────────────────────────────────────────────┴──────────────────────────────────────────────────────────────┴───────────────────────────────────────────────────────────────────────────────┴───────────────────────────────┘ 2. graphql-js, which is already a dependency ┌──────────────────────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Option │ Result │ Verdict │ ├──────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤ │ A scalar's parseValue, for │ It applies GraphQL's exact rules with clear messages: • Int accepts 5 and rejects 1.5 • ID accepts 5 and rejects 1.5 • an │ Worth doing. It would replace the checker's hand-written scalar table and rules, about 30 lines, │ │ single values │ enum accepts "FAST" and rejects "MEDIUM" • String rejects true • custom scalars like Decimal accept anything, matching "the │ with GraphQL's own implementation. A short rule stays for non-exact types like string or number. │ │ │ server decides" │ │ ├──────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤ │ getVariableValues, for a whole │ It catches a missing required variable, a wrong type, a bad enum value, a bad nested field, and 1.5 for an Int, all with GraphQL's own │ Worth doing in the test helpers. About 10 lines would check each prepare fixture's actual output │ │ variables object │ messages. It doesn't flag extra keys, as above. It needs real values, so it can't run at build time. │ against the run query, with a simple extra-keys check added. │ └──────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────┘ 3. Runtime validators (Zod, Valibot) A codegen plugin can generate Zod or Valibot schemas from the GraphQL schema. Like getVariableValues, though, they only work on real values, so they can't help at build time. They'd also add code to every function's Wasm and work to every run. graphql-js already does this job, without a new dependency. Not worth it.

I think I can certainly use graphql-js to replace some of our custom hand written code, but not all of it.

saga-dasgupta · 2026-11-12T14:38:32Z

You said valid code should still build so I got an agent to try to fail something here and it found some cases.


 ┌───────────────────────┬───────────┬──────────────┐
 │ Code                  │ Result    │ Why it's     │
 │                       │           │ wrong        │
 ├───────────────────────┼───────────┼──────────────┤
 │ handle: input.a &&    │ error:    │ For ||, it   │
 │ 'x' || 'y'            │ must be   │ only strips  │
 │                       │ String!,  │ null/undefin │
 │                       │ not false │ ed from the  │
 │                       │           │ left side,   │
 │                       │           │ not falsy    │
 │                       │           │ values like  │
 │                       │           │ false        │
 ├───────────────────────┼───────────┼──────────────┤
 │ mode: input.m where   │ error:    │ The value    │
 │ m: string             │ must be   │ isn't        │
 │                       │ Mode      │ provably     │
 │                       │           │ wrong; this  │
 │                       │           │ should be a  │
 │                       │           │ warning      │
 ├───────────────────────┼───────────┼──────────────┤
 │ const                 │ error     │ Same as      │
 │ MODES=['FAST','SLOW'] │           │ above        │
 │ ; mode: MODES[0]      │           │              │
 ├───────────────────────┼───────────┼──────────────┤
 │ id: 5 for an ID       │ error     │ The GraphQL  │
 │ variable              │           │ spec accepts │
 │                       │           │ Int input    │
 │                       │           │ for ID       │
 ├───────────────────────┼───────────┼──────────────┤
 │ price: 1.5 for        │ error     │ scalarKinds  │
 │ Decimal               │           │ hard-codes   │
 │                       │           │ Decimal as   │
 │                       │           │ string, but  │
 │                       │           │ the CLI lets │
 │                       │           │ users        │
 │                       │           │ override     │
 │                       │           │ codegen      │
 │                       │           │ scalars (its │
 │                       │           │ own test     │
 │                       │           │ uses         │
 │                       │           │ Decimal:     │
 │                       │           │ 'number').   │
 │                       │           │ The check    │
 │                       │           │ ignores      │
 │                       │           │ those        │
 │                       │           │ overrides    │
 ├───────────────────────┼───────────┼──────────────┤
 │ export const aPrepare │ error:    │ Code it      │
 │ = wrap(() => …), or   │ "has no   │ can't follow │
 │ an object method      │ aPrepare  │ should be a  │
 │                       │ function  │ warning      │
 │                       │ to check" │              │
 ├───────────────────────┼───────────┼──────────────┤
 │ A syntax error in the │ the same  │ Since the    │
 │ entry file            │ misleadin │ check runs   │
 │                       │ g error   │ before       │
 │                       │           │ esbuild, it  │
 │                       │           │ hides        │
 │                       │           │ esbuild's    │
 │                       │           │ real syntax  │
 │                       │           │ error        │
 └───────────────────────┴───────────┴──────────────┘

Thanks for catching these, I have fixed them all in this commit 5860588.

jacobsteves · 2026-11-12T16:58:59Z

I'm being a meat proxy but heres some more failures, you be the judge!

1. Read the prepare result type from the schema.
 - Find the *PrepareResult input type, for
   example from the prepare target's result:
   argument on the root mutation.
 - Check every field of that type, and replace
   only variables with the run query's variables
   type. That's what the Rust override does.
 - Today a new schema field would fail every
   build

2. Check @oneOf inputs.
 - When the input type has isOneOf, the object
   needs exactly one key, and its value can't be
   null.
 - Report zero or two or more written-out keys as
   errors. If keys come from spreads or computed
   names, warn.

3.  Keep null and shape checks for custom
 scalars
 - Check null, undefined, missing keys and list
   shape for every variable before skipping the
   value itself. Today hreq: null for a Handle!
   variable passes.
 - Type the scalars Rust maps to String as
   strings, so 5 for a Handle fails as it does in
   Rust: ID, URL, Handle, Date, DateTime,
   DateTimeWithoutTimezone, TimeWithoutTimezone.
 - Decimal should follow the effective codegen
   scalar config: the CLI defaults plus the
   user's package.json overrides. If that's not
   available, treat it as unknown.
 - JSON accepts anything, and Void accepts only
   null.

4. Generic helpers: don't fail on a type
 parameter.
 - When the type found inside a helper is a type
   parameter, check the call-site type instead,
   using
   checker.getTypeAtLocation(callExpression).
 - If that's still generic, warn. It should never
   be the error "must be Int, not T".

5. Assignments are checked wherever they appear,
    regardless of order or reachability. Both of
    these fail valid code:
     - const later = () => { variables.limit =
       'x' }, a callback that is never called.
     - if (a) return {variables};
       variables.handle = 5; return {variables:
       {…}}. The bad assignment comes after the
       only return that uses variables.

    This is rare, but it breaks the "only
    definitely wrong" rule. Possible fixes: only
    count assignments in the same block, before
    the return, and outside nested functions; or
    make the other cases warnings.

6. A syntax error in any local file turns the
    check off silently. It returns [] if any
    non-library file has a syntax error. Two
    cases never reach the bundler:
     - a file imported only with import type,
       which esbuild drops;
     - a broken generated/api.ts, or a TS file
       using syntax that TS 5.8 doesn't parse.

    The build then passes with no check and no
    message. It should warn, and ideally only
    skip when the syntax error is in a file the
    bundle actually includes.

saga-dasgupta · 2026-11-12T21:35:01Z

I'm being a meat proxy but heres some more failures, you be the judge!

1. Read the prepare result type from the schema.
 - Find the *PrepareResult input type, for
   example from the prepare target's result:
   argument on the root mutation.
 - Check every field of that type, and replace
   only variables with the run query's variables
   type. That's what the Rust override does.
 - Today a new schema field would fail every
   build

2. Check @oneOf inputs.
 - When the input type has isOneOf, the object
   needs exactly one key, and its value can't be
   null.
 - Report zero or two or more written-out keys as
   errors. If keys come from spreads or computed
   names, warn.

3.  Keep null and shape checks for custom
 scalars
 - Check null, undefined, missing keys and list
   shape for every variable before skipping the
   value itself. Today hreq: null for a Handle!
   variable passes.
 - Type the scalars Rust maps to String as
   strings, so 5 for a Handle fails as it does in
   Rust: ID, URL, Handle, Date, DateTime,
   DateTimeWithoutTimezone, TimeWithoutTimezone.
 - Decimal should follow the effective codegen
   scalar config: the CLI defaults plus the
   user's package.json overrides. If that's not
   available, treat it as unknown.
 - JSON accepts anything, and Void accepts only
   null.

4. Generic helpers: don't fail on a type
 parameter.
 - When the type found inside a helper is a type
   parameter, check the call-site type instead,
   using
   checker.getTypeAtLocation(callExpression).
 - If that's still generic, warn. It should never
   be the error "must be Int, not T".

5. Assignments are checked wherever they appear,
    regardless of order or reachability. Both of
    these fail valid code:
     - const later = () => { variables.limit =
       'x' }, a callback that is never called.
     - if (a) return {variables};
       variables.handle = 5; return {variables:
       {…}}. The bad assignment comes after the
       only return that uses variables.

    This is rare, but it breaks the "only
    definitely wrong" rule. Possible fixes: only
    count assignments in the same block, before
    the return, and outside nested functions; or
    make the other cases warnings.

6. A syntax error in any local file turns the
    check off silently. It returns [] if any
    non-library file has a syntax error. Two
    cases never reach the bundler:
     - a file imported only with import type,
       which esbuild drops;
     - a broken generated/api.ts, or a TS file
       using syntax that TS 5.8 doesn't parse.

    The build then passes with no check and no
    message. It should warn, and ideally only
    skip when the syntax error is in a file the
    bundle actually includes.

All good suggestions, I updated the PR for all of them.

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

- 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

- 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

- 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

- 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

- keep what a return returns when removing a `<T>` 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

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

saga-dasgupta changed the title ~~Type a prepare target's variables with its run query's variables type~~ Check prepare targets' variables when generating a function's types Nov 10, 2026

saga-dasgupta mentioned this pull request Nov 10, 2026

Run @shopify/shopify_function's check before building JavaScript functions Shopify/cli#8829

Closed

4 tasks done

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch from 8455e36 to 73807fd Compare November 10, 2026 18:17

saga-dasgupta mentioned this pull request Nov 10, 2026

Type a prepare result's variables with its run query's variables type #141

Closed

saga-dasgupta changed the title ~~Check prepare targets' variables when generating a function's types~~ Fail the build when a prepare target's variables don't match its run query Nov 10, 2026

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch 2 times, most recently from 266f89a to 46b5387 Compare November 10, 2026 21:31

saga-dasgupta marked this pull request as ready for review November 10, 2026 21:37

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch from 46b5387 to cec9bb3 Compare November 10, 2026 21:45

jacobsteves reviewed Nov 12, 2026

View reviewed changes

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch from cec9bb3 to 09287da Compare November 12, 2026 14:31

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch 2 times, most recently from 5e863d3 to 3d456bb Compare November 12, 2026 15:13

saga-dasgupta added 6 commits November 13, 2026 10:46

saga-dasgupta force-pushed the saga/custom-scalar-overrides branch from 81dbef5 to a36dc96 Compare November 13, 2026 16:49

Navigation Menu

Repository navigation

Fail the build when a prepare target's variables don't match its run query - #140

Fail the build when a prepare target's variables don't match its run query#140
saga-dasgupta wants to merge 8 commits into
mainfrom
saga/custom-scalar-overrides

saga-dasgupta commented Nov 10, 2026 •

edited

Loading

Uh oh!

jacobsteves commented Nov 12, 2026

Uh oh!

jacobsteves Nov 12, 2026

Uh oh!

saga-dasgupta Nov 12, 2026

Uh oh!

saga-dasgupta Nov 12, 2026

Uh oh!

saga-dasgupta commented Nov 12, 2026 •

edited

Loading

Uh oh!

jacobsteves commented Nov 12, 2026

Uh oh!

saga-dasgupta commented Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

saga-dasgupta commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Tested

Uh oh!

jacobsteves commented Nov 12, 2026

Uh oh!

jacobsteves Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

saga-dasgupta Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

saga-dasgupta Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

saga-dasgupta commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobsteves commented Nov 12, 2026

Uh oh!

saga-dasgupta commented Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

saga-dasgupta commented Nov 10, 2026 •

edited

Loading

saga-dasgupta commented Nov 12, 2026 •

edited

Loading