Skip to content
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

Remove meta.descs #10323

Merged
merged 1 commit into from
Feb 1, 2015
Merged

Remove meta.descs #10323

merged 1 commit into from
Feb 1, 2015

Conversation

ebryn
Copy link
Member

@ebryn ebryn commented Jan 31, 2015

@stefanpenner @krisselden @mmun One less allocation in meta. This also means we dont need to instantiate a meta object just to add a computed property to a POJO.

@@ -119,7 +119,8 @@ function lazyGet(obj, key) {
}

// if a CP only return cached value
var desc = meta && meta.descs[key];
var possibleDesc = obj[key];
var desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like a bikeshedding around the fastest/safest way to check if it's a descriptor @stefanpenner @krisselden

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although verbose this is likely the best mechanism to check.

Alternatively, as meta.descs is internal, and we vow never to set it to null rather undefined or a value, we wouldn't need the null check. That being said, the null check is essentially free.

@ebryn ebryn force-pushed the remove-meta-descs branch from 327d3eb to c3f13e8 Compare January 31, 2015 23:36
@@ -303,7 +302,7 @@ function Meta(obj) {
}

Meta.prototype = {
chainWatchers: null
chainWatchers: null // FIXME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, we should fix this, should be easy

@stefanpenner
Copy link
Member

I suspect having the descriptor be the property will also be less "magical" But lets see how people react.

We should confirm that ember-data behaves correctly with this patch, as it and reduce computed often reach around some of the private meta API

ebryn added a commit that referenced this pull request Feb 1, 2015
@ebryn ebryn merged commit 677cfac into emberjs:master Feb 1, 2015
bmac added a commit to bmac/data that referenced this pull request Feb 5, 2015
bmac added a commit to bmac/data that referenced this pull request Feb 5, 2015
bmac added a commit to bmac/data that referenced this pull request Feb 6, 2015
bmac added a commit to bmac/data that referenced this pull request Feb 7, 2015
bmac added a commit to bmac/data that referenced this pull request Feb 8, 2015
quiddle pushed a commit to quiddle/data that referenced this pull request Feb 28, 2015
@herom
Copy link

herom commented Apr 6, 2015

Now, that meta.descs is removed (and I also can't find __ember_meta__.descs on Objects anymore), what's the safest way to find out which of the properties of an Object are instances of ComputedProperty?

Until 1.10.0 my following code was working (although I expected it to be pretty "unstable" as it relied on __ember_meta__ (as you can see here):

_stripComputedPropertiesFromModel: function (model) {
    var serializedProperties = get(model, 'serializableProperties');
    var descs = model.__ember_meta__.descs;

    return serializedProperties.filter(function (property) {
      return !(descs[property] instanceof ComputedProperty);
    });
  }

@jmurphyau
Copy link
Contributor

Try this..

_stripComputedPropertiesFromModel: function (model) {
  var serializedProperties = get(model, 'serializableProperties');

  return serializedProperties.filter(function (property) {
    return !(model[property] instanceof ComputedProperty);
  });
}

When you access the properties directly on the object - without using .get() - it returns the computed property

@herom
Copy link

herom commented Apr 7, 2015

thanks @jmurphyau - I tried it in a JSBin example, but it doesn't seem to work?

@jmurphyau
Copy link
Contributor

@herom that JSBin still has meta.descs - it was using Ember 1.10.. Also the check wasn't complete.. I've added a bit more detail - an Ember.computed.alias isn't an instance of an Ember.ComputedProperty.

Take a look at this JSBin using Ember 1.11

@herom
Copy link

herom commented Apr 8, 2015

thanks a lot @jmurphyau - that's great! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants