Skip to content
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 params merging #2196

Closed

Conversation

zackseuberling
Copy link

@zackseuberling zackseuberling commented May 31, 2019

Fixes bug introduced in 0.19.0 where request params are not being merged correctly with default instance params. See bug #2190 for more information

According to the documentation:

The available instance methods are listed below. The specified config will be merged with the instance config.

Tests were failing because of the change to merge params, so hopefully my update to the tests is correct … and passes

@nothingismagick
Copy link

Nice job @zackseuberling

Copy link
Member

@jasonsaayman jasonsaayman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilyemorehouse, I tested this one locally and it seems good. I do agree with @zackseuberling that there has been a change from 0.18 to 0.19. You can run it in over here and see the behavior is different.

@kodmunki
Copy link

kodmunki commented Jun 2, 2019

0.19.0 is breaking our services. The root cause appears to be what this is fixing. What kind of timeline on a merge and release? 🤓🍻

@jasonsaayman
Copy link
Member

@kodmunki I cannot merge the changes, we will need to wait for @emilyemorehouse. I'm sure it will be done soon.

@lsagetlethias
Copy link

I think maybe this PR should be passed alongside this one: #2207 because more than one BC was induced by mergeConfig function.

@samerdernaika
Copy link

plz merge it

@code-a-cola
Copy link

Do we have an update as to when this is getting merged?

@nothingismagick
Copy link

nothingismagick commented Aug 21, 2019

I would just like to say that we (at quasarframework) are still recommending that people pin 0.18.1, and I have to agree that @code-a-cola is right - it would be good to update us on this issue / PR.

@ibrainventures
Copy link

@emilyemorehouse .. thanks for your work on this project .. it qould be great and neefull to see this pr merged! :-)

douira added a commit to douira/vodafone-api-crawl that referenced this pull request Aug 30, 2019
since they are taking a really long time
to merge the fix over at axios/axios#2196
@avindra
Copy link
Contributor

avindra commented Sep 14, 2019

I ran grunt karma:single against master after #2207 got merged.

@zackseuberling your test breaks against master, so PR 2207 seemed to be solving another issue:

Chrome 77.0.3865 (Linux 0.0.0) core::mergeConfig should merge auth, headers, params, proxy with defaults FAILED
	Expected $.params to have properties
	    foo: 'test'
	    at UserContext.<anonymous> (webpack:///test/specs/core/mergeConfig.spec.js:50:78 <- test/specs/core/mergeConfig.spec.js:96:83)
................................................................................
.....................................
Firefox 68.0.0 (Linux 0.0.0) core::mergeConfig should merge auth, headers, params, proxy with defaults FAILED
	Expected $.params to have properties
	    foo: 'test'
	@webpack:///test/specs/core/mergeConfig.spec.js:50:78 <- test/specs/core/mergeConfig.spec.js:96:83
................................................................................

lsagetlethias added a commit to lsagetlethias/sdkore that referenced this pull request Sep 17, 2019
@Alanscut
Copy link
Collaborator

@zackseuberling , code is updated, please resolve the conflicts, thanks!

@yasuf
Copy link
Collaborator

yasuf commented Nov 22, 2019

would you mind adding tests to this change?

@yasuf
Copy link
Collaborator

yasuf commented Nov 22, 2019

and fixing the existing tests

@zackseuberling
Copy link
Author

zackseuberling commented Nov 24, 2019

@yasuf I can no longer get the test suite to run for me, so it is quite hard for me to fix or change any tests. I rebased and force-updated so I lost the changes to tests I had written earlier.

@chinesedfan chinesedfan changed the title Fix param merging Fix params merging Jan 10, 2020
@textbook textbook mentioned this pull request Jan 12, 2020
@chinesedfan
Copy link
Collaborator

Closed in favor of #2656. Cheers.

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet