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

feat: allow fetch to be initialized with user-provided client #656

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

RonnyChan-okta
Copy link
Contributor

Description of changes

Allow consumers to supply their own hyper client when intializing fetch.

Combined with #655 this will allow people to add fetch with their own configured hyper clients. This is particularly important for FIPS compliance.

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RonnyChan-okta RonnyChan-okta force-pushed the fetch-custom-client branch 2 times, most recently from 39e6fef to 50d4b6a Compare October 30, 2024 18:33
@Sytten
Copy link
Collaborator

Sytten commented Oct 30, 2024

We really need to work on a better settings system for modules.
I think I would prefer a setter on the static object than this?

@RonnyChan-okta
Copy link
Contributor Author

RonnyChan-okta commented Oct 30, 2024

I would prefer not having to deal with the static, it's very spooky-action-at-a-distance when the static can change underneath, which would necessitate the Client being under an RwLock which is more expensive than the Lazy right now. Client objects are cheaply cloneable so having the closure own a client is fine.

As well for our use-case, we'd like to re-use the existing hyper client in the rest of our program as it's properly set up for FIPS use-cases when running under FIPS mode. Having the static there makes the boot-up process a bit of a dance to ensure quickjs is using the right hyper client.

Eventually we'd be looking at plugging in different fetch implementations that use different clients for security reasons, and this wouldn't be possible with the static.

BoxBody is a bit unfortunate but having a setter doesn't change this issue, if the client is going to be user-configurable it needs to be able to accept clients that can take Full, StreamBody or Empty as well as other body types. BoxBody is the correct combinator to use to type-erase these differences.

Leaving crate::init to use the static so llrt can keep its existing usage, while allowing fetch::init to take in a client is a fine compromise to allow these use cases without much churn.

@richarddavison
Copy link
Collaborator

@RonnyChan-okta Thanks for your PR. Me and @Sytten have been discussing a better settings system for module for a while now. We need to be able to provide modules that have user defined settings when building the module. The entire global/init/module builder is kind of wonky right now. The latest changes in rquickjs allows for custom user data which I think would be great for this. I'll merge this now and we can think about better options later.

@RonnyChan-okta
Copy link
Contributor Author

@richarddavison Looks like formatting didn't make it in for some reason. Should be fixed now.

@RonnyChan-okta RonnyChan-okta force-pushed the fetch-custom-client branch 2 times, most recently from cf8c22a to 3eb6d8a Compare October 31, 2024 13:13
@richarddavison
Copy link
Collaborator

richarddavison commented Oct 31, 2024

Now it's just clippy complainng about complex type. Run make fix & make check before pushing and we should be good to go!

@RonnyChan-okta
Copy link
Contributor Author

Refactored the type as of 9d0442c

@RonnyChan-okta
Copy link
Contributor Author

Apologies for the back-and-forth. The Makefile does not seem to be working on my machine so I just used cargo fix which caused formatting issues again.

I ran cargo fix --allow-dirty && cargo fmt && cargo fix --allow-dirty && cargo fmt to ensure no more formatting/clippy errors pop up.

@richarddavison
Copy link
Collaborator

No worries, almost there! I think the last issue now is to run cargo build with lambda feature to fix those errors as well

@RonnyChan-okta
Copy link
Contributor Author

RonnyChan-okta commented Oct 31, 2024

Should be fixed in cdc511c.

Not to derail this thread but I'm getting this error on my local machine when building llrt_core, hence the inability to run the makefiles. I had to hack around the build.rs to get cargo fix && cargo fmt to work but just as an FYI this is the error I get:

--- stderr
  !  Warning : nb of samples too low for proper processing ! 
  !  Please provide _one file per sample_. 
  !  Alternatively, split files into fixed-size blocks representative of samples, with -B# 
  Error 14 : nb of samples too low
  Error: Custom { kind: Other, error: "Failed to generate compression dictionary" }

@richarddavison
Copy link
Collaborator

Should be fixed in cdc511c.

Not to derail this thread but I'm getting this error on my local machine when building llrt_core, hence the inability to run the makefiles. I had to hack around the build.rs to get cargo fix && cargo fmt to work but just as an FYI this is the error I get:

--- stderr
  !  Warning : nb of samples too low for proper processing ! 
  !  Please provide _one file per sample_. 
  !  Alternatively, split files into fixed-size blocks representative of samples, with -B# 
  Error 14 : nb of samples too low
  Error: Custom { kind: Other, error: "Failed to generate compression dictionary" }

This is because you haven't built the javascript library yet. We should really give a better error for this!
make js

@RonnyChan-okta
Copy link
Contributor Author

Cool, ran make js && make check && make fix and looks like its good to go!

@richarddavison richarddavison merged commit d18dcf3 into awslabs:main Oct 31, 2024
9 checks passed
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