-
Notifications
You must be signed in to change notification settings - Fork 191
Add new loading mode: local #6127
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@shopify/app': patch | ||
--- | ||
|
||
Fix issue in `app build` when the app has certain extension types |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,13 @@ import ignore from 'ignore' | |
|
||
const defaultExtensionDirectory = 'extensions/*' | ||
|
||
export type AppLoaderMode = 'strict' | 'report' | ||
/** | ||
* The mode in which the app is loaded, this affects how errors are handled: | ||
* - strict: If there is any kind of error, the app won't be loaded. | ||
* - report: The app will be loaded as much as possible, errors will be reported afterwards. | ||
* - local: Errors for unknown extensions will be ignored. Other errors will prevent the app from loading. | ||
*/ | ||
export type AppLoaderMode = 'strict' | 'report' | 'local' | ||
|
||
type AbortOrReport = <T>(errorMessage: OutputMessage, fallback: T, configurationPath: string) => T | ||
|
||
|
@@ -466,6 +472,8 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS | |
|
||
if (specification) { | ||
usedKnownSpecification = true | ||
} else if (this.mode === 'local') { | ||
return undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a good sense of how the contents of the loaded app (the extensions list) are used throughout the CLI? If we knew this was only used by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is tricky, for instance the only command that uses the In this case, the |
||
} else { | ||
return this.abortOrReport( | ||
outputContent`Invalid extension type "${type}" in "${relativizePath(configurationPath)}"`, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,6 @@ async function logMetadata(app: {apiKey: string}, organization: Organization, re | |
export async function localAppContext({ | ||
directory, | ||
userProvidedConfigName, | ||
unsafeReportMode = false, | ||
}: LocalAppContextOptions): Promise<AppInterface> { | ||
// Load local specifications only | ||
const specifications = await loadLocalExtensionsSpecifications() | ||
|
@@ -162,6 +161,6 @@ export async function localAppContext({ | |
directory, | ||
userProvidedConfigName, | ||
specifications, | ||
mode: unsafeReportMode ? 'report' : 'strict', | ||
mode: 'local', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think since we dont' know who is using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Umm, my assumption was that if this is a local app, the expectation is that only local specifications can be used, and we should never crash in that case.
I can't imagine a case where you would want to load the app with the local specs, but crash if there is an unknown extension 🤔. Right now the default mode is The way I see it: |
||
}) | ||
} |
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.
It's not your change, but
usedKnownSpecification
seems pointless in this method since if there is no specification we always return early from the method, so there's no need to be checkingusedKnownSpecification
further down.Uh oh!
There was an error while loading. Please reload this page.
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.
Actually, that's still useful when not using the
local
mode. It makes sense for thereport
mode.