stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Specify details on how are things deeply merged #101

Closed danielkcz closed 8 years ago

danielkcz commented 8 years ago

This is very vague here as it was simply assuming use of lodash. However in case someone wants to make their own implementation, it should be clearly specified how this merge behaves. Concretely with stampit which now uses own merge function, it does clone functions as a set of properties instead of referencing them (see related issue).

I think that functions should be exception on deep cloning for sure. Do you have other opinion?

Might be also related to the Symbol support #86 which talks mainly about cloning Symbol when used instead of properties names. However Symbol reference should be kept intact even when used as value.

koresar commented 8 years ago

My mental model of stamps is following:

Thus, the metadata collection is the corner stone of the stamps philosophy. We should not limit people on how they merge metadata. I'm thinking of allowing people to overwrite the mergeComposable similar to the initializers way. In other words - if user specified a merging callback function and it returned non-undefined then use the returned value as a new descriptor. (Will provide code examples later.)

danielkcz commented 8 years ago

One thing is metadata as bunch of objects/arrays of primitives, but function (or Symbol) is something way different. As it cannot be cloned, it needs to stay in place untouched unless something else wants to put another kind of data in same spot (even another function). In that case it simply forgets completely about that function and new data are used. That's how lodash is doing it (see http://jsfiddle.net/ybem606s/3/).

I think we should adopt that behavior. It's something that sane person would expect. Imagine you use library for first time and suddenly your function is cloned as a bunch of properties. Some people would look into documentation and they would find there "you need to override merger" instruction. I believe most of people gets disgusted and just goes away from it because compared to plain&simple classes, it adds up another unwanted hassle for something trivial like this.

koresar commented 8 years ago

Totally agree with that. I will fix the merger later this week (sorry, busy atm). But yeah, functions should not be treated as a mergable objects.

danielkcz commented 8 years ago

Well this issue isn't about fixing merger here, but to actually expand specification and say how is deep merging behaving. But I am glad you agree ;)

koresar commented 8 years ago

Okay, here is the proposed merging algorithm /cc @ericelliott @unstoppablecarl @troutowicz


  1. Plain objects are deeply merged.
  2. Array are concatenated.
  3. Everything else (including Symbols) is copied by assignment.

The first two are already part of the specs. The third item is the new addition.

Opinions?

koresar commented 8 years ago

Since there are no objections I'll implement exactly that.

danielkcz commented 8 years ago

I definitely agree with it. Just encountered another use case where my function disappeared after merge.