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

Automatic dependency recognition #386

Closed
sheerun opened this issue Jan 12, 2012 · 23 comments
Closed

Automatic dependency recognition #386

sheerun opened this issue Jan 12, 2012 · 23 comments

Comments

@sheerun
Copy link

sheerun commented Jan 12, 2012

How about automatic recognizing (on first run) in Ember.computed body function what dependencies it uses?

See the example from docs:

Person = Ember.Object.extend({
  // these will be supplied by `create`
  firstName: null,
  lastName: null,

  fullName: Ember.computed(function() {
    var firstName = this.get('firstName');
    var lastName = this.get('lastName');

    return firstName + ' ' + lastName;
  }).property('firstName', 'lastName')
});

My idea is to reprogram this.get method so they'd automatically add property dependencies if executed in context of Ember.computed, so above code would become shorter and more coffee-script friendly:

Person = Ember.Object.extend({
  // these will be supplied by `create`
  firstName: null,
  lastName: null,

  fullName: Ember.computed(function() {
    var firstName = this.get('firstName');
    var lastName = this.get('lastName');

    return firstName + ' ' + lastName;
  })
});
Person = Ember.Object.extend
  firstName: null
  lastName: null
  fullName: Ember.computed ->
    @get("firstName") + " " + @get("lastName")

Oh. And use of property() method on Ember.computed would disable this behavior so code would be backward-compatible and customizable if needed to.

What do you think?

@wagenet
Copy link
Member

wagenet commented Jan 12, 2012

Seems interesting. However, I feel like this would probably be pretty fragile and prone to breaking. For instance, what if you did something like:

fullName: Ember.computed(function() {
  var self = this;
  var firstName = self.get('firstName');
  var lastName = self.get('lastName');

  return firstName + ' ' + lastName;
})

I know that's a bit contrived, but I can imagine many other similar situations.

@sheerun
Copy link
Author

sheerun commented Jan 12, 2012

Currently I can think of two solutions:

  1. passing shimmed proxy as this
  2. using global like Ember.context

I don't think about removing property method. It would be just nice feature for faster development of some simple methods, even enabled by some discover_dependencies: true option.

Anyways, I'll be glad if someone find elegant solution.

@exogen
Copy link

exogen commented Jan 13, 2012

I don't see anything about @wagenet's example that would break automatic detection. Knockout.js already does this. (Whenever a computed observable is evaluated, it pushes a dependencies array onto a stack, and any observables evaluated during that time are added. Works because JavaScript is single-threaded.)

@wagenet
Copy link
Member

wagenet commented Jan 13, 2012

@exogen: Ah good point, I was thinking of it in the wrong way. I'm open to someone attempting a pull request for this. I prefer explicit declaration, but we could probably do a half decent job of guessing. I still imagine we'd have problems with things like:

var content = this.get('content');
if (content) { content.get('value'); }

If content is null, the get('value') will never be called and we won't have it in the list of dependencies, but in these cases people could specify explicitly.

@exogen
Copy link

exogen commented Jan 13, 2012

@wagenet Knockout handles that case, too – the dependency list isn't determined once, it's updated whenever the observable is re-evaluated.

@wycats
Copy link
Member

wycats commented Jan 17, 2012

The biggest problem with this approach is that it requires executing all computed properties on boot to determine their dependencies. This would be:

  1. extremely expensive,
  2. conceptually problematic, because computed properties may rely on state that is not set up during this bootstrapping phase, and
  3. potentially fragile in unexpected ways, because it's not obvious to me that multiple conditionals inside a computed property could not conspire together to hide changeable dependencies from the automatic detector

I suspect that executing all computed properties for all new objects would be prohibitively expensive. I'd be open for someone to demonstrate that this was not true.

@wycats wycats closed this as completed Jan 17, 2012
@exogen
Copy link

exogen commented Jan 17, 2012

All I can offer is that Knockout has already solved your concerns, including the multiple complex conditionals, although I can't speak to the "expensive" part. It has the option to defer evaluation of computed properties – there's really no need to determine dependencies on boot vs. during the first evaluation, is there?

@wagenet
Copy link
Member

wagenet commented Jan 17, 2012

@exogen There's definitely a need to determine dependencies in advance. Otherwise we won't be able to trigger any change notifications until we've done one evaluation.

@judofyr
Copy link

judofyr commented Feb 5, 2012

If a tree falls in a forest and no one is around to hear it, does it make a sound?

