-
Notifications
You must be signed in to change notification settings - Fork 224
Add resilliency to errors on concurrent build asset writes #6559
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
Add resilliency to errors on concurrent build asset writes #6559
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3358 tests passing in 1372 suites. Report generated by 🧪jest coverage report action from b49c429 |
8e08b52 to
d7ebe8d
Compare
fdbf531 to
24a4df1
Compare
d7ebe8d to
df86305
Compare
24a4df1 to
84e181a
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
dmerand
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.
Couldn't tophat based on the instructions (assuming shopify app build?) but I'm probably missing something basic there.
Otherwise I really just have the one question about DRY.
df86305 to
3326b7e
Compare
dff9184 to
554d223
Compare
3326b7e to
4e68502
Compare
554d223 to
60859e7
Compare
|
/snapit |
|
🫰✨ Thanks @isaacroldan! 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/[email protected]Caution After installing, validate the version by running just |
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.
Again, I'm not sure about the approach. But maybe I'm missing context about the problem...
Why would you want to continue if something couldn't be copied? What's the problem with throwing an error instead of a warning?
|
when you have automatic build from an external vite running in parallel with dev which is, with every change that you do, deleting the assets folder and rewriting it, it may happend that you delete something that is listed in glob to be copied |
60859e7 to
54bda56
Compare
4e68502 to
5e2a2ce
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/analytics.d.ts@@ -27,6 +27,5 @@ interface EnvironmentData {
export declare function getEnvironmentData(config: Interfaces.Config): Promise<EnvironmentData>;
export declare function getSensitiveEnvironmentData(config: Interfaces.Config): Promise<{
env_plugin_installed_all: string;
- env_shopify_variables: string;
}>;
export {};
\ No newline at end of file
packages/cli-kit/dist/private/node/api.d.ts@@ -11,30 +11,6 @@ type RequestOptions<T> = {
request: () => Promise<T>;
url: string;
} & NetworkRetryBehaviour;
-/**
- * Checks if an error is a transient network error that is likely to recover with retries.
- *
- * Use this function for retry logic. Use isNetworkError for error classification.
- *
- * Examples of transient errors (worth retrying):
- * - Connection timeouts, resets, and aborts
- * - DNS failures (enotfound, getaddrinfo, eai_again) - can be temporary
- * - Socket disconnects and hang ups
- * - Premature connection closes
- */
-export declare function isTransientNetworkError(error: unknown): boolean;
-/**
- * Checks if an error is any kind of network-related error (connection issues, timeouts, DNS failures,
- * TLS/certificate errors, etc.) rather than an application logic error.
- *
- * These errors should be reported as user-facing errors (AbortError) rather than bugs (BugError),
- * regardless of whether they are transient or permanent.
- *
- * Examples include:
- * - Transient: connection timeouts, socket hang ups, temporary DNS failures
- * - Permanent: certificate validation failures, misconfigured SSL
- */
-export declare function isNetworkError(error: unknown): boolean;
export declare function simpleRequestWithDebugLog<T extends {
headers: Headers;
status: number;
packages/cli-kit/dist/public/node/metadata.d.ts@@ -55,7 +55,6 @@ declare const coreData: RuntimeMetadataManager<CmdFieldsFromMonorail, {
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
- env_shopify_variables?: string | null;
}, "store_", never>>;
export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>, getAllSensitiveMetadata: () => Partial<{
commandStartOptions: {
@@ -75,7 +74,6 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
- env_shopify_variables?: string | null;
}, "store_", never>>, addPublicMetadata: (getData: ProvideMetadata<CmdFieldsFromMonorail>, onError?: MetadataErrorHandling) => Promise<void>, addSensitiveMetadata: (getData: ProvideMetadata<{
commandStartOptions: {
startTime: number;
@@ -94,7 +92,6 @@ export declare const getAllPublicMetadata: () => Partial<CmdFieldsFromMonorail>,
cmd_all_environment_flags?: string | null;
cmd_dev_tunnel_custom?: string | null;
env_plugin_installed_all?: string | null;
- env_shopify_variables?: string | null;
}, "store_", never>>, onError?: MetadataErrorHandling) => Promise<void>, runWithTimer: (field: NumericKeyOf<CmdFieldsFromMonorail>) => <T>(fn: () => Promise<T>) => Promise<T>;
export type Public = PublicSchema<typeof coreData>;
export type Sensitive = SensitiveSchema<typeof coreData>;
packages/cli-kit/dist/public/node/monorail.d.ts@@ -2,7 +2,7 @@ import { JsonMap } from '../../private/common/json.js';
import { DeepRequired } from '../common/ts/deep-required.js';
export { DeepRequired };
type Optional<T> = T | null;
-export declare const MONORAIL_COMMAND_TOPIC = "app_cli3_command/1.20";
+export declare const MONORAIL_COMMAND_TOPIC = "app_cli3_command/1.19";
export interface Schemas {
[MONORAIL_COMMAND_TOPIC]: {
sensitive: {
@@ -14,7 +14,6 @@ export interface Schemas {
cmd_all_environment_flags?: Optional<string>;
cmd_dev_tunnel_custom?: Optional<string>;
env_plugin_installed_all?: Optional<string>;
- env_shopify_variables?: Optional<string>;
};
public: {
business_platform_id?: Optional<number>;
packages/cli-kit/dist/cli/api/graphql/admin/generated/theme_files_upsert.d.ts@@ -8,7 +8,6 @@ export type ThemeFilesUpsertMutation = {
themeFilesUpsert?: {
upsertedThemeFiles?: {
filename: string;
- checksumMd5?: string | null;
}[] | null;
userErrors: {
filename?: string | null;
|
|
@alfonso-noriega I think @gonzaloriestra is right, because this method is also used during deploy so we need to handle errors better |
5e2a2ce to
d724888
Compare
54bda56 to
ef4a528
Compare
ef4a528 to
b49c429
Compare
d724888 to
268feef
Compare
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |

WHY are these changes introduced?
Fixes an issue where the extension bundling process would fail completely if any single file couldn't be copied, preventing developers from continuing their work.
WHAT is this pull request doing?
Improves error handling during extension bundling by:
Promise.allSettled()instead ofPromise.all()to prevent the entire process from failing if a single file copy failsHow to test your changes?
shopify app devMeasuring impact
How do we know this change was effective? Please choose one:
Checklist