Skip to content

Commit c13ea3e

Browse files
committed
Check JS function prepare variables against the run query on deploy
A prepare target returns the variables for its run target's input query, but nothing checks them until the function runs, and a misspelled key silently falls back to the variable's default. Pre-deploy validation now follows what each prepare export returns back to where it's built, past type annotations and casts, and compares it with the run query's variable definitions. Anything it can't follow is an error.
1 parent 1cf6e57 commit c13ea3e

7 files changed

Lines changed: 562 additions & 2 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@shopify/app': minor
3+
'@shopify/cli': minor
4+
---
5+
6+
Fail `shopify app deploy` when a JavaScript function's prepare target returns variables its run target's input query doesn't accept

‎packages/app/src/cli/models/extensions/specifications/function.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {createExtensionSpecification} from '../specification.js'
22
import {BaseSchema} from '../schemas.js'
33
import {loadLocalesConfig} from '../../../utilities/extensions/locales-configuration.js'
44
import {ExtensionInstance} from '../extension-instance.js'
5+
import {prepareVariablesErrors} from '../../../services/function/prepare-variables.js'
56
import {zod} from '@shopify/cli-kit/node/schema'
67
import {joinPath} from '@shopify/cli-kit/node/path'
78
import {fileExists, readFile} from '@shopify/cli-kit/node/fs'
@@ -196,6 +197,15 @@ const functionSpec = createExtensionSpecification({
196197
`Make sure the build command outputs the wasm in the expected directory.`,
197198
)
198199
}
200+
201+
if (!extension.isJavaScript) return
202+
const errors = await prepareVariablesErrors(extension)
203+
if (errors.length > 0) {
204+
throw new AbortError(
205+
outputContent`The function extension "${extension.handle}" returns prepare variables its run query doesn't accept`,
206+
errors.join('\n'),
207+
)
208+
}
199209
},
200210
})
201211

‎packages/app/src/cli/models/extensions/specifications/type-generation.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {zod} from '@shopify/cli-kit/node/schema'
88
import {createRequire} from 'module'
99
import type ts from 'typescript'
1010

