Skip to content
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

Refactor FileWatcher into a Class #4916

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {AppEvent, AppEventWatcher, EventType, ExtensionEvent} from './app-event-watcher.js'
import {OutputContextOptions, WatcherEvent, startFileWatcher} from './file-watcher.js'
import {OutputContextOptions, WatcherEvent, FileWatcher} from './file-watcher.js'
import {ESBuildContextManager} from './app-watcher-esbuild.js'
import {
testAppAccessConfigExtension,
Expand All @@ -11,14 +11,14 @@ import {
} from '../../../models/app/app.test-data.js'
import {ExtensionInstance} from '../../../models/extensions/extension-instance.js'
import {loadApp, reloadApp} from '../../../models/app/loader.js'
import {AppInterface} from '../../../models/app/app.js'
import {describe, expect, test, vi} from 'vitest'
import {AbortSignal} from '@shopify/cli-kit/node/abort'
import {flushPromises} from '@shopify/cli-kit/node/promises'
import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'
import {Writable} from 'stream'

vi.mock('./file-watcher.js')
vi.mock('../../../models/app/loader.js')
vi.mock('./app-watcher-esbuild.js')

Expand Down Expand Up @@ -235,7 +235,6 @@ describe('app-event-watcher when receiving a file event', () => {
const mockedApp = testAppLinked({allExtensions: finalExtensions})
vi.mocked(loadApp).mockResolvedValue(mockedApp)
vi.mocked(reloadApp).mockResolvedValue(mockedApp)
vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange([fileWatchEvent]))

const buildOutputPath = joinPath(tmpDir, '.shopify', 'bundle')

Expand All @@ -245,7 +244,9 @@ describe('app-event-watcher when receiving a file event', () => {
configuration: {scopes: '', extension_directories: [], path: 'shopify.app.custom.toml'},
})

const watcher = new AppEventWatcher(app, 'url', buildOutputPath, new MockESBuildContextManager())
const mockManager = new MockESBuildContextManager()
const mockFileWatcher = new MockFileWatcher(app, outputOptions, [fileWatchEvent])
const watcher = new AppEventWatcher(app, 'url', buildOutputPath, mockManager, mockFileWatcher)
const emitSpy = vi.spyOn(watcher, 'emit')
await watcher.start()

Expand Down Expand Up @@ -305,7 +306,6 @@ describe('app-event-watcher build extension errors', () => {
extensionPath: '/extensions/ui_extension_1',
startTime: [0, 0],
}
vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange([fileWatchEvent]))

// Given
const esbuildError = {
Expand All @@ -325,9 +325,10 @@ describe('app-event-watcher build extension errors', () => {
allExtensions: [extension1],
configuration: {scopes: '', extension_directories: [], path: 'shopify.app.custom.toml'},
})
const mockFileWatcher = new MockFileWatcher(app, outputOptions, [fileWatchEvent])

// When
const watcher = new AppEventWatcher(app, 'url', buildOutputPath, mockManager)
const watcher = new AppEventWatcher(app, 'url', buildOutputPath, mockManager, mockFileWatcher)
const stderr = {write: vi.fn()} as unknown as Writable
const stdout = {write: vi.fn()} as unknown as Writable

Expand Down Expand Up @@ -358,7 +359,6 @@ describe('app-event-watcher build extension errors', () => {
extensionPath: '/extensions/flow_action',
startTime: [0, 0],
}
vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange([fileWatchEvent]))

// Given
const esbuildError = {message: 'Build failed'}
Expand All @@ -371,7 +371,9 @@ describe('app-event-watcher build extension errors', () => {
})

// When
const watcher = new AppEventWatcher(app, 'url', buildOutputPath, new MockESBuildContextManager())
const mockManager = new MockESBuildContextManager()
const mockFileWatcher = new MockFileWatcher(app, outputOptions, [fileWatchEvent])
const watcher = new AppEventWatcher(app, 'url', buildOutputPath, mockManager, mockFileWatcher)
const stderr = {write: vi.fn()} as unknown as Writable
const stdout = {write: vi.fn()} as unknown as Writable

Expand Down Expand Up @@ -403,3 +405,25 @@ class MockESBuildContextManager extends ESBuildContextManager {
async updateContexts(appEvent: AppEvent) {}
async deleteContexts(extensions: ExtensionInstance[]) {}
}

// Mock class for FileWatcher
// Used to trigger mocked file system events immediately after the watcher is started.
class MockFileWatcher extends FileWatcher {
private readonly events: WatcherEvent[]
private listener?: (events: WatcherEvent[]) => void

constructor(app: AppInterface, options: OutputContextOptions, events: WatcherEvent[]) {
super(app, options)
this.events = events
}

async start(): Promise<void> {
if (this.listener) {
this.listener(this.events)
}
}

onChange(listener: (events: WatcherEvent[]) => void) {
this.listener = listener
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable tsdoc/syntax */
import {OutputContextOptions, startFileWatcher} from './file-watcher.js'
import {FileWatcher, OutputContextOptions} from './file-watcher.js'
import {ESBuildContextManager} from './app-watcher-esbuild.js'
import {handleWatcherEvents} from './app-event-watcher-handler.js'
import {AppLinkedInterface} from '../../../models/app/app.js'
Expand Down Expand Up @@ -96,12 +96,14 @@ export class AppEventWatcher extends EventEmitter {
private started = false
private ready = false
private initialEvents: ExtensionEvent[] = []
private fileWatcher?: FileWatcher

constructor(
app: AppLinkedInterface,
appURL?: string,
buildOutputPath?: string,
esbuildManager?: ESBuildContextManager,
fileWatcher?: FileWatcher,
) {
super()
this.app = app
Expand All @@ -117,6 +119,7 @@ export class AppEventWatcher extends EventEmitter {
url: this.appURL ?? '',
...this.options,
})
this.fileWatcher = fileWatcher
}

async start(options?: OutputContextOptions, buildExtensionsFirst = true) {
Expand All @@ -139,8 +142,8 @@ export class AppEventWatcher extends EventEmitter {
await this.buildExtensions(this.initialEvents)
}

// Start the file system watcher
await startFileWatcher(this.app, this.options, (events) => {
this.fileWatcher = this.fileWatcher ?? new FileWatcher(this.app, this.options)
this.fileWatcher.onChange((events) => {
handleWatcherEvents(events, this.app, this.options)
.then(async (appEvent) => {
if (appEvent?.extensionEvents.length === 0) outputDebug('Change detected, but no extensions were affected')
Expand All @@ -163,6 +166,7 @@ export class AppEventWatcher extends EventEmitter {
this.options.stderr.write(`Error handling event: ${error.message}`)
})
})
await this.fileWatcher.start()

this.ready = true
this.emit('ready', {app: this.app, extensionEvents: this.initialEvents})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {OutputContextOptions, WatcherEvent, startFileWatcher} from './file-watcher.js'
import {FileWatcher, OutputContextOptions, WatcherEvent} from './file-watcher.js'
import {
testApp,
testAppAccessConfigExtension,
Expand Down Expand Up @@ -198,7 +198,10 @@ describe('file-watcher events', () => {
})

// When
await startFileWatcher(app, outputOptions, vi.fn())
const fileWatcher = new FileWatcher(app, outputOptions)
fileWatcher.onChange(vi.fn())

await fileWatcher.start()

// Then
expect(watchSpy).toHaveBeenCalledWith([joinPath(dir, '/shopify.app.toml'), joinPath(dir, '/extensions')], {
Expand Down Expand Up @@ -232,7 +235,10 @@ describe('file-watcher events', () => {

// When
const onChange = vi.fn()
await startFileWatcher(defaultApp, outputOptions, onChange)
const fileWatcher = new FileWatcher(defaultApp, outputOptions)
fileWatcher.onChange(onChange)

await fileWatcher.start()

// Then
await flushPromises()
Expand Down Expand Up @@ -260,7 +266,10 @@ describe('file-watcher events', () => {

// When
const onChange = vi.fn()
await startFileWatcher(defaultApp, outputOptions, onChange)
const fileWatcher = new FileWatcher(defaultApp, outputOptions)
fileWatcher.onChange(onChange)

await fileWatcher.start()

// Then
await flushPromises()
Expand Down
Loading
Loading