Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(http): add multiple parameters serializers #7423

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

Add the ability to add parameters serializers
Add common parameters serializers

Still needs quite some documentation changes, but putting it out to get feedback

Closes #7363

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7423)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

});
}

function jsonParametersSerializer(params, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(as far as I'm aware,) no backend expects a JSON query string, that would be crazy

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes sense to ship more than one of these in core, we should probably just keep doing what we're already doing in core, and give people the option to extend it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, that would be less code (and I always like less code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it makes sense to ship more than one of these in core, we should probably just keep doing what we're already doing in core, and give people the option to extend it

Mmm... not even the one that is asked at #7363 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want jQuery behaviour, you could easily just say

$httpProvider.querySerializer('jquery', jQuery.param);
$httpProvider.defaultQuerySerializer = 'jquery';

to use jQuery's style automatically --- or if you didn't want to include jQuery, you could probably come up with a variation on it

Add the ability to add parameters serializers
Add common parameters serializers

Closes angular#7363
@lgalfaso lgalfaso added cla: yes and removed cla: no labels May 12, 2014
@IgorMinar IgorMinar self-assigned this May 21, 2014
@IgorMinar
Copy link
Contributor

Instead of making the param serialization configurable, can't you just create an interceptor that will modify the config object by serializing the params, removing them from the config and updating the url?

@IgorMinar
Copy link
Contributor

I definitely don't like having many different strategies in the core. We have one strategy that works quite well for most of the people. If that's not the case then we should consider adopting a better serialization strategy. For corner cases, you should be able to use interceptors.

@IgorMinar IgorMinar added this to the Purgatory milestone May 21, 2014
@lgalfaso
Copy link
Contributor Author

@IgorMinar are you saying that the proposed strategy should be to add an interceptor that changes the param from

params: {
  foo: [1, 2],
  bar: { baz: 'abc' }
}

to

params: {
  "foo[]": [1, 2],
  "bar.baz" : 'abc'
}

?

@IgorMinar
Copy link
Contributor

No. Remove params altogether and set the url to contain the params in
whatever form you'd like.
On May 21, 2014 6:00 PM, "Lucas Galfasó" notifications@github.com wrote:

@IgorMinar https://github.com/IgorMinar are you saying that the
proposed strategy should be to add an interceptor that changes the param
from

params: {
foo: [1, 2],
bar: { baz: 'abc' }}

to

params: {
"foo[]": [1, 2],
"bar.baz" : 'abc'}

?


Reply to this email directly or view it on GitHubhttps://github.com//pull/7423#issuecomment-43836587
.

@lgalfaso
Copy link
Contributor Author

With the clarification from #7423 (comment) this is not going to be part of the core. Closing it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$http buildUri function not converting arrays properly
4 participants