qx.data.marshal.Json | ignore non-pojo objects when marshalling

[p9malino26]

This PR causes non-POJO objects to be ignored from the POJO that we pass into qx.data.marshal.Json.createModel. This behaviour is almost always undesired because we may have third party library objects in the POJO, which we don't want the marshaller to touch.

Here is a demo qx browser app. Try running it on both this PR branch and master and compare the results.
pr-demo.zip

[cboulanger]

I think the PR makes sense, but it certainly is backward incompatible. There might be users that rely on the expectation that their custom objects will be serialized. I would suggest that the PR is part of the upcoming breaking release and it should include a message in the change log and maybe a note in the documentation.

[derrell]

I think this is a useful addition, but I would go a step further than @cboulanger suggested, and add an additional parameter, strictlyPojo, for createModel, toModel, and toClass that could be passed when this new behavior is desired. The current behavior should remain the default if no changes are made to callers of these methods.

[derrell]

see my prior general comment

[derrell]

I think we also probably need new tests that (a) test to ensure that the original behavior is retained if the original call arguments are passed (a value for strictlyPojo is not passed), and (b) tests of the new functionality showing that objects are marshalled as expected with both settings of strictlyPojo.

[johnspackman]

isPojo does not need to be in qx.Bootstrap, which is effectively an internal class that exists in order to work around dependency management issues - IMHO it should move to qx.lang.Type

[johnspackman]

it would be helpful to explain the net effect - eg: "Objects can be either a POJO (ie {}) or an object which is created from a ES6-style class or prototypical-inheritance-based class; if you need to determine whether something is a POJO and not created from a class, use isPojo instead"

[johnspackman]

Same as above, the net effect is more useful in documentation than repeating the code, eg "Detects whether an object is just a plain POJO, ie {}; note that this excludes objects created via classes - @see qx.lang.Type.isObject

[johnspackman]

I agree that there should be unit tests for both isPojo and the marshalling, but I'm inclined to say that it is actually a bug for the marshalling to try and marshall class instances; the reason that this came up is because we're using BigNumber.js, and any kind of instance which has internal class implementation cannot reasonably be marshalled - for example, in the case of new BigNumber("567.89") you get an object with { s: 1, e: 2, c: [ 567, 890000000 ] } whereas JSON.stringify returns "567.89"

We could use an environment setting to preserve the old behaviour if required (the default being to use the new isPojo)?

Also there should be an explanation & warning in the release notes

[oetiker]

I agree that there should be unit tests for both isPojo and the marshalling, but I'm inclined to say that it is actually a bug for the marshalling to try and marshall class instances; the reason that this came up is because we're using BigNumber.js, and any kind of instance which has internal class implementation cannot reasonably be marshalled - for example, in the case of new BigNumber("567.89") you get an object with { s: 1, e: 2, c: [ 567, 890000000 ] } whereas JSON.stringify returns "567.89"

We could use an environment setting to preserve the old behaviour if required (the default being to use the new isPojo)?

Also there should be an explanation & warning in the release notes

how about adding the feature in the next 7.x release but with the default behavior unchanged except that warnings would be emitted when foreign objects are encountered with the suggestion to enable the new mode ?

[johnspackman]

how about adding the feature in the next 7.x release but with the default behavior unchanged except that warnings would be emitted when foreign objects are encountered with the suggestion to enable the new mode ?

may be make it deprecated first, so the new behaviour is enabled by an environment setting and issues a deprecation warning with foreign objects? And then make it "standard" in future?

I'm struggling to see when it would be intentional to serialise a properly defined Object (ie instance of an ES6 class) into a JSON structure when marshalling; what happens at the moment is that an actual ES6 Object is downgraded to JSON, when (imho) marshalling is about upgrading to an object model

eg this data:

qx.data.marshal.Json.createModel([
  { title: "Lunch", price: new BigNumber(12.34) },
  { title: "Dinner", price: new BigNumber(23.45) }
]);

Is kind of equivalent to:

qx.Class.define("mypkg.MyClass, {
  properties: {
    title: { check: "String" },
    price: { check: "Object" }
});
new mypkg.MyClass().set({ title: "Lunch", price: { s: 1, e: 2, c: [ 12, 340000000 ] } });
new mypkg.MyClass().set({ title: "Dinner", price: { s: 1, e: 2, c: [ 23, 450000000 ] } });

[derrell]

The current implementation of isPojo prevents object x here from being serialized. It is completely reasonable, I believe, to serialize x. As I think about this issue, I'm leaning towards John's expectation, but isPojo needs a bit of change as it's too limiting.

function isPojo(value)
{
  return (
    value !== undefined &&
    value !== null &&
    Object.getPrototypeOf(value) === Object.prototype);
}

const prototype = { num : 42 };
let x = { text : "hello", __proto__ : prototype };

console.log("prototype:", isPojo(prototype));
console.log("x:", isPojo(x));