11-
async function loadTypeScript(): Promise<typeof ts> {
11+
export async function loadTypeScript(): Promise<typeof ts> {
1212
// typescript is CJS; dynamic import wraps it as { default: ... }
1313
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1414
const mod: any = await import('typescript')

‎packages/app/src/cli/services/function/build.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ interface FunctionPackageJson {
4545
codegen?: FunctionCodegenConfig
4646
}
4747

48-
const shopifyFunctionCodegenDefaults = {
48+
export const shopifyFunctionCodegenDefaults = {
4949
defaultScalarType: 'unknown',
5050
scalars: {
5151
Date: 'string',
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
import {prepareVariablesErrors} from './prepare-variables.js'
2+
import {testFunctionExtension} from '../../models/app/app.test-data.js'
3+
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
4+
import {inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
5+
import {joinPath} from '@shopify/cli-kit/node/path'
6+
import {describe, expect, test} from 'vitest'
7+
8+
const schema = `
9+
scalar JSON
10+
enum Mode { FAST SLOW }
11+
input Rule { handle: String!, weight: Int }
12+
type Query { shop: String }
13+
`
14+
const runQuery = 'query Run($handle: String!, $limit: Int = 5, $mode: Mode, $rules: [Rule!], $data: JSON) { shop }'
15+
16+
function config(targeting: FunctionConfigType['targeting']) {
17+
return {
18+
name: 'function',
19+
type: 'function',
20+
description: '',
21+
api_version: '2026-07',
22+
configuration_ui: false,
23+
build: {wasm_opt: true},
24+
targeting,
25+
} as FunctionConfigType
26+
}
27+
28+
const paired = config([
29+
{target: 'cart.validations.generate.prepare', export: 'cart-validations-generate-prepare'},
30+
{target: 'cart.validations.generate.run', input_query: 'src/run.graphql', export: 'cart-validations-generate-run'},
31+
])
32+
33+
async function check(files: {[path: string]: string}, configuration = paired) {
34+
return inTemporaryDirectory(async (dir) => {
35+
await mkdir(joinPath(dir, 'src'))
36+
await writeFile(joinPath(dir, 'schema.graphql'), schema)
37+
await writeFile(joinPath(dir, 'src/run.graphql'), runQuery)
38+
await Promise.all(Object.entries(files).map(([path, content]) => writeFile(joinPath(dir, 'src', path), content)))
39+
const fun = await testFunctionExtension({dir, config: configuration, entryPath: joinPath(dir, 'src/index.js')})
40+
return (await prepareVariablesErrors(fun)).map((error) => error.replace(/^\S+:\d+:\d+ /, ''))
41+
})
42+
}
43+
44+
const prepare = (body: string) => ({'index.js': `export function cartValidationsGeneratePrepare(input) {\n${body}\n}`})
45+
46+
describe('prepareVariablesErrors', () => {
47+
test('accepts variables the run query declares', async () => {
48+
await expect(
49+
check(
50+
prepare(`
51+
const region = String(input.region)
52+
return {
53+
variables: {
54+
handle: region ? \`rule-\${region.toLowerCase()}\` : 'default',
55+
limit: undefined,
56+
mode: region === 'eu' ? 'FAST' : null,
57+
rules: [{handle: 'a', weight: 1}],
58+
data: JSON.parse(input.data),
59+
},
60+
}`),
61+
),
62+
).resolves.toEqual([])
63+
})
64+
65+
test('ignores functions without a prepare target', async () => {
66+
await expect(
67+
check({}, config([{target: 'cart.validations.generate.run', input_query: 'src/run.graphql', export: 'run'}])),
68+
).resolves.toEqual([])
69+
})
70+
71+
test('rejects keys the run query does not declare', async () => {
72+
await expect(check(prepare(`return {variables: {handel: 'a'}}`))).resolves.toEqual([
73+
"`variables.handel` isn't declared by src/run.graphql",
74+
'`variables.handle` is missing, and String! has no default',
75+
])
76+
})
77+
78+
test('rejects values of the wrong type', async () => {
79+
await expect(
80+
check(
81+
prepare(`return {variables: {handle: 1, limit: 1.5, mode: 'MEDIUM', rules: [{handle: 'a', weight: '1'}]}}`),
82+
),
83+
).resolves.toEqual([
84+
'`variables.handle` must be String!, not 1',
85+
'`variables.limit` must be Int, not 1.5',
86+
'`variables.mode` must be Mode, not "MEDIUM"',
87+
'`variables.rules[].weight` must be Int, not "1"',
88+
])
89+
})
90+
91+
test('rejects null and undefined where the variable needs a value', async () => {
92+
await expect(
93+
check(prepare(`return input.a ? {variables: {handle: null}} : {variables: {handle: undefined}}`)),
94+
).resolves.toEqual([
95+
"`variables.handle` can be null, but String! can't",
96+
'`variables.handle` can be undefined, but String! has no default',
97+
])
98+
await expect(check(prepare(`return {}`))).resolves.toEqual([
99+
'`variables` is missing, and Variables! has no default',
100+
])
101+
})
102+
103+
test('checks what is built, not what the return type declares', async () => {
104+
await expect(
105+
check({
106+
'index.js': `
107+
/** @returns {{variables: {handle: string}}} */
108+
export function cartValidationsGeneratePrepare() {
109+
const variables = {handle: 'a', region: 'eu'}
110+
return {variables}
111+
}`,
112+
}),
113+
).resolves.toEqual(["`variables.region` isn't declared by src/run.graphql"])
114+
})
115+
116+
test('sees through casts, @ts-ignore and helpers', async () => {
117+
await expect(
118+
check({
119+
'index.js': `
120+
export function cartValidationsGeneratePrepare(input) {
121+
// @ts-ignore
122+
if (input.a) return {variables: /** @type {{handle: string}} */ (JSON.parse(input.raw))}
123+
return {variables: build()}
124+
}
125+
function build() {
126+
return {handle: 2}
127+
}`,
128+
}),
129+
).resolves.toEqual([
130+
"`variables` is typed any, so it can't be checked",
131+
'`variables.handle` must be String!, not 2',
132+
])
133+
})
134+
135+
test('rejects variables changed or passed on after they are created', async () => {
136+
await expect(
137+
check(
138+
prepare(`
139+
const variables = {handle: 'a'}
140+
variables.limit = 'many'
141+
tweak(variables)
142+
return {variables}`),
143+
),
144+
).resolves.toEqual([
145+
"`variables` is changed or passed on after it's created, so it can't be checked",
146+
"`variables` is changed or passed on after it's created, so it can't be checked",
147+
])
148+
})
149+
150+
test('follows re-exports', async () => {
151+
await expect(
152+
check({
153+
'index.js': `export * from './prepare.js'`,
154+
'prepare.js': `export const cartValidationsGeneratePrepare = () => ({variables: {handle: 1}})`,
155+
}),
156+
).resolves.toEqual(['`variables.handle` must be String!, not 1'])
157+
})
158+
159+
test('rejects a missing prepare export', async () => {
160+
await expect(check({'index.js': 'export function other() {}'})).resolves.toEqual([
161+
expect.stringMatching(/index\.js has no `cartValidationsGeneratePrepare` function to check$/),
162+
])
163+
})
164+
})

0 commit comments

Comments
 (0)