Fix 'empty' assertion called on null

[shvaikalesh]

Because of typeof null thing and ToObject in Object.keys, TypeError was thrown.

[meeber]

@shvaikalesh Good catch. However, I think this issue needs some discussion before merging a PR.

Currently, .empty is documented as follows:

Asserts that the target's length is 0. For arrays and strings, it checks
the length property. For objects, it gets the count of
enumerable keys.

We need to decide and document what the expected behavior is for null, undefined, booleans, numbers, symbols, and functions. In particular, is "empty" synonymous with "falsey" when it comes to these other data types?

Current behavior:

describe("empty", () => {
  it("null", () => {
    expect(null).to.be.empty;  // TypeError
  });

  it("undefined", () => {
    expect(undefined).to.be.empty;  // Pass
  });

  it("true", () => {
    expect(true).to.be.empty;  // Fail
  });

  it("false", () => {
    expect(false).to.be.empty;  // Pass
  });

  it("0", () => {
    expect(0).to.be.empty;  // Pass
  });

  it("1", () => {
    expect(1).to.be.empty; // Fail
  });

  it("Symbol", () => {
    expect(Symbol()).to.be.empty;  // Fail
  });

  it("function", () => {
    function noop () {}
    expect(noop).to.be.empty;  // Fail
  });
});

My first impression is that .empty should behave almost exactly as it's currently documented, except rewording "objects" to "non-function objects". In other words, I think that .empty should only test arrays, strings, and non-function objects, and should otherwise throw a TypeError when passed null, undefined, booleans, numbers, symbols, or functions.

I'm not crazy about assertions like empty in the first place. By taking on different library-defined (as opposed to language-defined) meanings based on which type of variable is supplied, they add uncertainty and risk to a test, especially if the value under test isn't first tested for its type. Therefore, I think empty should be limited to as few meanings as possible. If a developer wishes to test that a value is falsey, they should assert for falseyness specifically instead of leveraging an empty assertion that has been overloaded to mean falsey for some data types but not others.

@keithamus @lucasfcosta?

Edit: Elaborated on my opinion.

[shvaikalesh]

@meeber thanks for the detailed feedback. We should definitely add tests for other data types.

I agree that it is better to avoid defining library-specific meanings and let empty be as thinner wrapper for Object.keys as possible, thus adhere to its new semantics for primitives: throw on == null and return empty arrays for others.

it("function", () => {
    function noop () {}
    expect(noop).to.be.empty; // Fail
});

In terms of the spec, functions are just objects with few additional internal methods (pretty much like promises or collections), thus this behavior does not seem very straightforward to me. What function is "empty"? Without formal parameters? With empty body? Without name or .name == "anonymous"? That returns undefined? With empty .prototype? I think we should spare the users from this ambiguity and just pass functions to Object.keys without making a special case for them.

Yet another matter to decide: Map and Set. Should .empty report true only when collections have .size == 0? It seems useful and straightforward.

[meeber]

@shvaikalesh Agreed that having .empty check Map and Set values for .size == 0 is in the same spirit as the current behavior for arrays, objects, and strings. Seems appropriate.

Yup, we had an almost identical thought process regarding the ambiguity of what it means for a function to be "chai-empty" (reminds me of "thai-spicy"). In my original unedited post, I had a separate paragraph suggesting that .empty is currently bugged for functions since a function without any enumerable keys should, by virtue of being an object, pass instead of fail. I then changed my mind to state that .empty should throw a TypeError when passed a function, mostly due to the ambiguity. But I find myself flip-flopping again in agreement with you that it should just be treated like any other object with regard to Object.keys().

Not sure I agree with using default ES6 Object.keys() behavior to convert all other primitive types to empty arrays (thus causing them to always pass the .empty assertion). If .empty had a 1-to-1 mapping with Object.keys(), then sure. But given the special behavior for strings, arrays, maps, and sets, I'm leaning toward throwing TypeError when provided a value of a type that .empty wasn't designed for.

[lucasfcosta]

@shvaikalesh Thanks for your PR! 😄

I totally agree with @meeber. If I just read the docs I'd expect .empty to check the length of any object that is not a Function. Semantically speaking, it is impossible to exactly define what is considered to be an "empty" function. Also, currently the .length property of a function even exists, but it represents how many named arguments that function takes, this could make someone thing that .empty does a check regarding this property.
IMO throwing a TypeError would be the best choice here to avoid ambiguity, I'd rather have an explicit error than having an ambiguous behavior.

It also seems too dangerous to me to convert primitive types to empty arrays, because, as @meeber said before, this would make them pass the assertion and this is also something that anyone who just read the docs wouldn't expect to happen.

To me, the goal of having an .empty assertion should be pretty straightforward: it should check if things that should contain other things (Arrays, Maps, Sets, Strings or Objects) contain anything or not.

[keithamus]

Agree with @meeber and @lucasfcosta here. We have .to.exist for null and undefined, but we should be throwing a typeerror for .to.be.empty for non-objects.

Let's close this one for now. @shvaikalesh thank you so much for your contributions. Please don't consider this a waste of time, we'd love to see more contributions from you! If you'd like, a small refactor to this one - having it make an assertion error for null values passed in would be welcome.

[shvaikalesh]

Thanks for the feedback, is.empty was changed a bit in 4.0 branch. I would like to prepare 2 PRs: first to change its behavior to match consensus we reached here and second one to account for Map and Set.

[lucasfcosta]

Great idea @shvaikalesh!
Thanks for your ideas, it is always great to have such dedicated contributors 😄