-
Notifications
You must be signed in to change notification settings - Fork 225
Support flexible templates for app init
#6751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3611 tests passing in 1423 suites. Report generated by 🧪jest coverage report action from 770bb28 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
5599003 to
770bb28
Compare
gonzaloriestra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving because we need it for the next release and it works. But all the loading code was already too confusing and this is adding more complexity... So I hope we can simplify later.
I have this draft PR to remove the legacy schema by adding a client_id to the templates. Maybe we could support this as well without adding a new schema.
| * Uses passthrough() to allow any extra keys from full-featured templates | ||
| * (e.g., metafields, metaobjects, webhooks) without strict validation. | ||
| */ | ||
| export const TemplateConfigSchema = zod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see more differences with LegacyAppSchema besides the passthrough (like extension_directories). Is that ok?
| return { | ||
| isLaunchable, | ||
| scopesArray: getAppScopesArray(config), | ||
| scopesArray: getTemplateScopesArray(config), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding a new function instead of updating the existing one?
| const isLaunchable = webs.webs.some((web) => isWebType(web, WebType.Frontend) || isWebType(web, WebType.Backend)) | ||
| // Use permissive schema to allow templates with extra configuration (metafields, metaobjects, etc.) | ||
| const config = await parseConfigurationFile(TemplateConfigSchema, configurationPath) | ||
| const webs = await loadWebsForAppCreation(appDirectory, config.web_directories) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use the regular loadWebs?

WHY are these changes introduced?
Fixes an issue where app templates using certain module types cause
app initto fail. The issue is due to the app template using module types that aren't defined within the CLI, but more generally due toapp initvalidating/understanding app configuration.WHAT is this pull request doing?
Instead, we process app config during app instantiation as opaque blobs -- we don't rely on being able to validate them or a particular structure being in place.
In
app initthe app is loaded twice -- once as a template, and then again as the local copy of the app is linked to the remote version of it.How to test your changes?
Locally:
pnpm shopify app init --path="<where I keep my apps>" --template="https://github.com/Shopify/shopify-app-template-react-router#include-declarative-definition"Measuring impact
How do we know this change was effective? Please choose one:
Checklist