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

Validate @shopify/shopify_function version on build #5153

Open
wants to merge 3 commits 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
5 changes: 5 additions & 0 deletions .changeset/tough-guests-behave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/app': patch
---

Validate the @shopify/shopify_function NPM package version is compatible with the Javy version
25 changes: 25 additions & 0 deletions packages/app/src/cli/services/function/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ async function installShopifyLibrary(tmpDir: string) {
const runModule = joinPath(shopifyFunctionDir, 'run.ts')
await writeFile(runModule, '')

const packageJson = joinPath(shopifyFunctionDir, 'package.json')
await writeFile(packageJson, JSON.stringify({version: '1.0.0'}))

return shopifyFunction
}

Expand Down Expand Up @@ -136,6 +139,28 @@ describe('bundleExtension', () => {
})
})

test('errors if shopify library is not on a compatible version', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
const incompatibleVersion = '0.0.1'
const ourFunction = await testFunctionExtension({dir: tmpDir})
ourFunction.entrySourceFilePath = joinPath(tmpDir, 'src/index.ts')
await installShopifyLibrary(tmpDir)
await writeFile(
joinPath(tmpDir, 'node_modules/@shopify/shopify_function/package.json'),
JSON.stringify({version: incompatibleVersion}),
)

// When
const got = bundleExtension(ourFunction, {stdout, stderr, signal, app})

// Then
await expect(got).rejects.toThrow(
/The installed version of the Shopify Functions JavaScript library is not compatible with this version of Shopify CLI./,
)
})
})

test('errors if user function not found', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
Expand Down
58 changes: 44 additions & 14 deletions packages/app/src/cli/services/function/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,34 @@
import {exec} from '@shopify/cli-kit/node/system'
import {dirname, joinPath} from '@shopify/cli-kit/node/path'
import {build as esBuild, BuildResult} from 'esbuild'
import {findPathUp, inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs'
import {findPathUp, inTemporaryDirectory, readFile, writeFile} from '@shopify/cli-kit/node/fs'
import {AbortSignal} from '@shopify/cli-kit/node/abort'
import {renderTasks} from '@shopify/cli-kit/node/ui'
import {pickBy} from '@shopify/cli-kit/common/object'
import {runWithTimer} from '@shopify/cli-kit/node/metadata'
import {AbortError} from '@shopify/cli-kit/node/error'
import {Writable} from 'stream'

export const SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION = '1'

class InvalidShopifyFunctionPackageError extends AbortError {
constructor(message: string) {
super(
message,
outputContent`Make sure you have a compatible version of the ${outputToken.yellow(
'@shopify/shopify_function',
)} library installed.`,
[
outputContent`Add ${outputToken.green(
`"@shopify/shopify_function": "~${SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION}.0.0"`,
)} to the dependencies section of the package.json file in your function's directory, if not already present.`
.value,
`Run your package manager's install command to update dependencies.`,
andrewhassan marked this conversation as resolved.
Show resolved Hide resolved
],
)
}
}

interface JSFunctionBuildOptions {
stdout: Writable
stderr: Writable
Expand Down Expand Up @@ -117,19 +137,7 @@
})

if (!entryPoint || !runModule) {
throw new AbortError(
'Could not find the Shopify Functions JavaScript library.',
outputContent`Make sure you have the latest ${outputToken.yellow(
'@shopify/shopify_function',
)} library installed.`,
[
outputContent`Add ${outputToken.green(
'"@shopify/shopify_function": "1.0.0"',
)} to the dependencies section of the package.json file in your function's directory, if not already present.`
.value,
`Run your package manager's install command to update dependencies.`,
],
)
throw new InvalidShopifyFunctionPackageError('Could not find the Shopify Functions JavaScript library.')
}

if (!fun.entrySourceFilePath) {
Expand All @@ -139,11 +147,32 @@
return entryPoint
}

async function validateShopifyFunctionPackageVersion(fun: ExtensionInstance<FunctionConfigType>) {
const packageJsonPath = await findPathUp('node_modules/@shopify/shopify_function/package.json', {
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tried this path with different package managers? I know that pnpm sometimes uses a different folder structure for modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not, but I was following the implementation here and assumed they validated this.

type: 'file',
cwd: fun.directory,
})

if (!packageJsonPath) {
throw new InvalidShopifyFunctionPackageError('Could not find the Shopify Functions JavaScript library.')
Copy link
Contributor

Choose a reason for hiding this comment

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

I could not get this to trigger in a way that made sense to me.

If I removed the dependency, deleted the node_modules folder, ran npm install in my extension, then tried the build command, I'd get an unrelated (?) error.

Command failed with exit code 1: npm exec -- graphql-code-generator --config package.json

But if I went in and manually removed just the node_modules/@shopify/shopify_function/package.json file, I got this message 👍

What kind of config / steps would lead to a user actually getting into this position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, I think this could happen if the user removed the @shopify/shopify_function dependency so that it wasn't present at all in their node_modules folder.

}

const packageJson = JSON.parse(await readFile(packageJsonPath))
const majorVersion = packageJson.version.split('.')[0]

if (majorVersion !== SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION) {
throw new InvalidShopifyFunctionPackageError(
'The installed version of the Shopify Functions JavaScript library is not compatible with this version of Shopify CLI.',
)
}
}

export async function bundleExtension(
fun: ExtensionInstance<FunctionConfigType>,
options: JSFunctionBuildOptions,
processEnv = process.env,
) {
await validateShopifyFunctionPackageVersion(fun)
const entryPoint = await checkForShopifyFunctionRuntimeEntrypoint(fun)

const esbuildOptions = {
Expand Down Expand Up @@ -276,6 +305,7 @@
}

async bundle(fun: ExtensionInstance<FunctionConfigType>, options: JSFunctionBuildOptions, processEnv = process.env) {
await validateShopifyFunctionPackageVersion(fun)
await checkForShopifyFunctionRuntimeEntrypoint(fun)

const contents = this.entrypointContents
Expand Down Expand Up @@ -333,7 +363,7 @@
}

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

Check warning on line 366 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#L366

[@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
7 changes: 5 additions & 2 deletions packages/app/src/cli/services/generate/extension.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {configurationFileNames, versions} from '../../constants.js'
import {AppInterface} from '../../models/app/app.js'
import {buildGraphqlTypes} from '../function/build.js'
import {buildGraphqlTypes, SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION} from '../function/build.js'
import {GenerateExtensionContentOutput} from '../../prompts/generate/extension.js'
import {ExtensionFlavor, ExtensionTemplate} from '../../models/app/template.js'
import {ensureDownloadedExtensionFlavorExists, ensureExtensionDirectoryExists} from '../extensions/common.js'
Expand Down Expand Up @@ -82,7 +82,7 @@
(flavor) => flavor.value === extensionFlavorValue,
)
const directory = await ensureExtensionDirectoryExists({app: options.app, name: extensionName})
const url = options.cloneUrl || options.extensionTemplate.url

Check warning on line 85 in packages/app/src/cli/services/generate/extension.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/generate/extension.ts#L85

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const uid = options.developerPlatformClient.supportsAtomicDeployments ? randomUUID() : undefined
const initOptions: ExtensionInitOptions = {
directory,
Expand Down Expand Up @@ -174,7 +174,7 @@
})

if (templateLanguage === 'javascript') {
const srcFileExtension = getSrcFileExtension(extensionFlavor?.value || 'rust')

Check warning on line 177 in packages/app/src/cli/services/generate/extension.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/generate/extension.ts#L177

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
await changeIndexFileExtension(directory, srcFileExtension, '!(*.graphql)')
}
},
Expand Down Expand Up @@ -299,7 +299,10 @@
export function getFunctionRuntimeDependencies(templateLanguage: string): DependencyVersion[] {
const dependencies: DependencyVersion[] = []
if (templateLanguage === 'javascript') {
dependencies.push({name: '@shopify/shopify_function', version: '1.0.0'})
dependencies.push({
name: '@shopify/shopify_function',
version: `~${SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION}.0.0`,
})
}
return dependencies
}
Expand Down
Loading