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

App watcher - Build extensions #4346

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
3 changes: 0 additions & 3 deletions packages/app/src/cli/commands/app/demo/watcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ export default class DemoWatcher extends Command {
case EventType.Updated:
outputInfo(` 🔄 Updated: ${colors.yellow(event.extension.handle)}`)
break
case EventType.UpdatedSourceFile:
outputInfo(` 🔄 Updated: ${colors.yellow(event.extension.handle)} (🏗️ needs rebuild)`)
break
}
})
})
Expand Down
5 changes: 4 additions & 1 deletion packages/app/src/cli/models/app/app.test-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,10 @@ export async function testUIExtension(
sources: [],
},
},
targeting: [{target: 'target1'}, {target: 'target2'}],
extension_points: [
{target: 'target1', module: 'module1'},
{target: 'target2', module: 'module2'},
],
}
const configurationPath = uiExtension?.configurationPath ?? `${directory}/shopify.ui.extension.toml`
const entryPath = uiExtension?.entrySourceFilePath ?? `${directory}/src/index.js`
Expand Down
15 changes: 12 additions & 3 deletions packages/app/src/cli/models/extensions/extension-instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,12 +330,11 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
}

async buildForBundle(options: ExtensionBuildOptions, bundleDirectory: string, identifiers?: Identifiers) {
const extensionId = identifiers?.extensions[this.localIdentifier] ?? this.configuration.uid ?? this.handle
const outputFile = this.isThemeExtension ? '' : joinPath('dist', `${this.outputFileName}`)
const extensionId = this.getOutputFolderId(identifiers?.extensions[this.localIdentifier])

if (this.features.includes('bundling')) {
// Modules that are going to be inclued in the bundle should be built in the bundle directory
this.outputPath = joinPath(bundleDirectory, extensionId, outputFile)
this.outputPath = this.getOutputPathForDirectory(bundleDirectory, extensionId)
}

await this.build(options)
Expand All @@ -345,6 +344,16 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
}
}

getOutputFolderId(extensionId?: string) {
return extensionId ?? this.configuration.uid ?? this.handle
}

getOutputPathForDirectory(directory: string, extensionId?: string) {
const id = this.getOutputFolderId(extensionId)
const outputFile = this.isThemeExtension ? '' : joinPath('dist', `${this.outputFileName}`)
return joinPath(directory, id, outputFile)
}

