Modify the object version of toInclude

[wuct]

This PR introduces following changes:

• Traverse both enumerable name keys and enumerable symbol keys.

  expect({a: 1}).toInclude({[Symbol()]: 1}) // fail

• Always pass a assertion with an empty object as an expected value.

  expect({a: 1}).toInclude({}) // pass

BREAKING CHANGE: compare an empty object property to its corresponding actual property.

Before:
expect({}).toInclude({a: {}}) // pass

After:
expect({a: 1}).toInclude({a: {}}) // fail

Closes #82

[wuct]

It is a rebased and squashed version of #83. I also rewrote the commit message.

[ljharb]

@wuct Thanks! It'd have been preferable to modify the original PR instead of opening a new one, but this will do :-)

[wuct]

@ljharb Thank you, too! I have learnt a lot from you 😊
Github automatically closed the origin PR while I was doing rebase and squash. I am still not sure what triggers Github to close a PR.

[ljharb]

@wuct afaik, only pushing a branch deletion to the remote will trigger the PR to close. You need to instead, actually force push, in order to update a PR in-place.

[ljharb]

@mjackson @elado Do you consider this a breaking change, or a bugfix on the initial toInclude support?

I originally classified it as a breaking change because it will throw in cases where it previously did not - however, in those cases, I suspect users want it to throw, meaning this would be revealing failing tests.

[elado]

From #82:

According to the doc, (object) toInclude uses assert.deepEqual by default. I expect the following assertion should fail but it does not.
expect({ a: { b: 4 }}).toInclude({ a: {}})

I don't expect this to fail. The original object includes a key a and the original object.a value contains {}. It's not supposed to deep equal the inner values but keep matching keys and values, in this case the expected.a is an object with no keys, so there's nothing to compare, therefore, it shouldn't fail.

[ljharb]

@elado in your situation i would not expect it to fail. However, if a was a non-object on the LHS and an empty object on the RHS, i would: in other words, expect({ a: 1 }).toInclude({ a: {} }) should fail.

[ljharb]

Actually, perhaps not - as long as the value on the LHS is not null or undefined, it might indeed become autoboxed - ie, expect({ a: 'b' }).toInclude({ a: { length: 1 } }) should pass.

If @elado concurs, then @wuct, can you change this so that an empty object on the RHS only fails (for any key) when the LHS is null or undefined?

[elado]

I think expect({ a: 1 }).toInclude({ a: {} }) should fail (simple type check, compare partial objects only if both are objects) and also expect({ a: 'b' }).toInclude({ a: { length: 1 } }) (can't compare a string with an object - I don't think this case of checking length is common)

So what might be missing is the type check on both values.

[ljharb]

That seems equally reasonable to me.

[wuct]

I agree with @elado. We should compare partial objects only when both types are "object". I can do this modification.

[wuct]

@ljharb @elado @mjackson I just updated the code according to our discussion. Please let me know if there is any thing not expected.

[mjackson]

Perhaps we could call this ownEnumerableKeys to avoid confusing it with Reflect.ownKeys?

[ljharb]

isn't it meant to be a polyfill for Reflect.ownKeys?

[mjackson]

@ljharb Not AFAICT. It restricts the result of Reflect.ownKeys to enumerable keys.

[ljharb]

ah, k

[wuct]

Thanks for merging :)