prepend api_version to schema#7480
Conversation
| test('prepends a comment block with the version marker', () => { | ||
| const result = prependSchemaVersionHeader('type Query { id: ID }', '2025-10') | ||
|
|
||
| expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10`)).toBe(true) |
There was a problem hiding this comment.
nit: can we assert there's a newline at the end here?
| expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10`)).toBe(true) | |
| expect(result.startsWith(`${SCHEMA_VERSION_MARKER_PREFIX}2025-10\n`)).toBe(true) |
| }) | ||
| }) | ||
|
|
||
| test('no-ops when the schema file has no version marker (legacy)', async () => { |
There was a problem hiding this comment.
nit: can we remove legacy, I think it's something good to have in perpetuity, but legacy implies to me that at some point this case might go away
| await expect(result).rejects.toThrow(/2025-07/) | ||
| await expect(result).rejects.toThrow(/2025-10/) |
There was a problem hiding this comment.
can we combine these into one regex so we know that the ordering is correct?
| // Only inspect the leading comment block — bail out as soon as we see a | ||
| // non-comment, non-empty line so we don't scan the whole SDL. | ||
| for (const line of contents.split(/\r?\n/)) { | ||
| if (line.startsWith(SCHEMA_VERSION_MARKER_PREFIX)) { | ||
| return line.slice(SCHEMA_VERSION_MARKER_PREFIX.length) | ||
| } | ||
| if (!line.startsWith('#')) break | ||
| } |
There was a problem hiding this comment.
do we really need to inspect the whole comment block and not just the first line?
There was a problem hiding this comment.
My concern here was trying to not rely on being the first line. Stuff like:
- Someone adds a # DO NOT EDIT / license / TODO comment above it
- A future change prepends another metadata line (e.g. # generated_at: …)
- Some tool reformats the file
I guess for now, since I'm the only one adding a special metadata comment, it's fine to just look at the first line.
| localIdentifier, | ||
| apiVersion, | ||
| }: ValidateSchemaApiVersionOptions): Promise<void> { | ||
| const schemaPath = joinPath(directory, 'schema.graphql') |
There was a problem hiding this comment.
hmm, not ideal that we're hardcoding the file name but I don't think we have this in the config anywhere 🤔
There was a problem hiding this comment.
Like we discussed on slack, we've got this hardcoded in a few places.
It's actually just the function build command that ends up looking at the package.json/codegen/schema property, so there's already a disconnect today if someone changes that file.
I've created an issue for this, but for this PR, I'll keep things consistent with the rest of the code.
| // non-comment, non-empty line so we don't scan the whole SDL. | ||
| for (const line of contents.split(/\r?\n/)) { | ||
| if (line.startsWith(SCHEMA_VERSION_MARKER_PREFIX)) { | ||
| return line.slice(SCHEMA_VERSION_MARKER_PREFIX.length) |
There was a problem hiding this comment.
should we trim too in case there is leading or trailing whitespace?
| expect(runWasmOpt).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('validates the schema api_version with the values from the extension config', async () => { |
There was a problem hiding this comment.
The title of this test makes me think that there will be some validation assertion but only thing we do is assert that we called the validate function.
There was a problem hiding this comment.
Fair enough. The actual logic is covered in the schema-version code so this is in fact just making sure we're invoking it with the right params
|
|
||
| extension.outputPath = joinPath(extension.directory, relativeBuildPath) | ||
|
|
||
| if (functionConfiguration.api_version) { |
There was a problem hiding this comment.
Why do we need this condition, why wouldn't there be an api_version in the function_configuration?
| directory, | ||
| localIdentifier, | ||
| apiVersion, | ||
| }: ValidateSchemaApiVersionOptions): Promise<void> { |
There was a problem hiding this comment.
Should the validate function just return a pure True or False type validation (with it defaulting to True if it cant verify because the schema file doesn't have the version) and then the extension file is responsible for throwing the AbortError like it does for other cases? So this method is pure validation, not responsible for responding to the user, that happens at the caller?
There was a problem hiding this comment.
I don't think so? I'm not sure what the other cases are here.
In extensions.ts, we're catching errors from other things (such as trying to acquire the lock) and re-throwing an AbortError. I think this is different than validators, such as validateShopifyFunctionPackageVersion, which do throw their own errors.
The validator also knows what it's looking for, and the error provided can include more detail (i.e. the different versions).
| const {api_version: version, type, targeting} = extension.configuration | ||
| const usingTargets = Boolean(targeting?.length) | ||
| const definition = await (usingTargets | ||
| const fetchedDefinition = await (usingTargets |
There was a problem hiding this comment.
I dont understand the difference between definition and fetched_definition, what if we keep this definition and call the other definition_with_version or something?
There was a problem hiding this comment.
definition is essentially what was here before, the contents we're about to write to the local schema.
I introduced fetchedDefinition to name the intermediate value: the raw schema we just grabbed from the server, before the prepending step.
Reading top-down: fetch -> add version header -> write.
Whereas before it was just fetch -> write.
Open to renaming if it still reads confusingly, but it felt clearer to me separated.
WHY are these changes introduced?
Part of https://github.com/shop/issues-shopifyvm/issues/945
When a developer bumps
api_versionin a function'sshopify.extension.toml, the on-diskschema.graphqlbecomes stale. The nextshopify app function buildthen fails with crypticgraphql-code-generatorerrors like:…with no indication that the underlying problem is a stale schema that just needs to be re-fetched. Today the build path is fully local — it never consults
api_versionandschema.graphqlcarries no version metadata, so staleness is undetectable.The error surfaced gives them no actionable hint pointing at
shopify app function schema.WHAT is this pull request doing?
Stamps the
api_versionintoschema.graphqlwhenshopify app function schemawrites it, then validates that marker against the extension's TOML at the start of everyshopify app function build. When they disagree, the build aborts up-front with an actionable message instead of letting codegen fail mysteriously.New file
packages/app/src/cli/services/function/schema-version.ts— self-contained helpers:prependSchemaVersionHeader(definition, version)— adds a# api_version: <ver>marker line to the top of the SDL.readSchemaApiVersion(path)— scans the leading comment block for the marker; returnsundefinedfor missing files or legacy schemas (no marker).validateSchemaApiVersion({directory, localIdentifier, apiVersion})— throws anAbortErrorwith remediation when the on-disk marker disagrees with the configuredapi_version.Modified files
services/generate-schema.ts—shopify app function schemanow writes the marker as the first line of the SDL (both file and stdout output).services/build/extension.ts—buildFunctionExtensioninvokesvalidateSchemaApiVersiononce, up-front, before any function build work (typegen, esbuild, javy, wasm-opt, trampoline).Behavior
shopify app function schema).The schema header is a single line:
How to test your changes?
api_version = "2025-07"in the function'sshopify.extension.toml.schema.graphqlnow starts with# api_version: 2025-07.api_versionto"2025-10"in the TOML.shopify app function schema.schema.graphql(simulate an older fetched file). The build command should behave exactly as it did before this change (no false-positive abort).Post-release steps
N/A.
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add