get singleTarget() {
const targets = (getPathValue(this.configuration, 'targeting') as {target: string}[]) ?? []
if (targets.length !== 1) return undefined
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {AppInterface} from '../../../models/app/app.js'
import {ExtensionInstance} from '../../../models/extensions/extension-instance.js'

export interface AppExtensionsDiff {
interface AppExtensionsDiff {
created: ExtensionInstance[]
updated: ExtensionInstance[]
deleted: ExtensionInstance[]
Expand Down
106 changes: 61 additions & 45 deletions packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ import {loadApp} from '../../../models/app/loader.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'

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

// Extensions 1 and 1B simulate extensions defined in the same directory (same toml)
const extension1 = await testUIExtension({type: 'ui_extension', handle: 'h1', directory: '/extensions/ui_extension_1'})
Expand Down Expand Up @@ -111,7 +114,7 @@ const testCases: TestCase[] = [
},
initialExtensions: [extension1, extension2, posExtension],
finalExtensions: [extension1, extension2, posExtension],
extensionEvents: [{type: EventType.UpdatedSourceFile, extension: extension1}],
extensionEvents: [{type: EventType.Updated, extension: extension1}],
},
{
name: 'file_updated affecting a single extension',
Expand Down Expand Up @@ -148,8 +151,8 @@ const testCases: TestCase[] = [
initialExtensions: [extension1, extension1B, extension2, posExtension],
finalExtensions: [extension1, extension1B, extension2, posExtension],
extensionEvents: [
{type: EventType.UpdatedSourceFile, extension: extension1},
{type: EventType.UpdatedSourceFile, extension: extension1B},
{type: EventType.Updated, extension: extension1},
{type: EventType.Updated, extension: extension1B},
],
},
{
Expand All @@ -163,8 +166,8 @@ const testCases: TestCase[] = [
initialExtensions: [extension1, extension1B, extension2, posExtension],
finalExtensions: [extension1, extension1B, extension2, posExtension],
extensionEvents: [
{type: EventType.UpdatedSourceFile, extension: extension1},
{type: EventType.UpdatedSourceFile, extension: extension1B},
{type: EventType.Updated, extension: extension1},
{type: EventType.Updated, extension: extension1B},
],
},
{
Expand All @@ -178,8 +181,8 @@ const testCases: TestCase[] = [
initialExtensions: [extension1, extension1B, extension2, posExtension],
finalExtensions: [extension1, extension1B, extension2, posExtension],
extensionEvents: [
{type: EventType.UpdatedSourceFile, extension: extension1},
{type: EventType.UpdatedSourceFile, extension: extension1B},
{type: EventType.Updated, extension: extension1},
{type: EventType.Updated, extension: extension1B},
],
},
{
Expand All @@ -193,7 +196,7 @@ const testCases: TestCase[] = [
initialExtensions: [extension1, extension2, posExtension, webhookExtension],
finalExtensions: [extension1, extension2, posExtensionUpdated, appAccessExtension],
extensionEvents: [
{type: EventType.UpdatedSourceFile, extension: posExtensionUpdated},
{type: EventType.Updated, extension: posExtensionUpdated},
{type: EventType.Deleted, extension: webhookExtension},
{type: EventType.Created, extension: appAccessExtension},
],
Expand All @@ -210,8 +213,8 @@ const testCases: TestCase[] = [
initialExtensions: [extension1, extension1B, extension2],
finalExtensions: [extension1Updated, extension1BUpdated, extension2],
extensionEvents: [
{type: EventType.UpdatedSourceFile, extension: extension1Updated},
{type: EventType.UpdatedSourceFile, extension: extension1BUpdated},
{type: EventType.Updated, extension: extension1Updated},
{type: EventType.Updated, extension: extension1BUpdated},
],
needsAppReload: true,
},
Expand All @@ -222,46 +225,59 @@ describe('app-event-watcher when receiving a file event that doesnt require an a
'The event $name returns the expected AppEvent',
async ({fileWatchEvent, initialExtensions, finalExtensions, extensionEvents, needsAppReload}) => {
// Given
vi.mocked(loadApp).mockResolvedValue(testApp({allExtensions: finalExtensions}))
vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange(fileWatchEvent))
await inTemporaryDirectory(async (tmpDir) => {
vi.mocked(loadApp).mockResolvedValue(testApp({allExtensions: finalExtensions}))
vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange(fileWatchEvent))

// When
const app = testApp({
allExtensions: initialExtensions,
configuration: {scopes: '', extension_directories: [], path: 'shopify.app.custom.toml'},
})
const watcher = new AppEventWatcher(app, outputOptions)
const emitSpy = vi.spyOn(watcher, 'emit')
await watcher.start()
const buildOutputPath = joinPath(tmpDir, '.shopify', 'bundle')

// When
const app = testApp({
allExtensions: initialExtensions,
configuration: {scopes: '', extension_directories: [], path: 'shopify.app.custom.toml'},
})

await flushPromises()
const watcher = new AppEventWatcher(app, 'url', outputOptions, buildOutputPath)
const emitSpy = vi.spyOn(watcher, 'emit')
await watcher.start()

expect(emitSpy).toHaveBeenCalledWith('all', {
app: expect.objectContaining({realExtensions: finalExtensions}),
extensionEvents: expect.arrayContaining(extensionEvents),
startTime: expect.anything(),
path: expect.anything(),
})
await flushPromises()

if (needsAppReload) {
expect(loadApp).toHaveBeenCalledWith({
specifications: expect.anything(),
directory: expect.anything(),
// The app is loaded with the same configuration file
userProvidedConfigName: 'shopify.app.custom.toml',
remoteFlags: expect.anything(),
// Wait until emitSpy has been called at least once
// We need this because there are i/o operations that make the test finish before the event is emitted
await new Promise<void>((resolve, reject) => {
const interval = setInterval(() => {
if (emitSpy.mock.calls.length > 0) {
clearInterval(interval)
resolve()
}
}, 100)
// Wait max 3 seconds, if not resolved, reject.
setTimeout(() => {
clearInterval(interval)
reject(new Error('Timeout waiting for emitSpy to be called'))
}, 3000)
Comment on lines +249 to +259
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this kind of thing would be a bit easier to read:

Suggested change
const interval = setInterval(() => {
if (emitSpy.mock.calls.length > 0) {
clearInterval(interval)
resolve()
}
}, 100)
// Wait max 3 seconds, if not resolved, reject.
setTimeout(() => {
clearInterval(interval)
reject(new Error('Timeout waiting for emitSpy to be called'))
}, 3000)
const initialTime = Date.now()
while (emitSpy.mock.calls.length === 0) {
if (Date.now() - initialTime < 3000) {
await sleep(100)
} else {
reject(new Error('Timeout waiting for emitSpy to be called'))
}
}
resolve()

Note we might have something about not awaiting in loops 😢

})
} else {
expect(loadApp).not.toHaveBeenCalled()
}

expect(emitSpy).toHaveBeenCalledWith('all', {
app: expect.objectContaining({realExtensions: finalExtensions}),
extensionEvents: expect.arrayContaining(extensionEvents),
startTime: expect.anything(),
path: expect.anything(),
})

if (needsAppReload) {
expect(loadApp).toHaveBeenCalledWith({
specifications: expect.anything(),
directory: expect.anything(),
// The app is loaded with the same configuration file
userProvidedConfigName: 'shopify.app.custom.toml',
remoteFlags: expect.anything(),
})
} else {
expect(loadApp).not.toHaveBeenCalled()
}
})
},
)
})

async function waitForEvent(watcher: AppEventWatcher) {
return new Promise((resolve) => {
watcher.onEvent((event) => {
resolve(event)
})
})
}
Loading
Loading