feat($httpParamSerializerJQLike): honor object.toJSON function if present

[gabrielmaldi]

$httpParamSerializerJQLike gives dates special treatment (i.e. treats them as values instead of drilling down into their properties).

This change extends this behavior by honoring the toJSON function, if present on the object being serialized.

While keeping the existing behavior for dates, it also adds support for serializing custom objects which should be treated as values, such as Moment.js moments.

[pkozlowski-opensource]

It sounds like a good addition, but it definitively need tests before it can be merged. @gabrielmaldi could you please add tests for this change? You can use the existing ones as inspiration.

[gabrielmaldi]

I added one test for toJSON, think it's enough because there already are others which test object serialization.

[lgalfaso]

Given the breaking chance nature, I am leaning on moving this to 1.5. @petebacondarwin WDYT?

[gabrielmaldi]

I made an adjustment:

The call to serializeValue(toSerialize) returned a quoted string because JSON.stringify("baz") === ""baz"" (and then encodeUriQuery would turn that into %22baz%22).

So now we call toSerialize.toJSON() directly and get "baz".

[gabrielmaldi]

Hey guys, I rebased my changes. Now that 1.5.0-beta.1 is out maybe we can continue working on this PR?

[gabrielmaldi]

I found a problem when a Resource is serialized.

Resource defines toJSON here, but it doesn't return a string, it returns the same object (cloned) minus a couple of properties.

Since toJSON is defined, it falls into this case:

} else if (isFunction(toSerialize.toJSON)) {
  parts.push(encodeUriQuery(prefix) + '=' + encodeUriQuery(toSerialize.toJSON()));

And everything ends up with "[object Object]".

I didn't find a place where Resource.prototype.toJSON is called explicitly. I did find this test.

If toJSON is not used, perhaps we could remove it? If it is in fact used, it is weird that it doesn't conform to what toJSON is expected to do (i.e. return a string), but if we change that we may break things. We could rename Resource.prototype.toJSON to something else. Or make a special case for Resource in this PR:

} else if (isFunction(toSerialize.toJSON) && toSerialize.constructor.name !== "Resource") {
  parts.push(encodeUriQuery(prefix) + '=' + encodeUriQuery(toSerialize.toJSON()));

But this is very fragile because of the use of .constructor.name and also depending on "Resource" which could be another object.

What do you guys think?

Thanks!

[gkalpak]

If toJSON is not used, perhaps we could remove it?

It is used implicitly by JSON.stringify() when the object is converted to JSON. This is not something Angular-specific, it's plain old JavaScript.
See toJSON() on MDN.

If it is in fact used, it is weird that it doesn't conform to what toJSON is expected to do (i.e. return a string)

It does conform to what toJSON() is expected to do (see link above). The name isn't very intuitive indeed, but toJSON() is supposed to return the value to be serialized (and not the serialized value).

---

WRT to this PR in general, I am not sure if it is a good idea to take toJSON() into account, since it's purpose is very specific (to be used when converting to JSON) and we are doing something completely different (serializing an object as URL query params according to jQuery's conventions).

If jQuery does that (which I don't think it should - but that is irrelevant), we should do it too in order to be consistent. But if jQuery doesn't do it, then neither should we.

[gabrielmaldi]

I had read that MDN article but I guess that since in their example they return a string, it wasn't immediatly obvious to me that

the value returned by the toJSON() method when called will be serialized

meant that you should return

the value to be serialized (and not the serialized value).

Sorry about that and thanks for clarifying!

If jQuery does that (which I don't think it should - but that is irrelevant), we should do it too in order to be consistent. But if jQuery doesn't do it, then neither should we.

I ran some tests on jQuery and Angular with and without this PR:

---

jQuery

jQuery.param({
    myObject: {
        prop1: 'value1',
        toJSON: function () {
            return 'anotherValue';
        }
    }
});

"myObject%5Bprop1%5D=value1&myObject%5BtoJSON%5D=anotherValue"

jQuery.param({
    myObject: {
        prop1: 'value1',
        func1: function () {
            return { 'prop2': 'value2' };
        }
    }
});

"myObject%5Bprop1%5D=value1&myObject%5Bfunc1%5D=%5Bobject+Object%5D"

---

Angular without this PR

jqrSer({
    myObject: {
        prop1: 'value1',
        toJSON: function () {
            return 'anotherValue';
        }
    }
});

"myObject%5Bprop1%5D=value1&myObject%5BtoJSON%5D=function+()+%7B%0A++++++++++++return+'anotherValue';%0A++++++++%7D"

jqrSer({
    myObject: {
        prop1: 'value1',
        func1: function () {
            return { 'prop2': 'value2' };
        }
    }
});

"myObject%5Bfunc1%5D=function+()+%7B%0A++++++++++++return+%7B+'prop2':+'value2'+%7D;%0A++++++++%7D&myObject%5Bprop1%5D=value1"

---

Angular with this PR

jqrSer({
    myObject: {
        prop1: 'value1',
        toJSON: function () {
            return 'anotherValue';
        }
    }
});

"myObject=anotherValue"

jqrSer({
    myObject: {
        prop1: 'value1',
        func1: function () {
            return { 'prop2': 'value2' };
        }
    }
});

"myObject%5Bfunc1%5D=function+()+%7B%0A++++++++++++return+%7B+'prop2':+'value2'+%7D;%0A++++++++%7D&myObject%5Bprop1%5D=value1"

---

The first thing that emerges (from the very first test) is that jQuery doesn't use toJSON to get the value of the object.

However, it seems that jQuery also includes the values returned by any functions (it doesn't treat toJSON specially in this regard, it treats it like any other function), and if the function returns another object, it doesn't serialize it recursively.

Angular is currently not consistent with this behaviour. I don't like jQuery's behaviour either, I'd ditch serializing functions altogether. But if jQuery does it this way, should we too? I don't know if consistency with jQuery should be the only deciding factor here, because what it is doing with funcions is IMHO nonsense (what we are doing is too). Also, jQuery doesn't sort parameters while Angular does (should we switch from forEachSorted to forEach?), so we're not currently replicating jQuery's logic strictly.

If it were up to me, I wouldn't serialize functions and I'd take toJSON into account; after all, if toJSON is defined in an object, it means that you want another representation of that object to be used when serializing, either if you are serializing to JSON or to query string params. I think this is a reasonable assumption that devs would make.

I updated the PR with these changes and added a couple of tests. This also solves the problem with Resource (since we now support toJSON returning an object as well as a string), and continues to work fine with dates and Moment.js moments (which was the motivation behind this PR).

Thanks for your time!

[gkalpak]

Sorry for the late response. I still think we should follow jQuery as close as possible (especially since this paramSerializer has that exact purpose: replicate jQuery's serialization).

[gkalpak]

Closing since I believe we should try to keep as close as possible to the jQuery behavior (whether we like it or not 😉). This is the purpose of $httpParamSerializerJQLike anyway.
Treating functions correctly will be fixed with #16139.

Thx for investigating and working on this, @gabrielmaldi ❤️