Stamp function accept refs

[koresar]

@ericelliott @troutowicz @unstoppablecarl Please, review.

The main change is the object instantiation. See function Factory().

Now all the references mixed into an object instance as is was before. But then the props are safely merged into the instance. "Safely" means that no refs data will be overriden, but props will be merged to the existing properties where possible. See the unit test of this PR.

In my opinion this is the very best approach for all existing user cases I have ever met so far.

[troutowicz]

algorithm

[koresar]

Fixed comments above. Thanks guys!

[unstoppablecarl]

I don't think ignoring existing properties is a good idea.

• It breaks backward compatibility
• does not follow our consistent 'last in' behavior
• I think most would be surprised to find properties ignored when creating an object instance with a stamp.

We need a name for this arg. It should be referred to as settings (or something else) to avoid confusing it with props, it is difficult to talk about it and compare it to props if it is also called props.

I am starting to question if the settings in var instance = myStamp(settings) should be merged at all. It may be making too many assumptions about usage. I would rather see it passed to the init functions and let the developer decide how it is used. init({ args: args, instance: instance, stamp: factory, settings: settings } (where settings is the refs arg above). I don't think we can make the same assumptions of purpose or intent with settings as we can with refs, init, props, and static.

I realize this would be a serious breaking change to backward compatibility and therefore probably not wise but I wanted to see what others thought. Would this make sense if backward compatibility were not an issue ?

I would like to see a way to circumvent the settings merge behavior. I have some ideas I will put in a PR.

[koresar]

I don't think ignoring existing properties is a good idea.

What "ignoring" are you referring to? The code does not ignore anything.

[koresar]

@unstoppablecarl #50 might give you some insight maybe.

[unstoppablecarl]

"Safely" means that no refs data will be overridden, but props will be merged to the existing properties where possible

I assume this means properties matching refs will be ignored?

[unstoppablecarl]

We discusses this here and agreed that the settings object should be shallow merged #57

#57 (comment)

[koresar]

Please, do not use any new terminology to replace the existing. What do you mean by settings?

[unstoppablecarl]

var instance = myStamp(settings) what should I call it to differentiate from props?

[koresar]

These are refs. Please, check the code - the variable name reflects the usage.
Also check the changes to the README. It also says refs or references literally everywhere.

[koresar]

I assume this means properties matching refs will be ignored?

Nope, they will be not. "props" will be merged into an instantiated object which already has all the refs mixed in. Pseudo code:

var instance = Object.create(...);
_.mixin(instance, refs);
_.merge(instance, props);

[unstoppablecarl]

I didn't completely understand the intended behavior. I get it now that I did some testing.

This is the only strange behavior I found but I think it is acceptable given the way js works.

var stamp = stampit().props({
    connection: 'my connection',
});

var instance = stamp({connection: undefined});
console.log(instance); // { connection: 'my connection' }

[koresar]

I'll merge this tomorrow if no objections.

[ericelliott]

Typo? Should be "merged from props."

[koresar]

👍 fixed

[koresar]

@unstoppablecarl The strange behavior you found can be easily fixed.

Your code:

var stamp = stampit().props({
    connection: 'my connection',
});

var instance = stamp({connection: undefined});
console.log(instance); // { connection: 'my connection' }

Replacing undefined with null fixes it:

var stamp = stampit().props({
    connection: 'my connection',
});

var instance = stamp({connection: null});
console.log(instance); // { connection: null }

[koresar]

Since Eric has done his part I'm merging this in.

@ericelliott, please publish to NPM.

Meanwhile, I'll work on the Advanced Examples.

[ericelliott]

var stamp = stampit().props({
    connection: 'my connection',
});

var instance = stamp({connection: undefined});
console.log(instance); // { connection: 'my connection' }

That is a confusing example. Normally you only want to override a property if the property is set in the override object. By passing in undefined, you're explicitly stating that the property is not set, so if there is a default value, as is the case here, then the default value should be used, which is exactly what happens.

As @koresar pointed out, if you want to override it with a null value, you should pass null instead of undefined. If you want to unset the key entirely, use an .init() function and the delete keyword, or compose from a stamp that doesn't contain the unwanted key in the first place. ;)

[unstoppablecarl]

I understand all of that. As I said before I think it is acceptable given the way js works.

[ericelliott]

:)