Avoid changing global axios.defaults.transformResponse #47
Conversation
src/apiProvider.ts
Outdated
| @@ -24,10 +24,16 @@ export class ApiProvider implements IApiProvider { | |||
| */ | |||
| constructor(url: string, config?: AxiosRequestConfig) { | |||
| this.url = url; | |||
| // See: https://github.com/axios/axios/issues/983 | |||
There was a problem hiding this comment.
Maybe tie the comment to the transformResponse property?
src/proxyProvider.ts
Outdated
| @@ -26,8 +26,14 @@ export class ProxyProvider implements IProvider { | |||
| */ | |||
| constructor(url: string, config?: AxiosRequestConfig) { | |||
| this.url = url; | |||
| // See: https://github.com/axios/axios/issues/983 | |||
| this.config = config || { | |||
There was a problem hiding this comment.
I guess we should use Object.assign or something to merge the provided config and the defaults so that one default won't get lost when a config is passed as a parameter.
There was a problem hiding this comment.
Ping @pctj101 - Do you think you can implement the requested changes? Or should we do it - there's no problem either way.
Then we can merge this PR :)
There was a problem hiding this comment.
Oh hi! I was caught up in a few things but I can get to it later today :) Thanks for saying hi!
There was a problem hiding this comment.
pushed up a change, but definitely take a look at the data type comment below in case you have any insight.
src/apiProvider.ts
Outdated
| } | ||
| // See: https://github.com/axios/axios/issues/983 regarding transformResponse | ||
| transformResponse: [ | ||
| function(data: any) { |
There was a problem hiding this comment.
Typescript complained about a lack of type for data, so I put "any"
It seems in most cases it's a string, but looking at axios itself, it seems there are times that even axios needs to fall back when data is not a string.
https://github.com/axios/axios/blob/master/lib/defaults.js#L72
At least for elrond cases, the config scoped for the purposes of elrond APIs seems to be able to assume a string.
What do you think?
There was a problem hiding this comment.
Hey there. It's ok for now to leave it with any. Regardless of what axios provides in transformResponse as data, I think we should make sure ourselves it's a string before passing it to JSONbig.parse. But this is out of the scope of this PR, I'll make sure we update the error handling here in the case data is not a string.
There was a problem hiding this comment.
@pctj101 I changed the base to development. Can you please resolve the two conflicts? They're small, you should just keep your changes :)
There was a problem hiding this comment.
@ccorcoveanu Hello again! Appreciate the feedback. Merged against development and matched up the indent.
Accept "Uint8Array", in addition to "Buffer" (on the main flows: sign, verify)
Accept "Uint8Array", in addition to "Buffer" (on the main flows: sign, verify)
What do you think about isolating the axios config to the local file, rather than changing axios for the global scope?
For example, this is one case of the axios global config change introduced by erdjs that conflicts with arweave.
#46
It could make it easier to integrate elrond/erdjs with more dapps with fewer chances of conflict.