Skip to content

Conversation

aturingmachine
Copy link

@aturingmachine aturingmachine commented Aug 6, 2025

Summary

Attempts to add support for using the Flagsmith Rust SDK on the Fastly Compute@Edge platform. This PR does not add the needed support for running Unit Tests on the main crate while compiling it to target Compute@Edge; more information is provided in the Notes section.

Problem

Related to #28.

  • Cannot compile while targeting wasm and using the blocking feature of the reqwest crate.
    • reqwest::blocking cannot run in an async runtime such as wasm.
    • essentially means we cannot make use of the Flagsmith SDK.
  • In order to make requests on Compute@Edge we need to make use of the fastly crate (as far as I know)
    • we do not want to compile with this crate if we are not specifically requesting it as testing Compute@Edge code requires viceroy.

Solution

Plan is to apply an abstraction over the reqwest::blocking::Client and a "Client shaped" wrapper around fastly::http::Request in order to avoid making too many modifications to the core SDK code.

In order to resolve the compilation issues anything making use of reqwest::blocking and httpmock have been flagged to compile IFF not(target = "wasm"), whereas the inverse target = "wasm" has been applied to the fastly dependent code.

Notes

  • Unit tests run via cargo test (for the default features) are passing, however running unit tests with the fastly crate enabled will require the addition of viceroy and running the tests via cargo-nextest, and quite a bit of scaffolding work that I am unsure if this package wants to take on.
    • existing (non wasm) unit tests can be run via cargo test
    • unit tests for fastly/wasm code can be run via cargo nextest run --target wasm32-wasip1 flagsmith::client; or by running ./scripts/fastly-unit-test.sh
    • running scripts/test.sh will run both platforms of unit tests

Links

- updates reqwest dependency to not include blocking feature
- adds features
  - "default" enables the reqwest/blocking feature
  - "non_blocking" enables the fastly module
  - throw compilation error if both default and non_blocking features
    are active
- adds abstraction for "client" interaction
  - SafeClient struct contains (based on feature flags)
    - a wrapper around reqwest::blocking::Client
    - or a struct to act as a Client for making requests via
      fastly::http::Request
- updates implementation of mod.rs and analytics.rs to consume new
  abstraction struct

- still need to update error handling/types/etc
- fix constructor call for FastlyClient
- remove stray whitespace from Cargo.toml
- remove commented code from analytics.rs
- update FastlyClient to actually set the headers on the request
@matthewelwell
Copy link
Contributor

Thanks for the PR @aturingmachine, we'll give it a review as soon as we can.

@aturingmachine
Copy link
Author

aturingmachine commented Aug 11, 2025

Thanks for the PR @aturingmachine, we'll give it a review as soon as we can.

Awesome, thanks! If need be I can try to find some time to get all the unit testing stuff set up for using the fastly crate.

If the splitting between using reqwest and fastly is too much, could it make sense to make this a crate of its own?

@gagantrivedi gagantrivedi self-requested a review August 13, 2025 04:12
Copy link
Member

@gagantrivedi gagantrivedi left a comment

Choose a reason for hiding this comment

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

Hi, thanks for this — it looks really good. The overhead of maintaining two different libs (one with reqwest and another with fastly) will be a bit much. I think the best path forward would be to add unit tests for the Fastly part of the code, and maybe in the future we can get rid of reqwest and use http (used by Fastly) directly.

- adds plumbing to run fastly depended specs via viceroy
- swaps from using feature flags to using architecture cfg flags
  - these play nicer with nextest
  - also allows us to exclude httpmock from the dev deps if we are
    targeting wasm as httpmock does not support wasm
- updates existing unit tests to be cfg flagged
  - excludes them from compilation if the target is wasm
  - im sorry but i could not find another way to accomplish this
@aturingmachine
Copy link
Author

aturingmachine commented Aug 13, 2025

Ok I have updated this to add the "plumbing" to run unit tests on the fastly dependent code. This was a bigger change than I anticipated as I had to swap from using feature flags to using the build target for the flag.

This is required me to completely exclude all the existing tests from the wasm target as httpmock requires polling which does not support wasm. This means that none of the existing integration tests are currently run on the fastly/wasm build.

I am unsure of the preferred way to split these tests up, so I have added some scripts:

  • scripts/fastly-unit-test.sh will run the fastly/wasm unit tests via nextest (only targeting the modules that actually use fastly)
  • scripts/test.sh will run the existing unit tests via cargo test, then execute the scripts/fastly-unit-test.sh script.

Where this sits now should be a good enough place to figure out a way to write some integration tests for the fastly side if that is desired. Let me know if I can be of any further help.

Should also note I have not tested this "in the wild" at all, and the code currently assumes that implementer has a fastly compute backend named 'flagsmith'; I call this out as this is something that should probably be added to documentation if/when this ever goes out.

- remove commented features and code
@gagantrivedi
Copy link
Member

Thanks, @aturingmachine,I will keep this open for now as a reference for anyone else looking to run our SDK on Fastly, and I’ll pick it up once we’re done with the major refactoring work (currently going on) across our SDKs and have tested this enough to feel confident about adding it to the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants