-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: add request with proxy support for chatgpt #78
Conversation
@@ -11,3 +11,4 @@ | |||
|
|||
# see the readme for how to find this | |||
SESSION_TOKEN= | |||
PROXY_URL= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.env.example is for required env variables; that is why CI is failing right now
@@ -1,5 +1,6 @@ | |||
import ExpiryMap from 'expiry-map' | |||
import pTimeout, { TimeoutError } from 'p-timeout' | |||
import { ProxyAgent, setGlobalDispatcher } from 'undici' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use undici
directly because this package supports more than just node.js.
E.g., we use the default fetch
implementation if one's available globally, and we only fallback to undici
if we have to for node.js 16/17.
@@ -67,6 +75,11 @@ export class ChatGPTAPI { | |||
|
|||
this._accessTokenCache = new ExpiryMap<string, string>(accessTokenTTL) | |||
|
|||
if (proxyUrl) { | |||
this._proxyAgent = new ProxyAgent(proxyUrl) | |||
setGlobalDispatcher(this._proxyAgent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to set a global dispatcher which will affect the whole process; rather, the proxy should only affect our usage of fetch
within this module.
Thanks @wlemuel 🙏 I think this is a great start to adding this feature, but the major blocker right now is that this implementation would drop support for environments other than node.js (like edge functions, browsers, deno, etc.). |
Thanks for mentioning the compatibility issue, I will take into consideration the different runtime environments and find the best solution for compatibility Since this will take some time, I would like to close this pull request temporarily. |
@wlemuel sounds good; I really do appreciate your help and am looking forward to another PR 😄 🙏 |
…ti-platform 👷 Add muti-platform support
No description provided.