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

Functions 7452: Bundling and uploading wasm file to GCS for function extension #4445

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 7 additions & 4 deletions packages/app/src/cli/models/extensions/extension-instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ import {constantize, slugify} from '@shopify/cli-kit/common/string'
import {hashString, randomUUID} from '@shopify/cli-kit/node/crypto'
import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn'
import {joinPath} from '@shopify/cli-kit/node/path'
import {useThemebundling} from '@shopify/cli-kit/node/context/local'
import {fileExists, touchFile, writeFile} from '@shopify/cli-kit/node/fs'
import {fileExists, touchFile, writeFile, copyFile} from '@shopify/cli-kit/node/fs'
import {getPathValue} from '@shopify/cli-kit/common/object'
import {useThemebundling} from '@shopify/cli-kit/node/context/local'

export const CONFIG_EXTENSION_IDS = [
AppAccessSpecIdentifier,
Expand Down Expand Up @@ -205,6 +205,7 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
apiKey,
developerPlatformClient,
}: ExtensionDeployConfigOptions): Promise<{[key: string]: unknown} | undefined> {
// To ensure this is not a breaking change we will delete this upload line on a second pass after we've ensured the new file upload process from core is working.
saga-dasgupta marked this conversation as resolved.
Show resolved Hide resolved
const {moduleId} = await uploadWasmBlob(this.localIdentifier, this.outputPath, developerPlatformClient)
return this.specification.deployConfig?.(this.configuration, this.directory, apiKey, moduleId)
}
Expand Down Expand Up @@ -335,11 +336,13 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi

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.isFunctionExtension ? this.outputPath : joinPath(bundleDirectory, extensionId, outputFile)
saga-dasgupta marked this conversation as resolved.
Show resolved Hide resolved
}

await this.build(options)

if (this.isFunctionExtension) {
await copyFile(this.outputPath, joinPath(bundleDirectory, extensionId, 'index.wasm'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying the wasm file generated from the function's dist directory to the bundleDirectory for it to get zipped and uploaded later.

}
if (this.isThemeExtension && useThemebundling()) {
await bundleThemeExtension(this, options)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const functionSpec = createExtensionSpecification({
'pickup_point_delivery_option_generator',
],
schema: FunctionExtensionSchema,
appModuleFeatures: (_) => ['function'],
appModuleFeatures: (_) => ['function', 'bundling'],
deployConfig: async (config, directory, apiKey, moduleId) => {
let inputQuery: string | undefined
const inputQueryPath = joinPath(directory, 'input.graphql')
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ describe('deploy', () => {
expect(updateAppIdentifiers).toHaveBeenCalledOnce()
})

test('uploads the extension bundle with 1 function', async () => {
test('uploads the extension bundle with 1 function extension', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the test names consistent.

// Given
const functionExtension = await testFunctionExtension()
vi.spyOn(functionExtension, 'preDeployValidation').mockImplementation(async () => {})
Expand Down Expand Up @@ -290,7 +290,7 @@ describe('deploy', () => {
],
developerPlatformClient,
extensionIds: {},
bundlePath: undefined,
bundlePath: expect.stringMatching(/bundle.zip$/),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bundle path now has a zip file for functions' assets.

release: true,
})
expect(bundleAndBuildExtensions).toHaveBeenCalledOnce()
Expand Down
33 changes: 32 additions & 1 deletion packages/app/src/cli/services/deploy/bundle.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {bundleAndBuildExtensions} from './bundle.js'
import {testApp, testThemeExtensions, testUIExtension} from '../../models/app/app.test-data.js'
import {testApp, testFunctionExtension, testThemeExtensions, testUIExtension} from '../../models/app/app.test-data.js'
import {AppInterface} from '../../models/app/app.js'
import {describe, expect, test, vi} from 'vitest'
import * as file from '@shopify/cli-kit/node/fs'
Expand Down Expand Up @@ -107,4 +107,35 @@ describe('bundleAndBuildExtensions', () => {
await expect(file.fileExists(bundlePath)).resolves.toBeTruthy()
})
})

test('creates a zip file with wasm file in it for a function extension', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not really testing that the zip file includes a wasm file no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I really tried to unzip it and assert on the contents but would need to install some libraries for unzipping so I let it be. I did do end to end testing so I think just asserting a zip got generated here is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is a good way to unzip files in cli? I didn't find any prior art for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to remove reference to wasm file in the test title.

await file.inTemporaryDirectory(async (tmpDir: string) => {
// Given
const bundlePath = joinPath(tmpDir, 'bundle.zip')

const functionExtension = await testFunctionExtension()
const extensionBundleMock = vi.fn().mockImplementation(async (options, bundleDirectory, identifiers) => {
file.writeFileSync(joinPath(bundleDirectory, 'index.wasm'), '')
})
functionExtension.buildForBundle = extensionBundleMock
const app = testApp({allExtensions: [functionExtension]})

const extensions: {[key: string]: string} = {}
for (const extension of app.allExtensions) {
extensions[extension.localIdentifier] = extension.localIdentifier
}
const identifiers = {
app: 'app-id',
extensions,
extensionIds: {},
extensionsNonUuidManaged: {},
}

// When
await bundleAndBuildExtensions({app, identifiers, bundlePath}, {})

// Then
await expect(file.fileExists(bundlePath)).resolves.toBeTruthy()
})
})
})
Loading