-
Notifications
You must be signed in to change notification settings - Fork 930
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
Remove web-streams-polyfill and abort controller polyfill #614
Conversation
I added I was able to set up tests via the OpenAPI spec, seems like there's a few unrelated errors: This is a bit bigger of a change than initially planned, but should be supported by all LTS Node versions. You can also add special instructions for people to polyfill Node 16 like this if you're very worried: https://js.langchain.com/docs/get_started/installation#unsupported-nodejs-16 LMK what you think! |
May also be worth removing |
It is indeed: #402 |
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.
Thank you Jacob!
Will need to have a member of our team review more thoroughly, but first will need to resolve question around DOM
@@ -3,7 +3,7 @@ | |||
"exclude": ["src/_shims/**/*-deno.ts"], | |||
"compilerOptions": { | |||
"target": "es2020", | |||
"lib": ["es2020"], |
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.
ah, sadly I don't think this is an acceptable change b/c we don't want to drag DOM types into our users' projects if they are in a Node setting. Do you remember why it was needed?
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.
ReadableStream
isn't defined in the base typing - let me see if I can find another workaround.
BTW why is this only a partial fix? |
Ah sorry that's a typo, another PR I did for LangChain.js was a partial fix for some typing issues we were seeing and I think my mind was on that. |
With some light Googling, it seems that the only way to get the We've been doing it upstream in LangChain.js for a while now with (seemingly) good compatibility: https://github.com/langchain-ai/langchainjs/blob/main/langchain/tsconfig.json#L10 But if we don't feel comfortable doing that here, then perhaps it's best to hope for a good downstream resolution here and bump the polyfill later on: |
K, then yes that might be best… Is If so, maybe we want to reexport from our node and web shims, rather than remove it? |
This has been fixed in the downstream polyfill - feel free to close this! |
Fixes #613