Assert #923

Merged
merged 4 commits into from Jun 6, 2012

3 participants

@joliss
  • Guard against undefined mixins

    This can easily happen with Ember.Object.extend(App.DoesNotExist, {...}).

  • Use Ember.assert instead of throw

  • Guard against implicit function application by Ember.assert

  • Guard mergeMixins parameters more generally

    Previously we caught passing null or undefined into Object.create() or
    .extend(). Now we also catch any other invalid types like strings,
    numbers, or arrays.

    We cannot use Ember.typeOf, since runtime depends on metal and we don't
    want circular dependencies. For the weird array check, see
    http://stackoverflow.com/a/1058753/525872

@travisbot

This pull request passes (merged 7fe24a22 into d710d62).

@travisbot

This pull request passes (merged d9fd7235 into d710d62).

@travisbot

This pull request passes (merged 9e2f9d07 into 9944215).

@travisbot

This pull request passes (merged ebd9befd into 9944215).

@wagenet wagenet commented on the diff Jun 3, 2012
packages/ember-metal/lib/mixin.js
@@ -76,7 +76,7 @@ function mergeMixins(mixins, m, descs, values, base) {
for(idx=0;idx<len;idx++) {
mixin = mixins[idx];
- if (!mixin) throw new Error('Null value found in Ember.mixin()');
+ Ember.assert('Expected hash or Mixin instance, got ' + Object.prototype.toString.call(mixin), typeof mixin === 'object' && mixin !== null && Object.prototype.toString.call(mixin) !== '[object Array]');
@wagenet
Ember.js member
wagenet added a note Jun 3, 2012

One difference between this and the previous code is that the Ember.assert will get stripped in production but the !mixin check won't be. Do you think it matters for us to have a check in production?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@wagenet wagenet commented on the diff Jun 3, 2012
packages/ember-metal/lib/mixin.js
@@ -224,7 +224,7 @@ function applyMixin(obj, mixins, partial) {
if (desc === REQUIRED) {
if (!(key in obj)) {
- if (!partial) throw new Error('Required property not defined: '+key);
+ Ember.assert('Required property not defined: '+key, !!partial);
@wagenet
Ember.js member
wagenet added a note Jun 3, 2012

Same as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@wagenet wagenet commented on the diff Jun 3, 2012
packages/ember-metal/lib/mixin.js
@@ -311,7 +311,8 @@ function applyMixin(obj, mixins, partial) {
if (META_SKIP[key]) continue;
keys.push(key);
}
- throw new Error('Required properties not defined: '+keys.join(','));
+ // TODO: Remove surrounding if clause from production build
+ Ember.assert('Required properties not defined: '+keys.join(','));
@wagenet
Ember.js member
wagenet added a note Jun 3, 2012

And again.

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

Do you think it matters for us to have a check in production?

I was thinking it's not necessary to have these in production. My sense is that in JS, people don't generally use exceptions for control flow, right? If so, these are just there to crash the app in case you pass a wrong parameter to .extend/.create/.reopen or miss a required key. In other words, they are assertions I want in dev/test mode to help me develop, but that I want stripped in production mode for performance.

Do you think we should keep them as throws? I can change them back if you prefer.

@travisbot

This pull request passes (merged 988be4b0 into 70bb9de).

joliss added some commits May 31, 2012
@joliss joliss Guard against undefined mixins
This can easily happen with Ember.Object.extend(App.DoesNotExist, {...}).
d6d29a7
@joliss joliss Use Ember.assert instead of throw b6c1c2f
@joliss joliss Guard against implicit function application by Ember.assert d2edc1d
@joliss joliss Guard mergeMixins parameters more generally
Previously we caught passing null or undefined into Object.create() or
.extend(). Now we also catch any other invalid types like strings,
numbers, or arrays.

We cannot use Ember.typeOf, since runtime depends on metal and we don't
want circular dependencies. For the weird array check, see
http://stackoverflow.com/a/1058753/525872
18ce711
@travisbot

This pull request passes (merged 18ce711 into c5e3962).

@wagenet wagenet merged commit 4a673c7 into emberjs:0-9-stable Jun 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment