Conversation
This comment has been minimized.
This comment has been minimized.
packages/app/src/cli/models/extensions/specifications/type-generation.ts
Show resolved
Hide resolved
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ 1 findings → ✅ No issues |
Coverage report
Test suite run success4009 tests passing in 1535 suites. Report generated by 🧪jest coverage report action from 777268a |
b207899 to
d22ff3b
Compare
|
/snapit |
|
🫰✨ Thanks @robin-drexler! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260311200812Caution After installing, validate the version by running |
d22ff3b to
56adcb7
Compare
| if (existingTypeDefinition) { | ||
| targets.forEach((target) => existingTypeDefinition.targets.add(target)) | ||
| existingTypeDefinition.toolsTypeDefinition ??= toolsTypeDefinition | ||
| if (isApiVersionNewer(apiVersion, existingTypeDefinition.apiVersion)) { |
There was a problem hiding this comment.
What is this for? We should just fail early because it's not expected for extensions on different api versions to share files
There was a problem hiding this comment.
@vividviolet what do you mean by "fail early" here btw? Actually throw or just silently do nothing?
There was a problem hiding this comment.
Wouldn't it be fine if the versions are different as long as the target exists? (Which is currently checked)
Otherwise, you'd force partners to always use the same version for all extensions, right? As it'd otherwise always abort(?)
There was a problem hiding this comment.
The types are generated per extension though. This code should only hit if they have shared imports. We shouldn't encourage sharing code between different major versions. When the types are not compatible they'd have to do a custom type guard to make it work. Even when the types are compatible it just seems risky to allow this. The toml's api version also sets the default GraphQL version which might not be compatible either.
There was a problem hiding this comment.
@vividviolet how do you suggest should the CLI behave if we encounter different versions?
There was a problem hiding this comment.
@vividviolet The more I think about this, the less I love breaking the build in that situation. It would even break if you use shared code and don't even rely on global APIs at all.
Unless I'm misunderstanding your suggestion.
There was a problem hiding this comment.
Ok fair. I will settle for a warning then and clearly tell them we're using the newer apiVersion since there are multiple
There was a problem hiding this comment.
we're using the newer apiVersion
are we really using a newer version, tho?
In the end, we create a shopify.d.ts file with something like:
import '@shopify/ui-extensions';
//@ts-ignore
declare module './shared/utils.ts' {
const shopify: (import('@shopify/ui-extensions/admin.orders-details.block.render').Api
| import('@shopify/ui-extensions/admin.product-details.action.render').Api);
const globalThis: { shopify: typeof shopify };
}What '@shopify/ui-extensions` resolves to is entirely up to where the closest tsconfig is and the node module resolution process, no?
So if shared-package has a package.json with '@shopify/ui-extensions in it (and a tsconfig), that version will be used.
If not, it'll go up to the root and use whatever version is in there.
Which versions the extensions declare for '@shopify/ui-extensions is almost irrelevant 😬
There was a problem hiding this comment.
Wait, what is isApiVersionNewer checking then? Is just checking eligibility for type generation?
There was a problem hiding this comment.
I think that's only used for the error message generated here: https://github.com/Shopify/cli/blob/generate-for-shared-folders/packages/app/src/cli/models/extensions/specifications/type-generation.ts#L247-L250
@JoviDeCroock can you maybe chime in on this thread too? Just wanna make sure I'm getting this all right.
| typeDefinitionsByFile.set(typeFilePath, currentTypeDefinitions) | ||
| } | ||
|
|
||
| export async function renderTypeDefinitions( |
There was a problem hiding this comment.
nit: can we rename this to something else? we're not rendering but returning content right?
56adcb7 to
8621f6e
Compare
8621f6e to
777268a
Compare
0028359 to
08fbe50
Compare

WHY are these changes introduced?
UI extensions can import files from shared folders outside their own extension directory (e.g. a top-level
shared/folder in the app workspace). Previously, type generation was scoped per-extension - so shared files either didn't
get types at all, or got incomplete ones because each extension only knew about its own targets.
WHAT is this pull request doing?
Changes the internal data structure for type generation from storing pre-rendered strings (
Map<string, Set<string>>) to storing structured definitions (Map<string, Map<string, SharedTypeDefinition>>). Rendering isdeferred to a new
renderTypeDefinitionsstep that runs after all extensions have contributed their types.This means when two extensions both import
shared/utils.tswith different targets, the targets get merged into asingle union type declaration instead of one overwriting the other.
Other changes:
findNearestTsConfigDirnow walks up toappDirectoryinstead ofextensionDirectory, so it can find tsconfigs for shared foldersaddTypeDefinition(handles target merging + API version tracking),assertTargetsResolvable, andrenderTypeDefinitionsas shared helpersHow to test your changes?
rm **/shopify.d.tspnpm shopify app build --path=/path/to/your-app(you might need to run this with --reset once, I'm not entirely sure. Make sure you have this setup as an app for you so you can run build)shopify.d.tsin the root with this contentMeasuring impact
Checklist