If a computed property isn't used, who cares if it changes?

@sheerun
Copy link
Author

sheerun commented Feb 5, 2012

I think of following counterexample:

  1. On first run Math.random() gives 0.9
  2. We set status to true so changedAt should change and notify other dependencies
  3. In fact it isn't because on first run no dependencies were recognized.
Person = Ember.Object.extend({
  status: false,
  changedAt: Ember.computed(function() {
    if(Math.random() > 0.5 || this.get('status')) {
      return Date();
    }
  })
});

@judofyr
Copy link

judofyr commented Feb 5, 2012

In those (rare) cases you must just use explicit dependency tracking.

@wycats
Copy link
Member

wycats commented Feb 5, 2012

@judofyr do you understand the concern about needing to execute the computed properties initially to bootstrap the dependencies?

@judofyr
Copy link

judofyr commented Feb 5, 2012

I don't see the need to bootstrap the dependencies. What I'm trying to say: Nearly all places where you care about updates to computed properties is a place where the property is actually called (e.g. in a view). If you don't call a computed property, there's a big chance you won't need the events either.

@nathggns
Copy link

I hate to bump an old thread, but anymore news on this?

@stefanpenner
Copy link
Member

@nathggns @wycats's comment is still our thoughts on this.

Some additional thoughts:

In theory inferring from syntax will work when it is syntactically obvious, emberscript's ~> function offers this. This idea could be extended to have dependencies aligned with the internal branching of a property.

@nathggns
Copy link

It's possible to defer dependencies without using the syntax. You can use the use of the get function to do it (similar to Knockout). You also don't need to call it at creation time to decide the dependencies as the value is irrelevant until it's needed (at which point, it has to be called anyway).

@stefanpenner
Copy link
Member

@nathggns this is true, but the extra complexity of tracking get's during during CP invocation and supporting arbitrary branching within CP's vs annotating the a CP DK's hasn't created a motivating situation.

If in fact you are motivated and have the bandwidth, I would invite you investigate this further.

@nathggns
Copy link

I'm currently looking at adding computed properties altogether to angular
(with automatic dependency tracking). I'll let you know how that goes.

On 22 December 2013 22:19, Stefan Penner notifications@github.com wrote:

@nathggns https://github.com/nathggns this is true, but the extra
complexity of tracking get's during during CP invocation and supporting
arbitrary branching within CP's vs annotating the a CP DK's hasn't created
a motivating situation.

If in fact you are motivated and have the bandwidth, I would invite you
investigate this further.


Reply to this email directly or view it on GitHubhttps://github.com//issues/386#issuecomment-31097432
.

Thanks,
Nathaniel Higgins
http://nath.is
@nathggns http://twitter.com/nathggns

@rwjblue
Copy link
Member

rwjblue commented Dec 22, 2013

@nathggns - I believe that https://github.com/gunn/ember-auto implements some nice improvements to reduce much of the duplication.

@nathggns
Copy link

I'd rather get them inline to be honest.

Sent from my iPhone

On 22 Dec 2013, at 22:22, Robert Jackson notifications@github.com wrote:

@nathggns - I believe that https://github.com/gunn/ember-auto implements some nice improvements to reduce much of the duplication.


Reply to this email directly or view it on GitHub.

@rhyek
Copy link

rhyek commented Aug 29, 2017

This is one of the things Ember devs should be looking into going forward, IMO. It is how React and Vue work and it is night and day in terms of simplicity and power. The sort of things you can achieve with computed properties in those frameworks are impossible in Ember or just not worth the effort.

Enough with the feature-less releases that we've been getting for the past 2-3 years. Do something interesting, please.

@rhyek
Copy link

rhyek commented Aug 29, 2017

Like, the problem where you can't have nested @eachs when declaring a computed shouldn't be a thing after all this time.

@mixonic
Copy link
Sponsor Member

mixonic commented Aug 29, 2017

There is a PR to Glimmer's object model that does this: glimmerjs/glimmer-component#61. I believe it is blocked on getting benchmarks to be sure we didn't cause a regression.

It is not likely that this feature would land on the current Ember object model. Instead the ES Classes RFC starts our slow migration away from being coupled to Ember's object model. The Glimmer object model can possibly replace it once we've gone a bit further.

Thanks for the feedback @rhyek.

sandstrom pushed a commit to sandstrom/ember.js that referenced this issue Jun 17, 2021
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

No branches or pull requests

10 participants