Concat Descriptor Properties (proposal)

[unstoppablecarl]

Concat Descriptor Properties Proposal

Add descriptor properties:

• concatProperties
• concatStaticProperties

(naming consistent with deepProperties and deepStaticProperties).

Reasoning

The descriptor spec currently has many different properties on it for handling different types of composition. I think concatenating arrays when composing is a low level behavior equivalent in specificity of use case to deepProperties and deepStaticProperties. Array concatenation is the the only basic / low level way of composing I can think of that is absent in the descriptor spec.

Spec Changes

Add descriptor properties:

• concatProperties
• concatStaticProperties

Example Code

Created Branch:
https://github.com/stampit-org/stamp-specification/tree/concat-properties

Implemented proposed changes: https://github.com/stampit-org/stamp-specification/blob/concat-properties/examples/compose.js

Tested Use Case:

import compose from './examples/compose.js';

const Jsonable = compose({
    concatStaticProperties: {
        toJsonConverters: [],
        fromJsonConverters: [],
    },
    staticProperties: {
        addToJsonConverter(func) {
            console.log('z', this.compose.concatStaticProperties);
            this.compose.concatStaticProperties.toJsonConverters.push(func);
            return this;
        },
        addFromJsonConverter(func) {
            this.compose.concatStaticProperties.fromJsonConverters.push(func);
            return this;
        },
        // convert object instance to json
        toJSON(obj){
            return this.compose.concatStaticProperties.toJsonConverters.reduce(function(json, func, index, array){
                // return new or mutate json object
                return func(obj, json) || json;
            }, {});
        },

        // prepares options object then calls factory
        fromJSON(json){
            const converters = this.compose.concatStaticProperties.fromJsonConverters;

            var preparedOptions = converters.reduce(function(options, func, index, array){
                // return new or mutate options object
                return func(options, json) || options;
            }, {});

            return this(preparedOptions);
        },
    },
    initializers: function (opts, {stamp}) {
        this.toJSON = function () {
            let converters = stamp.compose.concatStaticProperties.toJsonConverters;
            let obj = this;
            console.log('toJSON converters', converters);
            return converters.reduce(function(json, func, index, array){
                // return new or mutate json object
                return func(obj, json) || json;
            }, {});
         };
      }

});

const A = compose({ properties: {A: 'A'} })
.compose(Jsonable)

// console.log(A.addToJsonConverter);
.addToJsonConverter(function toJA(obj, json) {
    json.A = obj.A + ' as JSON';
})
.addFromJsonConverter(function fromJA(options, json) {
    options.A = json.A.replace(' as JSON','');;
});

const B = compose({ properties: {B: 'B'} })
.compose(Jsonable)
.addToJsonConverter(function toJB(obj, json) {
    json.B = obj.B + ' as JSON';
})
.addFromJsonConverter(function fromJB(options, json) {
    options.B = json.B.replace(' as JSON','');;
});

const C = compose(A, B);

console.log('C', C.compose.concatStaticProperties);

var c = C();
console.log('c', c);
var json = c.toJSON();
console.log('c.toJSON()', json);

console.log('C.fromJSON()', C.fromJSON(json));

[koresar]

Any code samples?

[koresar]

Thanks for the update @unstoppablecarl
Looks okay to me. Waiting for other opinions.

[koresar]

I gave it a deep thought. Maybe we should reconsider using the "lodash/merge" function. Let's just use mergeWith: https://lodash.com/docs#mergeWith

Let me copy&paste the example from the link above:

function customizer(objValue, srcValue) {
  if (_.isArray(objValue)) {
    return objValue.concat(srcValue);
  }
}

var object = {
  'fruits': ['apple'],
  'vegetables': ['beet']
};

var other = {
  'fruits': ['banana'],
  'vegetables': ['carrot']
};

_.mergeWith(object, other, customizer);
// → { 'fruits': ['apple', 'banana'], 'vegetables': ['beet', 'carrot'] }

So, my proposal: do not add any new Descriptor Properties. But replace the "merge" with the "mergeWith" code above.

• Not inflating the specs.
• Not complicating implementation.
• I have never used arrays inside the ".merge.*" object, it used to be useless. Not anymore!

[unstoppablecarl]

So you are proposing that we assume all arrays are intended to be concatenated when composing? I think that is just as bad as intending that they are replaced. Probably worse as I think you would more often want replacement.

You need to have the option for both. This is why we have properties and deepProperties. The whole point of stamps to me is a solution that does not make any strong assumptions about use but provides you options to leverage what you want. Things will be done differently depending on developer and project needs.

[koresar]

