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

Download and use Javy plugin when building JS functions #4831

Merged
merged 9 commits into from
Nov 22, 2024
5 changes: 5 additions & 0 deletions .changeset/dry-rice-guess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/app': patch
---

Update Javy invocation to use Javy plugin
41 changes: 36 additions & 5 deletions packages/app/src/cli/services/function/binaries.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import {javyBinary, functionRunnerBinary, installBinary} from './binaries.js'
import {javyBinary, functionRunnerBinary, downloadBinary, javyPluginBinary} from './binaries.js'
import {fetch, Response} from '@shopify/cli-kit/node/http'
import {fileExists, removeFile} from '@shopify/cli-kit/node/fs'
import {describe, expect, test, vi} from 'vitest'
import {gzipSync} from 'zlib'

const javy = javyBinary()
const javyPlugin = javyPluginBinary()
const functionRunner = functionRunnerBinary()

vi.mock('@shopify/cli-kit/node/http', async () => {
Expand Down Expand Up @@ -117,21 +118,51 @@ describe('javy', () => {
})
})

test('installs Javy', async () => {
test('downloads Javy', async () => {
// Given
await removeFile(javy.path)
await expect(fileExists(javy.path)).resolves.toBeFalsy()
vi.mocked(fetch).mockResolvedValue(new Response(gzipSync('javy binary')))

// When
await installBinary(javy)
await downloadBinary(javy)

// Then
expect(fetch).toHaveBeenCalledOnce()
await expect(fileExists(javy.path)).resolves.toBeTruthy()
})
})

