_.clone(...) modifies target and context

[skleeschulte]

These lines (core.js lines 118-119) modify the target and context parameters:

this.target         = _.clone(target || {})
this.context        = _.clone(context || {})

This breaks for example passing a bookshelf model instance as context. After the model instance went through _.clone, it doesn't have a get method anymore. Is there a specific reason for using _.clone here? I think these statements should be replaced by target || {} and context || {} - the user can still decide to use _.clone.

[rhys-vdw]

Not sure, but you can work around it by passing the context as: { model: bookshelfModelInstance } since it's a shallow clone.

On Wed, 24 Feb 2016 at 09:50 Stefan Kleeschulte notifications@github.com
wrote:

These lines (core.js lines 118-119)

checkit/core.js

Lines 118 to 119 in 1b4ebea

	this.target = _.clone(target || {})
	this.context = _.clone(context || {})

modify the target and context parameters:

this.target = _.clone(target || {})
this.context = _.clone(context || {})

This breaks for example passing a bookshelf model instance as context.
After the model instance went through _.clone, it doesn't have a get
method anymore. Is there a specific reason for using _.clone here? I
think these statements should be replaced by target || {} and context ||
{} - the user can still decide to use _.clone.

—
Reply to this email directly or view it on GitHub
#56.

[skleeschulte]

Yep, that's what I do now. But for me this was unexpected behaviour. I created a pull request to change it if you think there is no reason for the _.clone. However this might be a breaking change for someone who relies on the implicit _.clone.

[rhys-vdw]

I think that we should keep the original behaviour here. If this were intended to be any data type it would not default to {}.

[skleeschulte]

_.clone does not work only with objects:

console.log(typeof _.clone(1));
// number

If the argument has to be an object there should rather be a check like this:

if (typeof xyz !== 'object') throw new TypeError('Argument xyz must be an object.');

But there doesn't seem to be any problem with passing non-object values to context:

checkit.run(body, 1);

This runs without any problems, and I can access the value 1 in a custom validator:

Checkit.Validator.prototype.myValidator = (value, context) => {
    // context === 1
};

Even checkit.run(1, 1); runs without complaining, although this doesn't really make sense. I'll update my pull request, at least think about accepting it. ;)

[rhys-vdw]

My comment above was referring to the default value of the context attribute - an empty object. This suggests that code interfacing with Checkit should assume that it is a POJO.

But for me this was unexpected behaviour.

I'd prefer to update the documentation instead of the behaviour here.

The rationale of this code is that we can't assume that the client will not mutate the context object after passing it as an argument. Personally I don't know if this is necessary, but I don't see any reason to introduce a breaking change here.