Download and use Javy plugin when building JS functions #4831
Conversation
Coverage report
Test suite run success1949 tests passing in 886 suites. Report generated by 🧪jest coverage report action from be45523 |
andrewhassan
requested review from
a team,
jacobsteves,
saga-dasgupta and
lopert
and removed request for
a team and
saga-dasgupta
There was a problem hiding this comment.
How do I get to the expected underlying error from javy?
When tophatting, I get error exit code 2 🤔
~/src/github.com/Shopify/cli jc.javy-plugin *22 ?1 pnpm shopify app function build --path /Users/lopert/code/superset/superset-alex-local/extensions/unstable-ccv-vars-1 1 ✘ 22.2.0
> @0.0.0 shopify /Users/lopert/src/github.com/Shopify/cli
> nx build cli && node packages/cli/bin/dev.js "app" "function" "build" "--path" "/Users/lopert/code/superset/superset-alex-local/extensions/unstable-ccv-vars-1"
...more cli output
Building GraphQL types ...
── external error ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error coming from `/Users/lopert/src/github.com/Shopify/cli/packages/app/dist/cli/services/bin/javy build -C dynamic -C
plugin=/Users/lopert/src/github.com/Shopify/cli/packages/app/dist/cli/services/bin/javy_quickjs_provider_v3.wasm -C
wit=/private/var/folders/57/c41xs80j07vdldqk6k1ntxxm0000gn/T/d6830e15470cd535466f1ccfc4c02bb7/javy-world.wit -C wit-world=shopify-function -o
/Users/lopert/code/superset/superset-alex-local/extensions/unstable-ccv-vars-1/dist/function.wasm dist/function.js`
Command failed with exit code 2: /Users/lopert/src/github.com/Shopify/cli/packages/app/dist/cli/services/bin/javy build -C dynamic -C
plugin=/Users/lopert/src/github.com/Shopify/cli/packages/app/dist/cli/services/bin/javy_quickjs_provider_v3.wasm -C
wit=/private/var/folders/57/c41xs80j07vdldqk6k1ntxxm0000gn/T/d6830e15470cd535466f1ccfc4c02bb7/javy-world.wit -C wit-world=shopify-function -o
/Users/lopert/code/superset/superset-alex-local/extensions/unstable-ccv-vars-1/dist/function.wasm dist/function.js| describe('javy-plugin', () => { | ||
| test('properties are set correctly', () => { | ||
| expect(javyPlugin.name).toBe('javy_quickjs_provider_v3') | ||
| expect(javyPlugin.version).match(/^v\d.\d.\d$/) |
There was a problem hiding this comment.
Should part of the version identifier here match the expected version? Or rather, some kind of "minimum version"
There was a problem hiding this comment.
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.
|
@lopert can you comment out the Also I'm not sure why the contents of the stderr stream are not being displayed when it's set to |
|
/snapit |
|
🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/cli@0.0.0-snapshot-20241115163229
|
|
@lopert it looks like the cause was I had a more recent version of Javy cached in my local dev env for the CLI package so exit code and error message emitted to stderr was different. I'll have to rebase this PR on top of the changes in main anyway so that'll cause the slightly newer Javy to be used if you run |
jeffcharles
force-pushed
the
jc.javy-plugin
branch
from
4a60f20
to
ef5ac66
Compare
|
@lopert the rebased PR has an exit code of 1. You will want to run |
|
/snapit |
|
🫰✨ Thanks @jeffcharles! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/cli@0.0.0-snapshot-20241115171206
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Latest errors seem to be: which does not seem to be related to my change |
WHY are these changes introduced?
Javy is being updated to require a Javy plugin, which is a WebAssembly module exposing a specific API, when building dynamically linked modules. Not specifying a plugin when using
javy build -C dynamicwill cause Javy to exit with an error saying a plugin is required. So, the Shopify CLI, will need to be updated to download and use a Javy plugin. This change will use the default Javy plugin.This change WILL NOT WORK until a new version of Javy is released that supports the-C plugin=...argument for dynamically linked modules. I would like to get an advance review of this change to ensure there isn't a long wait for when the Shopify CLI will be able to use future versions of Javy before I publish a new release of Javy that will include this breaking change.WHAT is this pull request doing?
DownloadableBinaryan interface and creating two implementation classes. One is the existingDownloadableBinaryrenamed toExecutableand the otherJavyPlugin. This is because the logic for determining the path and download URL are different. Also renamedinstallBinarytodownloadBinary.javy buildto use the plugin that was downloaded by specifying an additional argument of-C plugin=<path_to_plugin>.How to test your changes?
pnpm cleanfollowed bypnpm shopify app function build --path <path_to_js_function_extension>.It'll complain about Javy returning an exit code of 1 and the underlying message from Javy will be that plugins are not currently supported for dynamically linked modules.Measuring impact
How do we know this change was effective? Please choose one:
Checklist