-
Notifications
You must be signed in to change notification settings - Fork 502
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
fix: omit 'path' in undici.request opts (ts) #559
Conversation
In undici.request|stream|pipeline helper functions, the opts param may not contain path. see https://github.com/nodejs/undici#undicirequesturl-opts-promise
Unfortunately the types are quite out of date from the v3 API. I'm okay with this change though to bring it closer - I'm fairly confident not a lot of folks are using undici in TS projects just yet |
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.
lgtm
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.
Could you fix the types tests?
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 16 16
Lines 1398 1398
=======================================
Hits 1392 1392
Misses 6 6 Continue to review full report at Codecov.
|
@mcollina type tests updated |
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.
lgtm
@Ethan-Arrowood could you take a look?
Hello! I've just run in to this issue in our TS project :) I can see this code has been merged but not deployed yet, do you have a planned release date at all for this fix? Thank you! |
In
undici.request|stream|pipeline
helper functions, the opts param may not contain path.see https://github.com/nodejs/undici#undicirequesturl-opts-promise
Without this fix you get the following Typescript error:
But if you pass the path parameter you get a runtime error: https://github.com/nodejs/undici/blob/master/lib/agent.js#L86-L88