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

fix($httpParamSerializerJQLike): Follow jQuery parameter serialization l...#11635

Closed
lgalfaso wants to merge 1 commit into
angular:masterfrom
lgalfaso:jquery-parameter-serialization
Closed

fix($httpParamSerializerJQLike): Follow jQuery parameter serialization l...#11635
lgalfaso wants to merge 1 commit into
angular:masterfrom
lgalfaso:jquery-parameter-serialization

Conversation

@lgalfaso

Copy link
Copy Markdown
Contributor

...ogic

Follow jQuery parameter serialization logic for nested objects.

Closes #11551

Comment thread test/ng/httpSpec.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

kay --> key

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@lgalfaso lgalfaso force-pushed the jquery-parameter-serialization branch from 57a82d8 to 089e246 Compare April 18, 2015 09:18
Comment thread src/ng/http.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor, but I think this can be just return parts.join('&');
In paramSerializer() too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@lgalfaso lgalfaso force-pushed the jquery-parameter-serialization branch from 089e246 to 551b63b Compare April 18, 2015 15:47
@lgalfaso

Copy link
Copy Markdown
Contributor Author

The travis failure looks like a flake

@gabrielmaldi

Copy link
Copy Markdown
Contributor

I think we need to make a special case for date parameters:

jQueryLikeParamSerializer({ dateFrom: new Date() }) === ""

Comment thread src/ng/http.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe } else if (isObject(toSerialize) && !isDate(toSerialize)) {?

…n logic

Follow jQuery parameter serialization logic for nested objects.

Closes angular#11551
@lgalfaso lgalfaso force-pushed the jquery-parameter-serialization branch from 551b63b to e77080f Compare April 19, 2015 17:39
@lgalfaso

Copy link
Copy Markdown
Contributor Author

@gabrielmaldi you are right, fixed this case

@gabrielmaldi

Copy link
Copy Markdown
Contributor

I thought this would make it into v1.4.0-rc.1 😢

Comment thread src/ng/http.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we are going to do this then we might as wel ditch the paramSerializerFactory altogether and just reference these two serializers directly in their respective providers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They only share serializeValue, maybe move a little more logic to this function but keep it as is?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's already refactored in 2420a0a

@petebacondarwin

Copy link
Copy Markdown
Contributor

One comment but otherwise LGTM

@lgalfaso lgalfaso closed this in 2420a0a May 5, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
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.

$httpParamSerializerJQLike serializing differently than jQuery.param

6 participants