Skip to content

Allow configuring axios, this enables eg passing an axios adapter which runs outside of node #29

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

Closed
wants to merge 1 commit into from

Conversation

nfcampos
Copy link

No description provided.

@zeke
Copy link
Member

zeke commented Mar 29, 2023

Hey @nfcampos 👋🏼 Thanks for the PR.

I heard about you from @hwchase17, whom I met last night at a meetup. I'm guessing you're proposing this change in relation to @cbh123's PR to add Replicate support to langchainjs? langchain-ai/langchainjs#510

A couple thoughts:

  • Can you explain what this is useful for? You're alluding to it in the PR title, but it would be useful to have more clarity around the purpose of this change. I'm guessing it's useful for testing, so you make a mock client that doesn't actually make HTTP calls. Is that right?
  • Exposing an option like this kind of binds us to using axios, which might make it harder to move to another HTTP client, if we ever want to do that... like fetch. I have no intention of doing this, but thought I'd surface it. @mattt what do you think?
  • If we wanna ship this, we should document it in the README.

@mattt
Copy link
Contributor

mattt commented Mar 29, 2023

  • Exposing an option like this kind of binds us to using axios, which might make it harder to move to another HTTP client, if we ever want to do that... like fetch. I have no intention of doing this, but thought I'd surface it. @mattt what do you think?

The decision to use axios was entirely pragmatic — it was the first thing I got working, so I went with that. I'd be very open to dropping it entirely for fetch if that works cross-platform reasonably well.

Speaking more directly to this PR: How we make HTTP requests is an implementation detail that we shouldn't expose to the public API. If we want users to configure how this is done, we should advise them to replace client.request with their preferred method.

@nfcampos
Copy link
Author

The only purpose this would serve for me (LangChain) is precisely to replace the implementation with a fetch-based adapter, so that this can be used outside Node. We already do this with the openai sdk which also (unfortunately) uses axios.

I agree with your comments re: not wanting to be tied to axios, this was just a lot less work for me than rewriting your whole library to use fetch

@mattt
Copy link
Contributor

mattt commented Mar 30, 2023

@nfcampos I got halfway through writing a response to your comment before deciding to do the exercise of swapping out the request implementation myself. There's a PR to swap out axios with polyfilled fetch here: #31. Please take a look when you have a chance 😃

@mattt
Copy link
Contributor

mattt commented Apr 5, 2023

Closing in favor of #31

Thanks for your help with this, @nfcampos!

@mattt mattt closed this Apr 5, 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.

3 participants