Skip to content

Commit 585c0f3

Browse files
committed
Preserve startup rebuild results before watcher readiness
1 parent 86775f2 commit 585c0f3

4 files changed

Lines changed: 203 additions & 3 deletions

File tree

‎.changeset/app-dev-watcher-startup.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'@shopify/cli': patch
33
---
44

5-
Wait for file watcher readiness in app dev and stop startup when cancelled.
5+
Wait for file watcher readiness and startup rebuilds in app dev, report their latest results, and stop startup when cancelled.
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
import {AppEventWatcher, type AppEvent} from './app-event-watcher.js'
2+
import {testAppLinked, testFunctionExtension} from '../../../models/app/app.test-data.js'
3+
import {type ExtensionInstance} from '../../../models/extensions/extension-instance.js'
4+
import {AbortController} from '@shopify/cli-kit/node/abort'
5+
import {inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
6+
import {joinPath} from '@shopify/cli-kit/node/path'
7+
import {flushPromises} from '@shopify/cli-kit/node/promises'
8+
import {expect, test, vi, type Mock, type MockInstance} from 'vitest'
9+
import chokidar, {type FSWatcher} from 'chokidar'
10+
import {Writable} from 'stream'
11+
12+
const realChokidarWatch = chokidar.watch.bind(chokidar)
13+
14+
function output() {
15+
return new Writable({
16+
write(_chunk, _encoding, callback) {
17+
callback()
18+
},
19+
})
20+
}
21+
22+
async function withStartupWatcher(
23+
run: (fixture: {
24+
build: MockInstance<ExtensionInstance['buildForBundle']>
25+
onStart: Mock<(event: AppEvent) => void>
26+
scanned: Promise<void>
27+
releaseReadiness: () => void
28+
changed: () => Promise<void>
29+
running: Promise<void>
30+
}) => Promise<void>,
31+
initialBuildFails = false,
32+
) {
33+
await inTemporaryDirectory(async (directory) => {
34+
const extensionDirectory = joinPath(directory, 'extensions', 'function')
35+
await mkdir(extensionDirectory)
36+
const sourcePath = joinPath(extensionDirectory, 'index.js')
37+
await writeFile(sourcePath, 'initial')
38+
const extension = await testFunctionExtension({dir: extensionDirectory})
39+
vi.spyOn(extension, 'watchedFiles').mockReturnValue([sourcePath])
40+
vi.spyOn(extension, 'rescanImports').mockResolvedValue(false)
41+
const build = vi.spyOn(extension, 'buildForBundle').mockResolvedValue(undefined)
42+
if (initialBuildFails) build.mockRejectedValueOnce(new Error('Initial build failed'))
43+
const app = testAppLinked({directory, allExtensions: [extension]})
44+
vi.spyOn(app, 'generateExtensionTypes').mockResolvedValue(undefined)
45+
const controller = new AbortController()
46+
let nativeWatcher: FSWatcher | undefined
47+
let releaseReadiness!: () => void
48+
let scanned!: () => void
49+
const nativeReady = new Promise<void>((resolve) => {
50+
scanned = resolve
51+
})
52+
const watchSpy = vi.spyOn(chokidar, 'watch').mockImplementation((paths, options) => {
53+
nativeWatcher = realChokidarWatch(paths, options)
54+
nativeWatcher.once('ready', scanned)
55+
const once = nativeWatcher.once.bind(nativeWatcher)
56+
vi.spyOn(nativeWatcher, 'once').mockImplementation((event, listener) => {
57+
if (event === 'ready') {
58+
let pending = true
59+
releaseReadiness = () => {
60+
if (!pending) return
61+
pending = false
62+
listener()
63+
}
64+
return nativeWatcher!
65+
}
66+
return once(event, listener)
67+
})
68+
return nativeWatcher
69+
})
70+
const watcher = new AppEventWatcher(app)
71+
const onStart = vi.fn<(event: AppEvent) => void>()
72+
watcher.onStart(onStart)
73+
const running = watcher.start({stdout: output(), stderr: output(), signal: controller.signal})
74+
try {
75+
await run({
76+
build,
77+
onStart,
78+
scanned: nativeReady,
79+
running,
80+
releaseReadiness: () => releaseReadiness(),
81+
changed: () => {
82+
const changed = new Promise<void>((resolve) => watcher.once('all', () => resolve()))
83+
nativeWatcher!.emit('all', 'change', sourcePath)
84+
return changed
85+
},
86+
})
87+
} finally {
88+
controller.abort()
89+
releaseReadiness?.()
90+
await running
91+
await nativeWatcher?.close()
92+
watchSpy.mockRestore()
93+
}
94+
})
95+
}
96+
97+
test('startup readiness contains a build failure that occurs during the initial scan', async () => {
98+
await withStartupWatcher(async ({build, onStart, scanned, releaseReadiness, changed, running}) => {
99+
await scanned
100+
build.mockRejectedValueOnce(new Error('Compilation failed'))
101+
await changed()
102+
expect(onStart).not.toHaveBeenCalled()
103+
releaseReadiness()
104+
await running
105+
expect(onStart).toHaveBeenCalledOnce()
106+
expect(onStart.mock.calls[0]![0].extensionEvents[0]?.buildResult).toMatchObject({
107+
status: 'error',
108+
error: 'Compilation failed',
109+
})
110+
})
111+
})
112+
113+
test('waits for a startup rebuild that is still running when the initial scan completes', async () => {
114+
await withStartupWatcher(async ({build, onStart, scanned, releaseReadiness, changed, running}) => {
115+
await scanned
116+
let finishBuild!: () => void
117+
build.mockImplementationOnce(
118+
() =>
119+
new Promise<void>((resolve) => {
120+
finishBuild = resolve
121+
}),
122+
)
123+
const change = changed()
124+
try {
125+
await vi.waitFor(() => expect(build).toHaveBeenCalledTimes(2))
126+
releaseReadiness()
127+
await flushPromises()
128+
expect(onStart).not.toHaveBeenCalled()
129+
finishBuild()
130+
await change
131+
await running
132+
expect(onStart).toHaveBeenCalledOnce()
133+
expect(onStart.mock.calls[0]![0].extensionEvents[0]?.buildResult?.status).toBe('ok')
134+
} finally {
135+
finishBuild?.()
136+
}
137+
})
138+
})
139+
140+
test('replaces an initial build failure with a successful startup rebuild', async () => {
141+
await withStartupWatcher(async ({onStart, scanned, releaseReadiness, changed, running}) => {
142+
await scanned
143+
await changed()
144+
releaseReadiness()
145+
await running
146+
expect(onStart.mock.calls[0]![0].extensionEvents[0]?.buildResult?.status).toBe('ok')
147+
}, true)
148+
})
149+
150+
test('waits for new file events received while a startup rebuild is running', async () => {
151+
await withStartupWatcher(async ({build, onStart, scanned, releaseReadiness, changed, running}) => {
152+
await scanned
153+
let finishFirstBuild!: () => void
154+
let finishSecondBuild!: () => void
155+
build.mockImplementationOnce(
156+
() =>
157+
new Promise<void>((resolve) => {
158+
finishFirstBuild = resolve
159+
}),
160+
)
161+
build.mockImplementationOnce(
162+
() =>
163+
new Promise<void>((resolve) => {
164+
finishSecondBuild = resolve
165+
}),
166+
)
167+
const firstChange = changed()
168+
try {
169+
await vi.waitFor(() => expect(build).toHaveBeenCalledTimes(2))
170+
releaseReadiness()
171+
await flushPromises()
172+
const secondChange = changed()
173+
await vi.waitFor(() => expect(build).toHaveBeenCalledTimes(3))
174+
finishFirstBuild()
175+
await firstChange
176+
await flushPromises()
177+
expect(onStart).not.toHaveBeenCalled()
178+
finishSecondBuild()
179+
await secondChange
180+
await running
181+
expect(onStart).toHaveBeenCalledOnce()
182+
} finally {
183+
finishFirstBuild?.()
184+
finishSecondBuild?.()
185+
}
186+
})
187+
})

‎packages/app/src/cli/services/dev/app-events/app-event-watcher.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ export class AppEventWatcher extends EventEmitter {
9797
private ready = false
9898
private initialEvents: ExtensionEvent[] = []
9999
private fileWatcher?: FileWatcher
100+
private readonly pendingEvents = new Set<Promise<void>>()
100101

101102
constructor(app: AppLinkedInterface, appURL?: string, buildOutputPath?: string, fileWatcher?: FileWatcher) {
102103
super()
@@ -126,7 +127,7 @@ export class AppEventWatcher extends EventEmitter {
126127

127128
this.fileWatcher = this.fileWatcher ?? new FileWatcher(this.app, this.options)
128129
this.fileWatcher.onChange((events) => {
129-
handleWatcherEvents(events, this.app, this.options)
130+
const pending = handleWatcherEvents(events, this.app, this.options)
130131
.then(async (appEvent) => {
131132
if (appEvent?.extensionEvents.length === 0) outputDebug('Change detected, but no extensions were affected')
132133
if (!appEvent) return
@@ -149,13 +150,25 @@ export class AppEventWatcher extends EventEmitter {
149150

150151
// Find deleted extensions and delete their previous build output
151152
await this.deleteExtensionsBuildOutput(appEvent)
153+
if (buildExtensionsFirst && !this.ready) {
154+
const initialEventsByUid = new Map(this.initialEvents.map((event) => [event.extension.uid, event]))
155+
for (const event of appEvent.extensionEvents) initialEventsByUid.set(event.extension.uid, event)
156+
this.initialEvents = Array.from(initialEventsByUid.values())
157+
}
152158
this.emit('all', appEvent)
153159
})
154160
.catch((error) => {
155161
this.emit('error', error)
156162
})
163+
.finally(() => this.pendingEvents.delete(pending))
164+
this.pendingEvents.add(pending)
157165
})
158166
await this.fileWatcher.start()
167+
while (!this.options.signal.aborted && this.pendingEvents.size > 0) {
168+
// Startup builds can trigger more file events before they finish.
169+
// eslint-disable-next-line no-await-in-loop
170+
await Promise.all(this.pendingEvents)
171+
}
159172
if (this.options.signal.aborted) return
160173

161174
this.ready = true

‎packages/app/src/cli/services/function/replay/watch.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ test('formats watch events as JSONL and keeps diagnostics on stderr', async () =
5959
})
6060
const resultJson = String.raw`{
6161
"name": "function.wasm", "size": 1, "memory_usage": 64,
62-
"instructions": 9007199254740993, "logs": "hello world \"quoted\" done\nsecond",
62+
"instructions": 9007199254740993, "logs": "hello world \"quoted words\" done\nsecond",
6363
"input": {}, "output": null, "success": false
6464
}`
6565
vi.mocked(watchReplay).mockImplementation(async ({onEvent}) => {

0 commit comments

Comments
 (0)