-
-
Notifications
You must be signed in to change notification settings - Fork 699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure frozen, sealed and extensible can only be used with non-null Objects and Arrays #491
Comments
Hey @astorije thanks for the issue.
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. |
Thanks for your answer @keithamus! Right, I wasn't clear. This also comes from the fact that in my mind the 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
Wouldn't it be worth having the tests fail with a more chai-related message, such as |
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 If you want to send a PR adding this feature in, I'd gladly accept it. |
@keithamus, I'll gladly send a PR. However, the check I need to add is, in pseudo-code: |
We might need to revisit this actually. ES6 changes the semantics of these functions to no longer return Instead of trying to do type checking on the Object, especially considering the semantics change between ES5 and ES6, I'd suggest we just 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
);
}); |
@keithamus I gave it a try in #496, let me know what you think. |
This is also fixed in a42ac43 and can be closed now. |
Thanks again @berkerpeksag 😄 |
Right now it seems to me that there is either no type check or that only
object
s are allowed: see this line (on the top of my head, I am not sure whatflag(this, 'object')
does and if it checks that we are giving it anobject
or not).These properties should be callable on
Object
andArray
elements, but careful thatnull
is considered anobject
although these functions cannot be used onnull
:The text was updated successfully, but these errors were encountered: