Skip to content

Support WASM Api - #460

Merged
mssalemi merged 1 commit into
mainfrom
ms.wasm-api-support
May 7, 2025
Merged

Support WASM Api#460
mssalemi merged 1 commit into
mainfrom
ms.wasm-api-support

Conversation

@mssalemi

@mssalemi mssalemi commented Apr 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Wasm API Support for function-runner

Closes: https://github.com/shop/issues-shopifyvm/issues/32

Function-runner now detects when a module is using the Wasm API by checking if it imports modules with names starting with shopify_function_v.

When a Wasm API function is detected:

  1. The input is automatically transcoded from JSON to MessagePack
  2. The Wasm API provider (shopify_function_v0.0.1.wasm) is loaded
  3. The output is automatically transcoded from MessagePack back to JSON

For output handling:

  1. Locates the actual MessagePack data in the output stream
  2. Extracts only the relevant MessagePack portion for processing
  3. Properly decodes it to JSON for the function result

🎩 Tophat

WASM API

cargo run -- -f tests/fixtures/build/echo.trampolined.wasm -i tests/fixtures/input/wasm_api_function_input.json

JAVY_V2

cargo run -- -f tests/fixtures/build/js_function_javy_plugin_v2.wasm -i tests/fixtures/input/js_function_input.json -e run

Testing

  • Added unit tests for Wasm API function execution
  • Added integration tests for running Wasm API functions via CLI
  • Tested with the example echo.trampolined.wasm function

note: todo - test everything else is still working as expected

@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from 6658614 to cc411d2 Compare April 14, 2025 14:40
@mssalemi
mssalemi marked this pull request as ready for review April 14, 2025 14:42
adampetro
adampetro reviewed Apr 16, 2025
View reviewed changes

@adampetro adampetro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if it would make sense to create a Codec enum like:

enum Codec {
  Json,
  Msgpack,
}

and then select a codec based on conditions, then put all of the conditional codec logic into methods on Codec, such as:

impl Codec {
  fn transcode_from_json_bytes(&self, bytes: Vec<u8>) -> `anyhow::Result<Vec<u8>>` {
    match self {
      Self::Json => Ok(bytes),
      Self::Msgpack => {
        todo!()
      }
    }
  }
}

Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from 3e3ef79 to b0786c0 Compare April 17, 2025 13:45
adampetro
adampetro reviewed Apr 17, 2025
View reviewed changes
Comment thread src/codec.rs Outdated
Comment thread src/codec.rs Outdated
Comment thread src/codec.rs Outdated
andrewhassan
andrewhassan reviewed Apr 23, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs
Comment thread tests/fixtures/build/echo.trampolined.wasm
@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch 2 times, most recently from 97e9e7c to f728a5f Compare April 24, 2025 13:47
@saulecabrera

saulecabrera commented Apr 25, 2025

Copy link
Copy Markdown
Contributor

@mssalemi could you hold on until #464 lands? In that PR I've fixed the codec handling across the entire codebase, which I think will make it easier to do the encoding inferencing introduced in this PR.

@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch 2 times, most recently from 5de7479 to a804934 Compare April 25, 2025 17:41
saulecabrera
saulecabrera reviewed Apr 25, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from 3258dcb to 7bd4fd0 Compare April 25, 2025 20:06
@mssalemi
mssalemi marked this pull request as draft April 28, 2025 12:45
@mssalemi
mssalemi marked this pull request as ready for review April 28, 2025 13:10
saulecabrera
saulecabrera reviewed Apr 28, 2025
View reviewed changes

@saulecabrera saulecabrera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, I think after the last two comments are resolved we can land this.

Comment thread src/engine.rs Outdated
Comment thread tests/fixtures/README.md Outdated
@mssalemi
mssalemi requested a review from saulecabrera April 29, 2025 15:00
andrewhassan
andrewhassan reviewed Apr 29, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/main.rs Outdated
saulecabrera
saulecabrera approved these changes Apr 29, 2025
View reviewed changes

@saulecabrera saulecabrera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for iterating on this. Approving from my end, I'll leave the final approval to @andrewhassan, given the latest comments.

@mssalemi
mssalemi requested a review from andrewhassan May 1, 2025 15:05
adampetro
adampetro reviewed May 1, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs Outdated
Comment thread src/engine.rs
adampetro
adampetro reviewed May 1, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
adampetro
adampetro reviewed May 1, 2025
View reviewed changes
Comment thread src/engine.rs Outdated
adampetro
adampetro approved these changes May 1, 2025
View reviewed changes

@adampetro adampetro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's squash the commits before merging

@adampetro

adampetro commented May 1, 2025

Copy link
Copy Markdown
Contributor

And hold off on merging for the finalized initial version of the provider module

@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from e9b12e9 to 469de76 Compare May 1, 2025 20:28
@adampetro
adampetro force-pushed the ms.wasm-api-support branch from 9bddef5 to 0a99983 Compare May 6, 2025 17:05
andrewhassan
andrewhassan reviewed May 6, 2025
View reviewed changes
Comment thread src/main.rs Outdated
@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from a33ddba to d331604 Compare May 7, 2025 12:45
adampetro
adampetro requested changes May 7, 2025
View reviewed changes
Comment thread tests/integration_tests.rs
Comment thread tests/integration_tests.rs
Comment thread tests/integration_tests.rs
Comment thread tests/integration_tests.rs
adampetro
adampetro reviewed May 7, 2025
View reviewed changes
Comment thread src/container.rs Outdated
adampetro
adampetro approved these changes May 7, 2025
View reviewed changes
Comment thread tests/integration_tests.rs Outdated
@mssalemi
mssalemi force-pushed the ms.wasm-api-support branch from 9a31447 to b0447d9 Compare May 7, 2025 19:20
@mssalemi
mssalemi merged commit 620e535 into main May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewhassan andrewhassan andrewhassan left review comments
@adampetro adampetro adampetro approved these changes
@otheriault otheriault Awaiting requested review from otheriault
+1 more reviewer
@saulecabrera saulecabrera saulecabrera approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mssalemi @saulecabrera @adampetro @andrewhassan