Nope. That's not what I meant. Let me get to a computer to show some code.

On Wed, 17 Feb 2016 06:33 Carl Olsen notifications@github.com wrote:

So you are proposing that we assume all arrays are intended to be
concatenated when composing? I think that is just as bad as intending that
they are replaced. Probably worse as I think you would more often want
replacement.

You need to have the option for both. This is why we have properies and
deepProperties. The whole point of stamps to me is a solution that does
not make any strong assumptions about use but provides you options to
leverage what you want. Things will be done differently depending on
developer and project needs.

—
Reply to this email directly or view it on GitHub
#66 (comment)
.

[koresar]

• I'm not talking about "properties" or "staticProperties".
• It's about deeply merged items - "deepProperties" and "deepStaticProperties".
• Deep merging of two arrays currently happens index-by-index:

let deep1 = compose({deepProperties: { key: ["a", "b", "c"] } });
let deep2 = compose({deepProperties: { key: [42, 42] } });
let composed = compose(deep1, deep2);
console.log(composed.compose.deepProperties); // { key: [ 42, 42, 'c' ] }

In other words, array indexes are like POJO keys.

let deep1 = compose({deepProperties: { key: {0: "a", 1: "b", 2: "c"} } });
let deep2 = compose({deepProperties: { key: {0: 42, 1: 42} } });
let composed = compose(deep1, deep2);
console.log(composed.compose.deepProperties); // { key: { '0': 42, '1': 42, '2': 'c' } }

I find it useless. For the past 9 months since stampit v2 was released I have never ever used arrays inside the deepProperties just because of that.

• However, I was constantly finding that array concatenation would help me from time to time.

Proposal: arrays inside the deepProperties and deepStaticProperties should be concatenated instead of merged.

[unstoppablecarl]

I was unaware of merging of objects in arrays in _.merge(). I thought it was the same as assign but make new instances of all objects via Object.assign({}, obj). This is how I think deepProperties should behave. I would expect and want properties with an array value to replace. This array merging behavior is a strange and surprising. I cannot think of a use case for. I know I was out of the loop when this was discussed.

I would very much like to see this behavior removed from the spec. I think this merge behavior is much more specific and obscure than the array concatenation I am proposing.

Proposal: arrays inside the deepProperties and deepStaticProperties should be concatenated instead of merged.

As I have said before: I think the default behavior should be to replace and concatenate should be an option. I think concatenation would just be a different surprising behavior of deepProperties.

I think for completeness the spec should include the following simple reasonably common low level object composition behaviors:

Core

• methods: Object.create()
• initializers: [].concat() called at instance creation
• configuration: recursive Object.assign()

Instance

• properties: Object.assign()
• deepProperties: recursive Object.assign()
• propertyDescriptors: Object.assign() then Object.defineProperties(obj, propertyDescriptors);
• concatProperties: [].concat() of all arrays with matching property name

Static (same abilities as instance but merged into stamp instead of instance)

• staticProperties
• deepStaticProperties
• staticPropertyDescriptors
• concatStaticProperties

[koresar]

I would expect and want properties with an array value to replace.

Use properties. :)

This array merging behavior is a strange and surprising.

It is not surprising to "underscore/lodash" users though. We are using _.assign and _.merge for properties and deepProperties.

properties: Object.assign()
deepProperties: recursive Object.assign()

WAT? :) Then what's the difference?

[unstoppablecarl]

Recursive Object.assign() would be like _.merge() without the strange array merge behavior.

[koresar]

@unstoppablecarl could you elaborate on Recursive Object.assign() with some code maybe?

[unstoppablecarl]

This is what I would expect merge to do with arrays by default.

function merge(obj1, obj2){
    return _.mergeWith(obj1, obj2, function(v1, v2){
        if (_.isArray(v2)) {
            return v2;
        }
    });
}

I just discovered _.merge will merge objects and arrays.

_.merge({foo: 'bar'}, ['a','b','c']); // {0: "a", 1: "b", 2: "c", foo: "bar"}

I do not think I would ever want this result.

[unstoppablecarl]

Even stranger:

_.merge({foo: {bar: 'bam'}}, {foo: ['a','b','c']}); // {foo: ['a','b','c']}

So this should not affect stampit as it should never be merging arrays unless they are properties of an object.

[koresar]

Asked the question here: lodash/lodash#2064

[ericelliott]

I like the idea of passing a customizer for specialized merge behavior, rather than bloating the spec further, but where is the customizer decided? In any stamp? What happens if different stamps propose different customizers?

[unstoppablecarl]

I think #69 is a better solution to this problem.

[ericelliott]

Agreed.