describe('javy-plugin', () => {
test('properties are set correctly', () => {
expect(javyPlugin.name).toBe('javy_quickjs_provider_v3')
expect(javyPlugin.version).match(/^v\d.\d.\d$/)
expect(javyPlugin.path).toMatch(/(\/|\\)javy_quickjs_provider_v3.wasm$/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should part of the version identifier here match the expected version? Or rather, some kind of "minimum version"

Copy link
Contributor Author

@jeffcharles jeffcharles Nov 15, 2024

Choose a reason for hiding this comment

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

There isn't really a concept of a minimum version for this. The version should generally be whatever the latest release of Javy is until we switch to the Shopify Functions plugin. But it shouldn't be set to the latest version if there are breaking changes in the latest version until the Shopify CLI code has been updated to not break when using that version. So essentially, the version should be whatever is set in the binaries.ts file.

I opted to go with a check on the format of the version rather than the version itself since the format of a version should be constant over time whereas the version should change frequently. If you have to update the test every time the version changes, it's arguably more likely the test won't catch an incorrect change since it'll just be updated to whatever is in binaries.ts.

})

test('downloadUrl returns the correct URL', () => {
// When
const url = javyPlugin.downloadUrl('', '')

// Then
expect(url).toMatch(/https:\/\/github.com\/bytecodealliance\/javy\/releases\/download\/v\d\.\d\.\d\/plugin.wasm.gz/)
})

test('downloads javy-plugin', async () => {
// Given
await removeFile(javyPlugin.path)
await expect(fileExists(javyPlugin.path)).resolves.toBeFalsy()
vi.mocked(fetch).mockResolvedValue(new Response(gzipSync('javy-plugin binary')))

// When
await downloadBinary(javyPlugin)

// Then
expect(fetch).toHaveBeenCalledOnce()
await expect(fileExists(javyPlugin.path)).resolves.toBeTruthy()
})
})

describe('functionRunner', () => {
test('properties are set correctly', () => {
expect(functionRunner.name).toBe('function-runner')
Expand Down Expand Up @@ -234,14 +265,14 @@ describe('functionRunner', () => {
})
})

test('installs function-runner', async () => {
test('downloads function-runner', async () => {
// Given
await removeFile(functionRunner.path)
await expect(fileExists(functionRunner.path)).resolves.toBeFalsy()
vi.mocked(fetch).mockResolvedValue(new Response(gzipSync('function-runner binary')))

// When
await installBinary(functionRunner)
await downloadBinary(functionRunner)

// Then
expect(fetch).toHaveBeenCalledOnce()
Expand Down
66 changes: 55 additions & 11 deletions packages/app/src/cli/services/function/binaries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,24 @@ import * as gzip from 'node:zlib'
import {fileURLToPath} from 'node:url'

const FUNCTION_RUNNER_VERSION = 'v6.3.0'
const JAVY_VERSION = 'v3.2.0'
const JAVY_VERSION = 'v4.0.0'
// The Javy plugin version does not need to match the Javy version. It should
// match the plugin version used in the function-runner version specified above.
const JAVY_PLUGIN_VERSION = 'v3.2.0'

interface DownloadableBinary {
path: string
name: string
version: string

downloadUrl(processPlatform: string, processArch: string): string
processResponse(responseStream: PipelineSource<unknown>, outputStream: fs.WriteStream): Promise<void>
}

// The logic for determining the download URL and what to do with the response stream is _coincidentally_ the same for
shauns marked this conversation as resolved.
Show resolved Hide resolved
// Javy and function-runner for now. Those methods may not continue to have the same logic in the future. If they
// diverge, make `Binary` an abstract class and create subclasses to handle the different logic polymorphically.
class DownloadableBinary {
// diverge, create different classes to handle the different logic polymorphically.
class Executable implements DownloadableBinary {
readonly name: string
readonly version: string
readonly path: string
Expand Down Expand Up @@ -71,31 +83,58 @@ class DownloadableBinary {
}

async processResponse(responseStream: PipelineSource<unknown>, outputStream: fs.WriteStream): Promise<void> {
const gunzip = gzip.createGunzip()
await stream.pipeline(responseStream, gunzip, outputStream)
return gunzipResponse(responseStream, outputStream)
}
}

class JavyPlugin implements DownloadableBinary {
readonly name: string
readonly version: string
readonly path: string

constructor() {
this.name = 'javy_quickjs_provider_v3'
this.version = JAVY_PLUGIN_VERSION
this.path = joinPath(dirname(fileURLToPath(import.meta.url)), '..', 'bin', 'javy_quickjs_provider_v3.wasm')
}

downloadUrl(_processPlatform: string, _processArch: string) {
return `https://github.com/bytecodealliance/javy/releases/download/${this.version}/plugin.wasm.gz`
jacobsteves marked this conversation as resolved.
Show resolved Hide resolved
}

async processResponse(responseStream: PipelineSource<unknown>, outputStream: fs.WriteStream): Promise<void> {
return gunzipResponse(responseStream, outputStream)
}
}

let _javy: DownloadableBinary
let _javyPlugin: DownloadableBinary
let _functionRunner: DownloadableBinary

export function javyBinary() {
if (!_javy) {
_javy = new DownloadableBinary('javy', JAVY_VERSION, 'bytecodealliance/javy')
_javy = new Executable('javy', JAVY_VERSION, 'bytecodealliance/javy')
}
return _javy
}

export function javyPluginBinary() {
if (!_javyPlugin) {
_javyPlugin = new JavyPlugin()
}
return _javyPlugin
}

export function functionRunnerBinary() {
if (!_functionRunner) {
_functionRunner = new DownloadableBinary('function-runner', FUNCTION_RUNNER_VERSION, 'Shopify/function-runner')
_functionRunner = new Executable('function-runner', FUNCTION_RUNNER_VERSION, 'Shopify/function-runner')
}
return _functionRunner
}

export async function installBinary(bin: DownloadableBinary) {
const isInstalled = await fileExists(bin.path)
if (isInstalled) {
export async function downloadBinary(bin: DownloadableBinary) {
const isDownloaded = await fileExists(bin.path)
if (isDownloaded) {
return
}

Expand All @@ -118,7 +157,7 @@ export async function installBinary(bin: DownloadableBinary) {
}

// Download to a temp location and then move the file only after it's fully processed
// so the `isInstalled` check above will continue to return false if the file hasn't
// so the `isDownloaded` check above will continue to return false if the file hasn't
// been fully processed.
await inTemporaryDirectory(async (tmpDir) => {
const tmpFile = joinPath(tmpDir, 'binary')
Expand All @@ -132,3 +171,8 @@ export async function installBinary(bin: DownloadableBinary) {
2,
)
}

async function gunzipResponse(responseStream: PipelineSource<unknown>, outputStream: fs.WriteStream): Promise<void> {
const gunzip = gzip.createGunzip()
await stream.pipeline(responseStream, gunzip, outputStream)
}
15 changes: 13 additions & 2 deletions packages/app/src/cli/services/function/build.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {buildGraphqlTypes, bundleExtension, runJavy, ExportJavyBuilder, jsExports} from './build.js'
import {javyBinary} from './binaries.js'
import {javyBinary, javyPluginBinary} from './binaries.js'
import {testApp, testFunctionExtension} from '../../models/app/app.test-data.js'
import {beforeEach, describe, expect, test, vi} from 'vitest'
import {exec} from '@shopify/cli-kit/node/system'
Expand Down Expand Up @@ -143,7 +143,16 @@ describe('runJavy', () => {
await expect(got).resolves.toBeUndefined()
expect(exec).toHaveBeenCalledWith(
javyBinary().path,
['build', '-C', 'dynamic', '-o', joinPath(ourFunction.directory, 'dist/index.wasm'), 'dist/function.js'],
[
'build',
'-C',
'dynamic',
'-C',
`plugin=${javyPluginBinary().path}`,
'-o',
joinPath(ourFunction.directory, 'dist/index.wasm'),
'dist/function.js',
],
{
cwd: ourFunction.directory,
stderr: 'inherit',
Expand Down Expand Up @@ -230,6 +239,8 @@ describe('ExportJavyBuilder', () => {
'-C',
'dynamic',
'-C',
`plugin=${javyPluginBinary().path}`,
'-C',
expect.stringContaining('wit='),
'-C',
'wit-world=shopify-function',
Expand Down
21 changes: 16 additions & 5 deletions packages/app/src/cli/services/function/build.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import {installBinary, javyBinary} from './binaries.js'
import {downloadBinary, javyBinary, javyPluginBinary} from './binaries.js'
import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
import {AppInterface} from '../../models/app/app.js'
Expand Down Expand Up @@ -168,12 +168,23 @@
extra: string[] = [],
) {
const javy = javyBinary()
await installBinary(javy)
const plugin = javyPluginBinary()
await Promise.all([downloadBinary(javy), downloadBinary(plugin)])

// Using the `build` command we want to emit:
//
// `javy build -C dynamic -C wit=<path> -C wit-world=val -o <path> <function.js>`
const args = ['build', '-C', 'dynamic', ...extra, '-o', fun.outputPath, 'dist/function.js']
// `javy build -C dynamic -C plugin=path/to/javy_quickjs_provider_v3.wasm -C wit=<path> -C wit-world=val -o <path> <function.js>`
const args = [
'build',
'-C',
'dynamic',
'-C',
`plugin=${plugin.path}`,
...extra,
'-o',
fun.outputPath,
'dist/function.js',
]

return exec(javy.path, args, {
cwd: fun.directory,
Expand All @@ -187,7 +198,7 @@
const javyRequired = app.allExtensions.some((ext) => ext.features.includes('function') && ext.isJavaScript)
if (javyRequired) {
const javy = javyBinary()
await installBinary(javy)
await downloadBinary(javy)
}
}

Expand Down Expand Up @@ -274,7 +285,7 @@
}

export function jsExports(fun: ExtensionInstance<FunctionConfigType>) {
const targets = fun.configuration.targeting || []

Check warning on line 288 in packages/app/src/cli/services/function/build.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/function/build.ts#L288

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const withoutExport = targets.filter((target) => !target.export)
const withExport = targets.filter((target) => Boolean(target.export))

Expand Down
6 changes: 3 additions & 3 deletions packages/app/src/cli/services/function/runner.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {runFunction} from './runner.js'
import {functionRunnerBinary, installBinary} from './binaries.js'
import {functionRunnerBinary, downloadBinary} from './binaries.js'
import {testFunctionExtension} from '../../models/app/app.test-data.js'
import {describe, test, vi, expect} from 'vitest'
import {exec} from '@shopify/cli-kit/node/system'
Expand All @@ -10,7 +10,7 @@ vi.mock('./binaries.js', async (importOriginal) => {
const original = await importOriginal<typeof import('./binaries.js')>()
return {
...original,
installBinary: vi.fn().mockResolvedValue(undefined),
downloadBinary: vi.fn().mockResolvedValue(undefined),
}
})

Expand All @@ -23,7 +23,7 @@ describe('runFunction', () => {
await runFunction({functionExtension})

// Then
expect(installBinary).toHaveBeenCalledOnce()
expect(downloadBinary).toHaveBeenCalledOnce()
})

test('runs function with options', async () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/function/runner.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {functionRunnerBinary, installBinary} from './binaries.js'
import {functionRunnerBinary, downloadBinary} from './binaries.js'
import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
import {exec} from '@shopify/cli-kit/node/system'
Expand All @@ -19,7 +19,7 @@ interface FunctionRunnerOptions {

export async function runFunction(options: FunctionRunnerOptions) {
const functionRunner = functionRunnerBinary()
await installBinary(functionRunner)
await downloadBinary(functionRunner)

const args: string[] = []
if (options.inputPath) {
Expand Down
Loading