Skip to content

[Refactor] Fix incorrect Promise.all usage in app preDeployValidation #7540

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.

Sign up for GitHub

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

Open
gonzaloriestra wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −1
Open

[Refactor] Fix incorrect Promise.all usage in app preDeployValidation #7540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/app/src/cli/models/app/app.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,3 +851,21 @@ function createPackageJson(tmpDir: string, type: string, version: string) {
const dirPath = joinPath(tmpDir, 'node_modules', '@shopify', type)
return mkdir(dirPath).then(() => writeFile(packagePath, JSON.stringify(packageJson)))
}

describe('preDeployValidation awaiting', () => {
test('awaits extension validation and catches rejections', async () => {
Copy link
Copy Markdown
Contributor

@craigmichaelmartin craigmichaelmartin May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Suggestion: The regression test proves a rejection is caught for one extension, but the original bug specifically affected Promise.all with multiple promises. With a single extension, the behavioral difference between Promise.all([promise]) and Promise.all([[promise]]) is less discriminating. A test with two or more extensions — where one succeeds and one rejects — would more directly demonstrate the fix works for the N > 1 case and guard against future regressions.

Suggestion: Consider adding a second test case with multiple extensions (e.g., one that resolves and one that rejects) to verify Promise.all correctly awaits all of them. This more directly exercises the array-wrapping bug that was fixed.

// Given
const extension = await testUIExtension()
vi.spyOn(extension, 'preDeployValidation').mockImplementation(async () => {
await new Promise((resolve) => setTimeout(resolve, 10))
Copy link
Copy Markdown
Contributor

@craigmichaelmartin craigmichaelmartin May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Suggestion: The test uses setTimeout(resolve, 10) to simulate async delay. While 10ms is short, real timers in tests can be a source of flakiness under CI load. More importantly, the regression this test guards against is structural (array wrapping in Promise.all), not timing-related — any async rejection triggers it regardless of delay.

Suggestion: The mock can be simplified to remove the artificial delay:

Suggested change
await new Promise((resolve) => setTimeout(resolve, 10))
vi.spyOn(extension, 'preDeployValidation').mockRejectedValue(new Error('Validation failed'))

throw new Error('Validation failed')
})

const app = testApp({
allExtensions: [extension],
})

// When / Then
await expect(app.preDeployValidation()).rejects.toThrow('Validation failed')
})
})
2 changes: 1 addition & 1 deletion packages/app/src/cli/models/app/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ export class App<
}
}

await Promise.all([this.allExtensions.map((ext) => ext.preDeployValidation())])
await Promise.all(this.allExtensions.map((ext) => ext.preDeployValidation()))
}

extensionsForType(specification: {identifier: string; externalIdentifier: string}): ExtensionInstance[] {
Expand Down
Loading