Type check for parsePath variable would be nice to have

[sverrirs]

When calling the parsePath function (https://github.com/chaijs/chai/blob/master/chai.js#L10117) type checking can help users identify incorrect usage of API.

Problematic line:

function parsePath(path) {
   var str = path.replace(/([^\\])\[/g, '$1.[');

When running this (slightly incorrect) code:

expect(data).to.have.nested.property({'a':'1'})

The following error is thrown:

TypeError: path.replace is not a function

Because path in this case is an object {'a':'1'}
Would be nice to throw a more descriptive error :)

[keithamus]

Hey @sverrirs thanks for the issue

You're totally right, so I've raised this as a bug with a pull-request-wanted label. Our property assertion should only accept strings - so we should raise the error there. Here's the assertion code:

chai/lib/chai/core/assertions.js

Lines 1759 to 1825 in dbeae08

	function assertProperty (name, val, msg) {
	if (msg) flag(this, 'message', msg);
	var isNested = flag(this, 'nested')
	, isOwn = flag(this, 'own')
	, flagMsg = flag(this, 'message')
	, obj = flag(this, 'object')
	, ssfi = flag(this, 'ssfi');
	if (isNested && isOwn) {
	flagMsg = flagMsg ? flagMsg + ': ' : '';
	throw new AssertionError(
	flagMsg + 'The "nested" and "own" flags cannot be combined.',
	undefined,
	ssfi
	);
	}
	if (obj === null || obj === undefined) {
	flagMsg = flagMsg ? flagMsg + ': ' : '';
	throw new AssertionError(
	flagMsg + 'Target cannot be null or undefined.',
	undefined,
	ssfi
	);
	}
	var isDeep = flag(this, 'deep')
	, negate = flag(this, 'negate')
	, pathInfo = isNested ? _.getPathInfo(obj, name) : null
	, value = isNested ? pathInfo.value : obj[name];
	var descriptor = '';
	if (isDeep) descriptor += 'deep ';
	if (isOwn) descriptor += 'own ';
	if (isNested) descriptor += 'nested ';
	descriptor += 'property ';
	var hasProperty;
	if (isOwn) hasProperty = Object.prototype.hasOwnProperty.call(obj, name);
	else if (isNested) hasProperty = pathInfo.exists;
	else hasProperty = _.hasProperty(obj, name);
	// When performing a negated assertion for both name and val, merely having
	// a property with the given name isn't enough to cause the assertion to
	// fail. It must both have a property with the given name, and the value of
	// that property must equal the given val. Therefore, skip this assertion in
	// favor of the next.
	if (!negate || arguments.length === 1) {
	this.assert(
	hasProperty
	, 'expected #{this} to have ' + descriptor + _.inspect(name)
	, 'expected #{this} to not have ' + descriptor + _.inspect(name));
	}
	if (arguments.length > 1) {
	this.assert(
	hasProperty && (isDeep ? _.eql(val, value) : val === value)
	, 'expected #{this} to have ' + descriptor + _.inspect(name) + ' of #{exp}, but got #{act}'
	, 'expected #{this} to not have ' + descriptor + _.inspect(name) + ' of #{act}'
	, val
	, value
	);
	}
	flag(this, 'object', value);
	}

And here's an example of how we can do type-checking to ensure that name is a string:

chai/lib/chai/core/assertions.js

Lines 1152 to 1165 in dbeae08

	if (!doLength && (objType === 'date' && nType !== 'date')) {
	errorMessage = msgPrefix + 'the argument to above must be a date';
	} else if (nType !== 'number' && (doLength || objType === 'number')) {
	errorMessage = msgPrefix + 'the argument to above must be a number';
	} else if (!doLength && (objType !== 'date' && objType !== 'number')) {
	var printObj = (objType === 'string') ? "'" + obj + "'" : obj;
	errorMessage = msgPrefix + 'expected ' + printObj + ' to be a number or a date';
	} else {
	shouldThrow = false;
	}
	if (shouldThrow) {
	throw new AssertionError(errorMessage, undefined, ssfi);
	}

So effectively, we'll need to add the following the property assertion:

if (typeof name !== 'string') {
  var msgPrefix = flag(this, 'message')
  msgPrefix = msgPrefix ? msgPrefix + ': ' : ''
  throw new AssertionError(msgPrefix + 'the argument to `property` must be a string')
}

Anyone who wants to is welcome to make this PR; if you do decide you want to make this PR please add a comment below so others do not work on it at the same time!

[meeber]

The value only needs to be a string if the nested flag is set. If the nested flag isn't set, the assertion should also accept Symbols:

const prop = Symbol();
expect({[prop]: 4}).to.have.property(prop);

Arguably, it should accept numbers too, even though there is type coercion:

expect({42: 'secret'}).to.have.property(42);

[solodynamo]

Hey @keithamus , I would like to give a try to this.