-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
replace request & httpntlm with axios & axios-ntlm #1146
Conversation
@vision10 please get the build to pass. |
Something got broken with version 0.40.0 (I don't know if it's directly related to that PR).
|
@j0k3r can you submit a PR? |
I didn't take a closer look atm. Maybe @vision10 can have an idea? |
I think you need to use |
with this PR we lost support for sending attachments in MTOM? |
just to inform I have checked and we have a project MTOM attachments working fine on 0.39.0 and after upgrade to 0.40.0 it's stopped to work. This pull request is a breaking change. |
Yes, I said it in the beginning, axios does not handle attachments automatically like request did, and a decision is needed if we should include another pachage or let the user handle them |
Ok but isn't better decide it and implement before release it? |
I agree this is a BC change, but before 1.0.0, everything can be broken.
|
I still think that isn't make sense release this change considering doesn't have any real benefit that justify a feature loss (it is just a replacement of request by axios). It can be worked in separate branch or a RC release until this open issues are fixed. Anyway, I think that will be a good mention it in documentation about this BC. |
I have just upgraded from This I can only assume it is associated to this PR 👀 |
Same here... |
SOAP faults also appear to be broken by this PR.
|
I found the same @wagnerch my solution was just to add the following to my entry point: require('axios').defaults.validateStatus = () => true; I will note this only work for me as this is the only package in my entire deps tree that uses axios 👀 I will note that any error handling you have will probably not work now also as the errors come formatted differently, but that is at least something we can handle in our client code. I should probably make a PR to change the request options here also though 👀 |
@scagood Yeah, unfortunately I use axios, and it is also used in Azure which I also use, so setting this as a default wouldn't work for me. Not sure what the scoping of this default would be? I don't think the exception behavior should change because the underlying HTTP engine has changed. I'll stick on 0.39.0 for now, and see where things go. |
@scagood Thanks for the tip, submitted a PR to fix it. It looks like the requestStream pieces are broken with restoring the status code behavior that requests had, haven't had a chance to investigate it. |
for anyone having an issue with this, since this is using axios you need to change your options
|
other way to use it
|
This is my attempt to replace request and httpntlm packages with axios and axios-ntlm
Its not perfect yet and there are still some issues that need to be addressed:
form-data
package for attachments, but I don't know if we should include another package or let the user handle thisrequestStream
method does not return a request stream but a data response as a stream (because axios does not return the request stream, if only has the option to return the response as a stream with options.responseType='stream')I have updated a few packages (those with minor changes)
Some files have a few more changes because of vs code text autoformat (mainly whitespaces)
On a personal note, I don't think Promises mix well together with callbacks, so maybe callbacks should be dropped in a future version if axios is adopted ?