Fix merging of params

[textbook]

Move 'params' to the mergeDeepPropertiesKeys array, so that the specific parameters for the request get merged with the defaults, rather than overwriting them. This allows e.g. setting authentication params for a specific API in a central location, and matches the description in the README that "The specified config will be merged with the instance config."

Also the existing merging tests repeated the same case twice and didn't actually cover the described behaviour, so I've fixed that.

Fixes #2190 and various linked issues, supersedes #2196.

[jonathanalberghini]

PS this is still broken in 19.1

[textbook]

@jonathanalberghini I opened this after 0.19.1 was released, so that's more-or-less inevitable...

[elboletaire]

A lot of people stuck with version 0.18.1 due to this regression added almost a year ago. I hope it's merged soon.

[textbook]

@yasuf any feedback on this? I'm happy to make updates if needed.

[elboletaire]

Sorry but how is this still open? A new version already published and this regression added in 0.19.0 here still waiting to be merged...???

PS. And we're talking about a regression added ONE YEAR AGO 😕

[elboletaire]

🎉 🎉 🎉

[alimek]

when we can expect this to be released to NPM ?

[mateuszmazurek]

@textbook @yasuf @chinesedfan it can't be released to npm in this form because it brokes compatibility with existing code.

Before this fix:
Param with value of null was omitted

Now:
Param with value of null is replaced with "{}"

Example:
axios.get('/whatever', {params: {test: null}}) used to make GET request to /whatever, now it's /whatever?test=%7B%7D

[textbook]

@mateuszmazurek what's the use case for a null-valued parameter rather than undefined? The current version is breaking a lot of workflows, it wouldn't be unreasonable for the maintainers to take a view on regressing in an edge case.

[mateuszmazurek]

@textbook I wouldn't say it's edge case. Here's how I see it:

interface IState {
 something: null | ISomething;
 // ...
}

interface ISomething {
 value: string;
 smthelse: boolean;
 // ...
}

state: IState = {
 something: null // default state, can be overriden somewhere else
}

Now I want to pass state.something.value param only if something is not null (don't want to have "cannot read property value of null" exception obviously). My existing code looks like this:

const something = state.something && state.something.value;
// ...
axios.get('/whatever', {params: {something}})

And it works. After the change it won't pass my backend validation because something is optional, but if sent should be, let's say for example, 5 characters long. Or any other rule.

Who knows how many cases like this depentends of axios have in theirs codebases. I confirmed 1, but didn't do any regression, would probably find more.

Moreover - buildURL helper explicitly checks param value and if it's null - omit it (https://github.com/axios/axios/blob/master/lib/helpers/buildURL.js#L38). After change it will never be null, so it doesn't make any sense.

Just please be very careful with your versioning policy.

[textbook]

@mateuszmazurek that's why I mentioned undefined; for that case I'd expect:

interface IState {
 something?: ISomething;
  ...
}

It would work with the same truthiness check, and not require a null default.

[mateuszmazurek]

@textbook it doesn't matter why it is null, not undefined. Of corse in new code I can pass undefined, but there's a lot of existing code which works well and after your change it won't = there should be new major, that's all imho.

[chinesedfan]

@mateuszmazurek It's better to open a new issue to discuss. I opened #2819.

And of course this PR will break something, which will be released in 0.20 at least. Note that in semantic versions less than 1.0.0, a minor upgrade (the second version number) will also bring breaking changes.

[paztis]

This PR need to be cherry-pick to a release/0.19.X branch also, but seems not to exist...
You need to cut a branch from last 0.19 commit and put the fix inside.

[jasonsaayman]

Once again that is not the current plan, we will be releasing v0.20.0 once it is ready and free of defects and regression.

[paztis]

Is there a target date for the 0.20 release ?

[jasonsaayman]

I am working on that and will try give you an indication as soon as possible, watch the project and milestones :)