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

QUnit 1.20 `deepEqual` with EmptyObject #12756

Closed
rwjblue opened this Issue Dec 27, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@rwjblue
Member

rwjblue commented Dec 27, 2015

In versions of QUnit prior to 1.20 it was not possible to compare any object without a constructor against one with a constructor, but a special exemption was made for those with a constructor of null. @stefanpenner reported this issue in qunitjs/qunit#851 and goes into a bunch more detail there. This was fixed upstream in QUnit in qunitjs/qunit@b8f1926 and released as part of QUnit 1.20.

Unfortunately, we do not perfectly matched the scenario that was fixed because we use constructor value of undefined (and QUnit 1.20.0 is specifically checking for null).

I am unsure if we should change our internal EmptyObject to use null or if we should update QUnit to detect falsey constructor.

I'd love @stefanpenner's and @leobalter's thoughts here.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 28, 2015

I'm game to updating QUnit, but I'm also interested to see @stefanpenner thoughts here. The EmptyObject using a null constructor seems like a good idea as well.

I'm game to updating QUnit, but I'm also interested to see @stefanpenner thoughts here. The EmptyObject using a null constructor seems like a good idea as well.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 6, 2016

Member

Unfortunately, we do not perfectly matched the scenario that was fixed because we use constructor value of undefined (and QUnit 1.20.0 is specifically checking for null).

do we need that?

Member

stefanpenner commented Jan 6, 2016

Unfortunately, we do not perfectly matched the scenario that was fixed because we use constructor value of undefined (and QUnit 1.20.0 is specifically checking for null).

do we need that?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 6, 2016

Member

Not sure what you mean. I'm not certain that Ember needs to use undefined as the constructor value (if that is what you mean).

It would certainly be nice to be able to use deepEqual with our EmptyObjects, and for that to work we either need to swap to use null or QUnit needs to do a falsey check.

I'm just not sure who is "wrong" here...

Member

rwjblue commented Jan 6, 2016

Not sure what you mean. I'm not certain that Ember needs to use undefined as the constructor value (if that is what you mean).

It would certainly be nice to be able to use deepEqual with our EmptyObjects, and for that to work we either need to swap to use null or QUnit needs to do a falsey check.

I'm just not sure who is "wrong" here...

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 6, 2016

Member

Ya I was curious about the undefined constructor bit

Member

stefanpenner commented Jan 6, 2016

Ya I was curious about the undefined constructor bit

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 6, 2016

Member

I have no clue why we ended up using undefined. I'll do some digging to see when it was introduced if there was any discussion about undefined vs null.

Member

rwjblue commented Jan 6, 2016

I have no clue why we ended up using undefined. I'll do some digging to see when it was introduced if there was any discussion about undefined vs null.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 6, 2016

Member

i don't think its needed.

Member

stefanpenner commented Jan 6, 2016

i don't think its needed.

@pixelhandler

This comment has been minimized.

Show comment
Hide comment
@pixelhandler

pixelhandler Jan 8, 2016

Contributor

@rwjblue I think using null is just fine. I often use Object.create(null) objects for a custom map of data that I don't want or need a prototype of Object for, nice for iterating with for (let key in map) {} (no need to check with map.hasOwnProperty(key) since there is no prototype.

var g = Object.create(null, {
  constructor: {
    value: null,
    writable: true,
    configurable: true
  }
});
> undefined
g.yo = "Yo";
> "yo"
g.ho = "Ho";
> "ho"
Object.freeze(g);
> Object {yo: "Yo", ho: "Ho", constructor: null}
Object.keys(g);
> ["yo", "ho"]

My vote is to change https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/empty_object.js#L11 to use null for the constructor. Since that maps nicely to how that object was created using null as well. I have no idea on the impact of any other code expecting undefined for a constructor, but I bet if there is any could be changed to expect null.

Contributor

pixelhandler commented Jan 8, 2016

@rwjblue I think using null is just fine. I often use Object.create(null) objects for a custom map of data that I don't want or need a prototype of Object for, nice for iterating with for (let key in map) {} (no need to check with map.hasOwnProperty(key) since there is no prototype.

