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

allow passing request wrapper option #124

Closed
wants to merge 1 commit into from

Conversation

jstewmon
Copy link

request has a robust set of options that are currently inaccessible. This PR allows FB users to supply their own request wrapper, so that all request options are configurable.

allows #100 to be addressed by library users by supplying their own agent

@dantman
Copy link

dantman commented Dec 12, 2017

What request options other than those in #100 are useful?

Request refuses to play nice with libraries following semver, it's bloated making our library+deps larger than it needs to be, and the bloat is also a potential source of future security issues caused by things we don't need. Rather than exposing request options the current goal is to replace request entirely with a much more lightweight library.

@jstewmon
Copy link
Author

I'm only concerned with being able to pass an agent and set the time option, so that I can instrument and optimize connection handling. Getting the timing info will require another change to expose the response object somewhere.

I suppose you're reluctant to make an option for request b/c swapping the client library would become a major semver change?

Do you have an alternate library in mind? I can make another PR that moves to the library you want and make it configurable.

@dantman
Copy link

dantman commented Dec 12, 2017

I'm reluctant because it would make some usage of the library request-specific and would make upgrading difficult when we do switch libraries.

I haven't done a complete comparison yet, but when I did a quick look over node.js http libraries, needle seemed like it might be a good choice. Few deps, promises, recent commits, and no fancy version restrictions.

@jstewmon
Copy link
Author

I think needle is a fine choice. I implemented it in #125

@jstewmon jstewmon closed this Oct 3, 2023
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.

2 participants