Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Ember.Object.create() options should set properties, not overwrite computed property functions #777

Closed
adamjmurray opened this Issue · 13 comments

6 participants

@adamjmurray

As we work through bugs in our application, we are introducing more computed property setter functions to clean up data, validate, create bi-directional associations, etc. But we have built out this app assuming calls to create() basically just called the setters for the given properties. Unfortunately, it works much more like extend() and will overwrite any computed property functions with static values.

Here is a very contrived (unrealistic) example that shows you how unintuitive the behavior can get: http://jsfiddle.net/QJ8vX/2/

Another thread discussing the issue in on stack overflow: http://stackoverflow.com/questions/10420357/why-dont-the-arguments-to-create-behave-more-like-setproperties

FWIW myself and a few coworkers feel that these behaviors violates standard expectations about how an object-oriented object model should behave. As I mentioned on the stack overflow thread, the only reason I can see for the current behavior is to support Ruby-style singleton instance method implementations, but this would easily be supported via Class.extend({ singletonMethods: ... }).create(properties)

@wagenet
Owner

@adamjmurray This has been discussed before. create extends prototypes, it's not for setting computed property values. This isn't something that is going to change. You can consider using setProperties if you want this.

@wycats or @kselden might be able to chime in with more of the rationale for this.

@wagenet wagenet closed this
@devinus

@adamjmurray create isn't a setter. It's not a "setting" operation.

@devinus

@adamjmurray also, performance reasons. ~*~magic I don't understand~*~

@adamjmurray

My main issue is that I want to create an instance, set the initial property values, and then have init() be called. I will have to adjust my mental model about how Ember is supposed to work.

I have seen this confuse a few people, but if you're not going to change it, so be it. We'll deal.

@wagenet
Owner

@adamjmurray Why not just do the property setup in init?

@endash

@wagenet I suggested to Adam that he open a ticket for discussion because I was having similar thoughts the other day. If this has been hashed out in a previous ticket, could you link to the dupe? I think this is something that a fair number of people are gonna look at askance and go looking for answers or changes, and it'd help to have a record of the reasoning behind this design decision.

@adamjmurray

@wagenet I like that idea. Can you recommend a pattern for that? Does something like the following seem reasonable?

init: function() {
  var initProperties = this.get('initProperties');
  if(initProperties) {
    this.setProperties(initProperties);
    this.set('initProperties', null); // don't need lingering references to this data
  }
  // proceed with "constructor" logic
}

I wouldn't expect to code this in most classes, it's the sort of thing that would be well suited for a top-level superclass.

And I agree with endash. This behavior is the kind of thing that should go into the FAQ or docs. If the reasoning behind it is "for performance", that's fine, but people will want to know and should understand they need to be careful with create() arguments when using computed property functions.

@krisselden
Owner

Ember.Object.create is more like Object.create(prototype) where prototype is an instance constructed from the super's constructor.

What are you using the CP setter for? Curious about your use case.

If you want, you can use args to init and use new Class(args) instead but you won't be able to have the autowiring of bindings, CP, and observes and you'll have to remember to call super with arguments inside init.

App.Foo = Ember.Object.extend({
    init: function (props) {
      this.setProperties(props);
   }
});

new App.Foo(props);

you have to be careful when overriding classes that expect args to init to call super with args

this._super.apply(this, arguments);
@wagenet
Owner

@adamjmurray You can probably do something like that.

@endash, this is mainly a design decision. Right now, the fact that we override computed properties, means the following code will work:

Ember.View.create({ template: Ember.Handlebars.compile(...) })

Since template is a computed property, if we change it, then this code wouldn't work anymore. You'd have to do:

Ember.View.extend({ template: Ember.Handlebars.compile(...) }).create()

So either we have to make that more verbose or you have to be more verbose by using setProperties or a custom init setup. Either way there's a cost.

@krisselden
Owner

I would suggest instead of a setter, design the CP so you would set its dependency in create. Just like templateName and template in View. CP setters are rare.

But if you don't care about wiring in create, you don't have to use it.

App.Person = Ember.Object.extend({
    init: function (firstName, lastName) {
      this.set('firstName', firstName);
      this.set('lastName', lastName);
   }
});

App.Employee = App.Person.extend(
   init: function (firstName, lastName, manager) {
      this._super(firstName, lastName);
      this.set('manager', manager);
   }
);

new App.Employee('John', 'Doe', manager);
@adamjmurray

@kselden, my current use cases for computed property setters are:

  1. Enforcing bi-directional relationships. We have a 1-to-1 Parent-Child relationship, and every time you move the child to another parent, you would need to call child.set('parent', parent); parent.set('child', child);. It simplified the code and made it less error-prone to enforce the relationship in the CP setter, so only one of those two statements would be needed.

  2. Enforcing data validity. In an image-editing app, certain combinations of operations can lead to floating point values when we want to enforce integers in the model. It simplified the code to allow most of the app to not worry about this, but round to the nearest integer in the model CP setter.

  3. Data normalization (really just a variation of data validity like above). In a text-editing app, different browsers will try to set different markup in our model. We normalize the markup via a CP setter.

In all cases, I applied patterns from previous app building experience to build models to enforce that their state is valid. As new code paths are introduced that may interact with our models in new ways, we don't have to worry about regressions on these behaviors.

Now I am getting the feeling I simply should not use CP setters. I suppose I might consider funneling all model changes through an authorized controller that can enforce these things. I will keep brainstorming ideas. Any input is appreciated.

@demongloom

It's may be a dirty hack, but it's works for me.

 Em.Object.reopenClass({ 
    create: function(config) {
        return this._super().setProperties(config); 
    }
 });
@endash

FYI, this requested behaviour is now the standard behaviour as of 1 Dec 2012. create() uses computed property setters, with the old behaviour relegated to createWithMixins. (commit c1c720)

@knusul knusul referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.