var g = Object.create(null, {
  constructor: {
    value: null,
    writable: true,
    configurable: true
  }
});
> undefined
g.yo = "Yo";
> "yo"
g.ho = "Ho";
> "ho"
Object.freeze(g);
> Object {yo: "Yo", ho: "Ho", constructor: null}
Object.keys(g);
> ["yo", "ho"]

My vote is to change https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/empty_object.js#L11 to use null for the constructor. Since that maps nicely to how that object was created using null as well. I have no idea on the impact of any other code expecting undefined for a constructor, but I bet if there is any could be changed to expect null.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 8, 2016

Member

@pixelhandler:

Since that maps nicely to how that object was created using null as well.

not really. Consider this:

f = Object.create(null)
// => Object {}
f.constructor
// => undefined

Versus:

// changes `EmptyObject` to use `null` instead of `undefined`

var proto = Object.create(null, {
  // without this, we will always still end up with (new
  // EmptyObject()).constructor === Object
  constructor: {
    value: null,
    enumerable: false,
    writable: true
  }
});

function EmptyObject() {}
EmptyObject.prototype = proto;

f = new EmptyObject()
// => EmptyObject {}
f.constructor
// => null

As you can see, the .constructor value of an EmptyObject when using null as the value, is null whereas with Object.create(null) the value is undefined.

Member

rwjblue commented Jan 8, 2016

@pixelhandler:

Since that maps nicely to how that object was created using null as well.

not really. Consider this:

f = Object.create(null)
// => Object {}
f.constructor
// => undefined

Versus:

// changes `EmptyObject` to use `null` instead of `undefined`

var proto = Object.create(null, {
  // without this, we will always still end up with (new
  // EmptyObject()).constructor === Object
  constructor: {
    value: null,
    enumerable: false,
    writable: true
  }
});

function EmptyObject() {}
EmptyObject.prototype = proto;

f = new EmptyObject()
// => EmptyObject {}
f.constructor
// => null

As you can see, the .constructor value of an EmptyObject when using null as the value, is null whereas with Object.create(null) the value is undefined.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 8, 2016

Member

I assumed that the above example was why undefined was chosen (because it resembles Object.create(null) a bit better).

In the end, I am personally fine with changing undefined to null. The goal of this issue is to decide if that is acceptable....

Member

rwjblue commented Jan 8, 2016

I assumed that the above example was why undefined was chosen (because it resembles Object.create(null) a bit better).

In the end, I am personally fine with changing undefined to null. The goal of this issue is to decide if that is acceptable....

@pixelhandler

This comment has been minimized.

Show comment
Hide comment
@pixelhandler

pixelhandler Jan 8, 2016

Contributor

@rwjblue Hum, when setting a constructer value to null, this adds a property to EmptyObject vs. using undefined results in no constructor property.

So I think the question is… should the EmptyObject be changed to have a constructor property at all ? (currently it does not, adding a constructor: null property is technically not an empty object, but practically I think it works, maybe that's why QUnit's deepEqual made a special exemption)

Contributor

pixelhandler commented Jan 8, 2016

@rwjblue Hum, when setting a constructer value to null, this adds a property to EmptyObject vs. using undefined results in no constructor property.

So I think the question is… should the EmptyObject be changed to have a constructor property at all ? (currently it does not, adding a constructor: null property is technically not an empty object, but practically I think it works, maybe that's why QUnit's deepEqual made a special exemption)

@pixelhandler

This comment has been minimized.

Show comment
Hide comment
@pixelhandler

pixelhandler Apr 29, 2016

Contributor

@rwjblue any update on whether this is still needed, or not? re: QUnit's special comparison of constructor == null

Contributor

pixelhandler commented Apr 29, 2016

@rwjblue any update on whether this is still needed, or not? re: QUnit's special comparison of constructor == null

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 4, 2016

Member

I'm going to close this, doesn't really cause problems in practice so far...

Member

rwjblue commented Nov 4, 2016

I'm going to close this, doesn't really cause problems in practice so far...

@rwjblue rwjblue closed this Nov 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment