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

saga-dasgupta · 2024-09-12T19:06:26Z

Issue: https://github.com/Shopify/script-service/issues/7452

WHY are these changes introduced?

To have a consistent way of uploading and obtaining extension data is core using the App Module Framework.

WHAT is this pull request doing?

This PR adds the "bundling" feature to functions and sets the output path for compilation to the bundle directory to ensure the wasm file generated is zipped and uploaded to GCS like every other extension.

The deployment plan is:
This version of CLI goes out which bundles the wasm file and uploads it (existing wasm blob upload stays in place).
Then on Core's side we verify we are able to obtain the wasm content and upload it to our runtime-engine bucket. Then we come back here and delete the uploadWasmBlob path and in core we throw an error when we fail to upload. TLDR: existing upload path stays in place till we can verify new path works.

How to test your changes?

Checkout my branch and set up dev GCS bucket credentials for spin instructions here.
Set up spin credentials instructions here.
Create a functions app in your spin instance.
Checkout my core branch functions.move_wasm_file_to_permanent_bucket on your spin instance.
To remove any false positives replace this upload wasm line in cli with a random UUID.
Run pnpm run shopify app deploy --path [path to your app] from cli
Go to either cli or core logs to obtain the UUID.wasm type name of the file uploaded to the staging bucket. It should match your UUID in the previous step.
Find the uploaded file here.
Run pnpm run shopify app dev --path [path to your functions app] from cli and ensure the wasm file is present in the staging bucket.

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

github-actions · 2024-09-12T19:06:44Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-inner-loop

packages/app/src/cli/models/extensions/extension-instance.ts

saga-dasgupta · 2024-09-13T18:22:04Z

packages/app/src/cli/models/extensions/extension-instance.ts

    }

    await this.build(options)
-
+    if (this.isFunctionExtension) {
+      await copyFile(this.outputPath, joinPath(bundleDirectory, extensionId, 'index.wasm'))


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

saga-dasgupta · 2024-09-13T18:22:32Z

packages/app/src/cli/services/deploy.test.ts

@@ -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 () => {


Keeping the test names consistent.

saga-dasgupta · 2024-09-13T18:23:34Z

packages/app/src/cli/services/deploy.test.ts

@@ -290,7 +290,7 @@ describe('deploy', () => {
      ],
      developerPlatformClient,
      extensionIds: {},
-      bundlePath: undefined,
+      bundlePath: expect.stringMatching(/bundle.zip$/),


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

github-actions · 2024-09-17T16:30:44Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	73% (-0.02% 🔻)	8396/11501
🟡	Branches	69.58% (+0.06% 🔼)	4095/5885
🟡	Functions	71.75% (-0.07% 🔻)	2179/3037
🟡	Lines	73.35% (-0.02% 🔻)	7945/10832

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / marketing_activity.ts	25% (-75% 🔻)	100%	0% (-100% 🔻)	25% (-75% 🔻)
🟢	`...` / context.ts	91.67% (-0.03% 🔻)	87.28%	88.57%	92.62% (-0.03% 🔻)
🔴	`...` / extension.ts	54.55% (-0.72% 🔻)	50%	57.14%	55.81% (-0.94% 🔻)

Test suite run success

1891 tests passing in 857 suites.

Report generated by 🧪jest coverage report action from c1c12ea

github-actions · 2024-09-17T16:32:50Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

andrewhassan

This is looking great! My comments are mostly around Isaac's concerns 😄

packages/app/src/cli/models/extensions/extension-instance.ts

saga-dasgupta · 2024-09-20T20:38:11Z

packages/app/src/cli/services/build/extension.ts

    if (extension.isJavaScript) {
      await runCommandOrBuildJSFunction(extension, options)
    } else {
      await buildOtherFunction(extension, options)
    }
+    if (fileExistsSync(extension.outputPath)) {


Had to add this if condition because I see there are tests like succeeds when is a JS function and build command is not present so I assume thats the intention.

saga-dasgupta · 2024-09-20T20:42:17Z

packages/app/src/cli/services/build/extension.ts

@@ -141,11 +142,18 @@ export async function buildFunctionExtension(
  }

  try {
+    const bundlePath = joinPath(extension.outputPath.split('/').slice(0, -2).join('/'), 'index.wasm')
+    extension.outputPath = joinPath(extension.directory, joinPath('dist', 'function.wasm'))


This little dance avoids isFunctionExtension? in build.

isaacroldan · 2024-09-23T08:35:45Z

packages/app/src/cli/services/dev/update-extension.ts

-      ...config,
-      serialized_script: encodedFile,
-    }),
+    config: JSON.stringify(draftableConfig),


Does this mean we can remove the custom wasm update then? or are we waiting until this is released and used for a bit?

Yes, the latter!

isaacroldan · 2024-09-23T08:41:49Z

packages/app/src/cli/services/deploy/bundle.test.ts

@@ -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 () => {


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

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.

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

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

isaacroldan · 2024-09-23T08:44:09Z

packages/app/src/cli/services/build/extension.ts

+    if (fileExistsSync(extension.outputPath)) {
+      const base64Contents = await readFile(extension.outputPath, {encoding: 'base64'})
+      await touchFile(bundlePath)
+      await writeFile(bundlePath, base64Contents)


Have you tested if you need the touchFile line or is enough with writeFile? (I don't think writeFile needs the file to exist, but I might be wrong)

I do, it was complaining no such file or directory exists without it so I had to add it.

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch 3 times, most recently from e0c4923 to 8c1179a Compare September 13, 2024 16:26

saga-dasgupta commented Sep 13, 2024

View reviewed changes

packages/app/src/cli/models/extensions/extension-instance.ts Outdated Show resolved Hide resolved

saga-dasgupta commented Sep 13, 2024

View reviewed changes

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch 3 times, most recently from ad36761 to 04ce5e5 Compare September 17, 2024 16:22

Bundling and uploading for functions

c5a48a8

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch from 04ce5e5 to c5a48a8 Compare September 17, 2024 16:26

saga-dasgupta marked this pull request as ready for review September 17, 2024 16:32

andrewhassan reviewed Sep 17, 2024

View reviewed changes

packages/app/src/cli/models/extensions/extension-instance.ts Show resolved Hide resolved

packages/app/src/cli/models/extensions/extension-instance.ts Outdated Show resolved Hide resolved

saga-dasgupta requested a review from jacobsteves September 18, 2024 16:00

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch 2 times, most recently from a53dfe6 to 16a1fca Compare September 20, 2024 20:27

saga-dasgupta commented Sep 20, 2024

View reviewed changes

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch from 16a1fca to c1c12ea Compare September 20, 2024 20:40

saga-dasgupta commented Sep 20, 2024

View reviewed changes

saga-dasgupta requested review from isaacroldan and andrewhassan September 20, 2024 20:43

isaacroldan reviewed Sep 23, 2024

View reviewed changes

draftable dev works now and addressing pr comments

4c9da9a

saga-dasgupta force-pushed the functions_7452.gcs_upload_solution branch from c1c12ea to 4c9da9a Compare September 23, 2024 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saga-dasgupta commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024

saga-dasgupta Sep 13, 2024

saga-dasgupta Sep 13, 2024

saga-dasgupta Sep 13, 2024

github-actions bot commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024

andrewhassan left a comment

saga-dasgupta Sep 20, 2024 •

edited

Loading

saga-dasgupta Sep 20, 2024

isaacroldan Sep 23, 2024

saga-dasgupta Sep 23, 2024

isaacroldan Sep 23, 2024

saga-dasgupta Sep 23, 2024

saga-dasgupta Sep 23, 2024

saga-dasgupta Sep 23, 2024

isaacroldan Sep 23, 2024

saga-dasgupta Sep 23, 2024

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

Are you sure you want to change the base?

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

Conversation

saga-dasgupta commented Sep 12, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

github-actions bot commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Sep 17, 2024

andrewhassan left a comment

Choose a reason for hiding this comment

saga-dasgupta Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saga-dasgupta commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

saga-dasgupta Sep 20, 2024 •

edited

Loading