Make sure frozen, sealed and extensible can only be used with non-null Objects and Arrays

[astorije]

Right now it seems to me that there is either no type check or that only objects are allowed: see this line (on the top of my head, I am not sure what flag(this, 'object') does and if it checks that we are giving it an object or not).

These properties should be callable on Object and Array elements, but careful that null is considered an object although these functions cannot be used on null:

> Object.isFrozen({});
false
> Object.isSealed({});
false
> Object.isExtensible({});
true
> Object.isFrozen([]);
false
> Object.isSealed([]);
false
> Object.isExtensible([]);
true
> Object.isFrozen(null);
TypeError: Object.isFrozen called on non-object
    at Function.isFrozen (native)
    ...

[keithamus]

Hey @astorije thanks for the issue.

flag(this, 'object') is just to extract the object passed in via expect(object). It doesn't do anything in particular.

To be honest, I'm not sure what the issue is here. Can you reproduce with Chai code, something that is either a false positive or negative? Could you please elaborate further on what the issue is.

[astorije]

Thanks for your answer @keithamus!

Right, I wasn't clear. This also comes from the fact that in my mind the flag was actually doing checks.

So no false positives or false negatives, but maybe a confusing error message. Consider the following test suite:

describe('.frozen edge cases', function () {
  it('with an Object', function () {
    expect(Object.freeze({ bar: 42 });).to.be.frozen;
  });

   it('with an Array', function () {
    expect(Object.freeze([1, 2, 3]);).to.be.frozen;
  });

   it('with a litteral', function () {
    expect(42).to.be.frozen;
  });

  it('with a null object', function () {
    expect(null).to.be.frozen;
  });
});

Because .frozen is just a shorthand to Object.isFrozen, the last 2 tests output:

  1) .frozen edge cases with a litteral:
     TypeError: Object.isFrozen called on non-object
      at Function.isFrozen (native)
      ...

  2) .frozen edge cases with a null object:
     TypeError: Object.isFrozen called on non-object
      at Function.isFrozen (native)
      ...

Wouldn't it be worth having the tests fail with a more chai-related message, such as .frozen can only be used on arrays and non-null objects?

[keithamus]

I guess each of these assertions could first assert that the object is indeed an object, or rather that it is not-null. So frozen would become:

  Assertion.addProperty('frozen', function() {
    var obj = flag(this, 'object');
    new Assertion(obj).to.not.be.null;
    this.assert(
      Object.isFrozen(obj)
      , 'expected #{this} to be frozen'
      , 'expected #{this} to not be frozen'
    );
  });

(Not the new Assertion(object).to.not.be.null;)

If you want to send a PR adding this feature in, I'd gladly accept it.

[astorije]

@keithamus, I'll gladly send a PR.

However, the check I need to add is, in pseudo-code: obj is an Array OR (obj is an Object AND obj is not null).
Since Chai assertions won't let me use a OR mechanism, I guess I have to check that with good ole typeofs or similar and call Chai's .fail() method when the contract (the check) is not respected. What would you think of that?

[keithamus]

We might need to revisit this actually. ES6 changes the semantics of these functions to no longer return TypeErrors.

Instead of trying to do type checking on the Object, especially considering the semantics change between ES5 and ES6, I'd suggest we just catch() the Object.isFrozen errors and wrap them in a Chai Assertion failure, like so:

  Assertion.addProperty('frozen', function() {
    var obj = flag(this, 'object');
    // try/catch because calls to Object.isFrozen in ES5 will throw TypeErrors on non-objects
    try {
        Object.isFrozen(obj)
        this.assert(
          Object.isFrozen(obj)
          , 'expected #{this} to be frozen'
          , 'expected #{this} to not be frozen'
        );
    } catch(e) {
        this.assert(
            false
          , 'expected #{this} to be frozen, but got #{act}'
          , 'expected #{this} to not be frozen but got #{act}'
          , obj
          , e
        );
  });

[astorije]

@keithamus I gave it a try in #496, let me know what you think.

[berkerpeksag]

This is also fixed in a42ac43 and can be closed now.

[keithamus]

Thanks again @berkerpeksag 😄