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

computedFrom doesn't support paths #149

Closed
jods4 opened this issue Aug 12, 2015 · 38 comments
Closed

computedFrom doesn't support paths #149

jods4 opened this issue Aug 12, 2015 · 38 comments

Comments

@jods4
Copy link
Contributor

jods4 commented Aug 12, 2015

It is my understanding and my experience that computedFrom does not work with paths, like: @computedFrom('user.firstName').

I think that this is a very hard limitation that should be lifted:

  1. Modeling and using sub-properties is common once your application gets even moderately complex.
  2. There is sometimes no good alternative to computedFrom.

My use case is a computed property that creates an array from complex computations and lookups from other arrays. Definitively not something you want to poll and it is too complex for the auto-observation plugin. On the other hand its dependency is trivial to express as it is a single, nested property.

@gabor-farkas
Copy link

+1 :)
(btw, is there no other way to create model listeners (watchers in angular terms) in the controllers?)

@gabor-farkas
Copy link

Well and another thing: if we write paths or expressions in @computedFrom, there's no error message, it simply doesn't do anyting. Neither does the documentation mention exactly what can you write in there.

@davismj
Copy link
Member

davismj commented Nov 30, 2015

+1

2 similar comments
@StrahilKazlachev
Copy link
Contributor

+1

@andrevlins
Copy link

+1

@waywardz
Copy link

waywardz commented Feb 4, 2016

++1

@jods4
Copy link
Contributor Author

jods4 commented Feb 4, 2016

+1 my own ticket along other people, just to attract attention here @EisenbergEffect

We've used Aurelia for 6 months in a complex project and there is something crucial missing; this might be part of the solution.

In UI you commonly have dependencies/computations between various elements. Some simple examples: list elements are filtered by other inputs (commonly just a textbox), you display the total of a shopping cart, you display shipping options only when the user did not choose "pickup" and is not in a foreign country, and so on.

A modern application needs to be able to express such values declaratively, as a computation from their dependencies:

get filteredList() { return this.list.filter(x => x.name.startsWith(x.search)) ; }
get grandTotal() { return this.list.reduce((total, x) => total + x.lineTotal); }
get showShippingOptions() { return this.shipping !== 'pickup' && this.country === 'US'; }

I've used properties for all these examples, although there can be other representations.

Aurelia needs a good story for those use-cases.

It could be some good integration with Reactive programming patterns and/or libraries, which are suited for those kind of things (and more!). I think Rx is great for many advanced scenarios but maybe a little overboard for something as simple as the motivating examples I've shown here.

Coming from Knockout, the examples above would 'just work', thanks to the inherent capability of KO to automatically discover and watch the dependencies of any computations (method, property or otherwise).

To be honest, the examples above would also 'just work' in Aurelia because they would trigger dirty checking. It's just that we need to be careful with dirty checking. Overusing it can lead to perf, battery or animation issues. Examples like the grand total can quickly become expensive to evaluate repeatedly. So we need to have another option.

@computedFrom could be that tool but it only works for simple properties. That's too limiting and if it is the only solution to this problem it needs to beef up: at least start with sub-properties and arrays support.

I personally believe that this theme is very important to Aurelia and it's really overdue.

@davismj
Copy link
Member

davismj commented Feb 4, 2016

@jods4

I have a blog post coming out tomorrow on this topic, but:

get filteredList() { return this.list.filter(x => x.name.startsWith(x.search)) ; }
get grandTotal() { return this.list.reduce((total, x) => total + x.lineTotal); }

Are best handled using a valueConverter. For example, filteredList would be:

components/filterValueConverter.js

export class FilterValueConverter {

  toView(array, search) {
    return array.filter((item) => x.name.startsWith(search));
  }
}

view.html

<require from="components/filterValueConverter"></require>
<select value.bind="selection">
    <option repeat.for="item of array | filter: search" model.bind="item">
        ${item.name}
    </option>
</select>

However, showShippingOptions is best handled with computedFrom, and you can do this today.

@computedFrom('shipping','country')
get showShippingOptions() { return this.shipping !== 'pickup' && this.country === 'US'; }

However, I'm with you and everyone else here. There is a legitimate use case for pathed arguments in computedFrom. This is on our radar, but the +1's do help us assess the value of this feature.

@EisenbergEffect
Copy link
Contributor

@jods4 We've had a computed plugin for many months that does some of this. Maybe @jdanyow can provide some feedback on that...maybe it's time to roll it into the core?

@jods4
Copy link
Contributor Author

jods4 commented Feb 4, 2016

@davismj You are nitpicking on the examples I gave. They were all made-up to illustrate and I agree some have alternate solutions. I have much more complex examples from real applications.

I agree the first two were very well suited to value converters. Especially because they are so re-usable across your app. Just imagine they're not ;)

The last one works... until I tweak my model to:
get showShippingOptions() { return this.shippingMethod !== 'pickup' && this.shippingAddress.country === 'US'; }
And now it does not.

Looking forward to your blog post BTW!

@jods4
Copy link
Contributor Author

jods4 commented Feb 4, 2016

@EisenbergEffect I saw this plugin when it was fresh.
I'm a bit wary of it. It's leaning toward too much magic for my taste:

  • it only supports single return expressions;
  • it does not work on hidden dependencies (e.g. inside method calls);
  • it is unclear what language features (esp. with the ever moving ES standard) are supported or not;
  • without constantly using the logging feature, developers can be unaware that their code is dirty checked because it failed the analyzer.
  • it is hard to be sure that all of the above won't break, e.g. after changing your build to use a more agressive minifier.

In the end, if you're only going to use it for a few trivial computed properties, you might as well slap a computedFrom on them and be done. At least that's how I feel vs the added complexity/bloat.

Mind you, I love the idea and actually was asking for some automatic dependency tracking when I first got into Aurelia. Coming from KO, this was a very sweet feature.
I have realized that this is not easy to build into the platform (not without read detection) and accepted that we might have to use manual dependency declaration instead.

@davismj
Copy link
Member

davismj commented Feb 4, 2016

@jods4 Using @computedFrom to solve the list filter problem is a common pitfall in Aurelia, and I wanted to make sure I pointed it out. As noted at the bottom of my comment, I am still in favor of making this an out of the box feature because there is a real use case for it.

@EisenbergEffect @jdanyow I'm all in favor of getting this feature into the core sooner than later. Without pathed arguments, @computedFrom loses all of its value except in very special use cases.

@EisenbergEffect
Copy link
Contributor

@jods4 The automatic dependency checking is not possible to build without you explicitly declaring something. If you have some ideas on how this might work, please share. You could even built it as a plugin to prototype if you want. The system is extensible enough for that.

@jods4
Copy link
Contributor Author

jods4 commented Feb 5, 2016

@EisenbergEffect At this point I gave up on automatic dependency checking (not for technical reasons, though. Can't say I won't ever look back).

FWIW my former ideas in this space were some kind of @dependencySource annotation that would turn on setter instrumentation similar to the write detection in getters; and another @dependencyAuto that would trigger watching those reads to establish a list of dependencies for arbitrary code/property.

@davismj
Copy link
Member

davismj commented Feb 8, 2016

@jods4 I decided to write an actual valueConverter instead. See here

@jods4
Copy link
Contributor Author

jods4 commented Feb 8, 2016

@davismj thanks a lot, that is a very complete and flexible filter! As filtering is one of the most commonly performed operation, this is an awesome value converter to have.
Suggestion: put it in the standard aurelia libraries! ✨

That said I hope it's not "instead" of this ticket. There are other use cases where a value converter is not a good mental model for this. I don't think we should see value converters as the solution to declare dependencies for any kind computations.

If it were the case, we wouldn't need @computedFrom and we probably would call them multi-binding rather than valueConverter.

@gheoan
Copy link
Contributor

gheoan commented Feb 8, 2016

@davismj how about Array.prototype.reduce? I am using it like this:

// Return the sum of ranks of all Masteries
get rank(): number {
  return this.masteries.reduce((previous, current) => previous + current.rank, 0);
}

Where

this.masteries: Array<Mastery>;

and Mastery is

interface Mastery {
  ...
  rank: number;  
  ...
}

In this case the property rank of all the Mastery inside this.masteries should be observed. There is no way to express that with @computedFrom and aurelia-computed fallbacks to dirty-checking.

@davismj
Copy link
Member

davismj commented Feb 8, 2016

@jods4 definitely not a replacement, just a tool I built that I hope will help the community.

I agree, @gheoan.

@alvarezmario
Copy link

@gheoan there is a better alternative, I did it for a shopping cart I've implemented, and is using the bindingEngine. You need to observe both the this.masteries array and the rank field of each item.
Every time one of them change, I re-run my reduce function to calculate the total items in the cart.

_observeItems(bindingEngine) {
    let subscriptions = [];
    let observer;

    let observeItemsAmountField = () => {
      while (subscriptions.length > 0) {
        subscriptions.pop().dispose();
      }

      this.items.forEach(item => {
        observer = bindingEngine.propertyObserver(item, 'amount');
        subscriptions.push(observer.subscribe(() => this._calculateTotalItems()));
      });
    };

    observeItemsAmountField();

    bindingEngine.collectionObserver(this.items).subscribe(() => {
      this._calculateTotalItems();
      observeItemsAmountField();
    });
  }

_calculateTotalItems() {
    this.totalItems = this.items
      .map(x => parseInt(x.amount, 10))
      .reduce((a, b) => a + b, 0);
  }

Of course this is more complex solution, but it works like a charm and it doesn't use dirty checking.
Hope it helps :)

@gheoan
Copy link
Contributor

gheoan commented Feb 8, 2016

@nomack84 Thank you. In my case I don't need to observe the array (masteries`items`) because its length is static.

@jods4
Copy link
Contributor Author

jods4 commented Feb 9, 2016

@nomack84 that's the kind of things I find myself doing as well when I want to compute something that depends on others that @computeFrom can't observe.

It seems evident to me that we need to find a way to hide this plumbing behind some Aurelia primitive. I'm fine with @computedFrom but it needs to accept a more useful syntax than just a direct property.

We could imagine something like @computedFrom('items[].amount') to encapsulate the code you've presented.

I remember mentioning the computedFrom use-case when discussing the BindingEngine API in another ticket, but it didn't get much traction unfortunately.

@jdanyow
Copy link
Contributor

jdanyow commented Feb 9, 2016

All- I wanted to share some of my thoughts/opinions on computed from- let me know what you think...

  1. Dirty checking a few properties is perfectly fine. If there's a ton of properties being dirty-checked that's when you'll want to watch out for perf issues. This probably means you need to refactor things so that dirty-checking or observing a lot of expressions/properties isn't necessary.
  2. For scenarios like get fullName() { return this.user.firstName + ' ' + this.user.lastName; }
    Why not dirty-check?
  3. For scenarios where you'd want @computedFrom('items[].amount'), there's a point at which the cost of observing the amount property on each item in the array exceeds the cost of periodically calculating the sum. Dirty checking the "total" getter is a perfectly good approach. Alternatively you could also setup your binding such that dirty-checking isn't necessary, for example:
    <input repeat.for="item of items" value.one-way="item.amount" change.delegate="updateItemAmountAndTotal(item, $event.target.value)">

In my experience there's usually a much better way to achieve the same result without observing tons of properties/expressions on arrays of objects. Making @computedFrom more advanced won't make apps more performant and instead might make it easier for people to shoot themselves in the foot.

All that said, it should be pretty easy to enhance computedFrom to the point where it can understand the same expressions Aurelia supports in bindings, I'll review the existing PR for that and merge or suggest changes as-needed.

Be sure to use the upcoming "improved" computedFrom with caution...

@jods4
Copy link
Contributor Author

jods4 commented Feb 9, 2016

@jdanyow I already saw the "sometimes dirty checking is better" argument. Most of the time I don't buy it and when I do I turn to alternative solutions.

Dirty checking is a dangerous rabbit hole. It's very easy to be unaware how many properties are dirty-checked. "A ton" can be a lot less than you'd expect before you have perf issues.

Even if you have a reasonable amount a quickly computed properties that are dirty checked, that periodic process (several times per second) can have negative impacts. It's not good for your battery life. It may interfere with your smooth, 60fps animations.

Refactoring things to observe less is good when you can. At the same time, change observation is the foundation of frameworks such as Aurelia. You do it a lot and it's often the most elegant way. It has to provide all the bells and whistles to be efficiently supported.

For scenarios like get fullName() { return this.user.firstName + ' ' + this.user.lastName; }
Why not dirty-check?

Because that's exactly the kind of scenario where precise observation is more efficient and easy. Why dirty-check?

Some properties that are dirty checked may create a non-trivial amount of work. Also a non-trivial GC pressure, which is also important perf-wise. Especially examples that involve arrays... filtering then reducing a 1'000 elements array 10 times per second is not that cheap. If you do that often, your dirty checking costs can go up quickly. No need for tons of watched properties.

there's a point at which the cost of observing the amount property on each item in the array exceeds the cost of periodically calculating the sum.

Two comments: first it's comparing apples to oranges, because the costs are spread very differently. In one case the cost is additional memory + dependencies set-up at each change (better yet: offer a "one-time" mode for cases where we know the dependencies won't change after evaluation), in the other it's constant CPU usage (+ possible memory). So the comparison very much depends on how frequently data changes. Typically in front-ends (user driven): not very much. If you have optimized code when dependencies don't change, it's only a one-time fee + some memory, that's actually hard to beat.

Second, this is how it was typically done with KO and my practical experience is that it worked very nicely, quite a long way. Actually I never encountered a perf problem with this in my projects -- granted, I never did silly things such as watch a 10'000 items array.

Making @computedFrom more advanced won't make apps more performant and instead might make it easier for people to shoot themselves in the foot.

I'd argue that dirty checking is very much an easy way to shoot yourself in the foot. In the end, if it's so much better, why did Aurelia not use it for all its observables -- like Angular did? What is special in the core framework that it prefers instrumenting (lots of properties obviously) over plain dirty checking? I think this goes to show which one of the two scales best.

In my experience there's usually a much better way to achieve the same result without observing tons of properties/expressions on arrays of objects.

Well yes. If I need to observe huge things like a 1000 elements array, I'll probably rather use a signal. Or update things incrementally when I can. But neither a computedFrom nor dirty checking. I think it was said support for signals was coming and this is a good news.

I don't quite like your option with change.delegate because it puts core logic in the UI. It means any change that is triggered by some business logic is lost. Which is IMHO not a good design and against proper MVVM.

@gheoan
Copy link
Contributor

gheoan commented Feb 9, 2016

@jdanyow

  1. You are right. However, there is no way to find how many properties are dirty checked. Some console message would be helpful (aurelia-computed does that).
  2. That question can be asked for any getter that could use @computedFrom.
  3. What if the amount property is already observed? I'm not sure how the observer-subscribers relation works, but there should be only one observer and one/many subscribers. If that's right the cost of adding one more subscriber (for each item) should be lower than dirty checking the sum.

@jsobell
Copy link

jsobell commented Feb 20, 2016

Nobody wants an inefficient solution, but if we have a choice between being unable to detect changes and do it sub-optimally, I'll got the sub-optimal approach until someone finds a way of optimising the process.
I currently have to bodge dependent variables as dummy params into bindings to force re-evaluation of functions that take any parameters (i.e. can't be made a get accessor), meaning I'm exposing my functional dependencies into the View.
Likewise with personChanged() not firing on @bindable person if I modify person.firstName. This is fundamental stuff.
Are we obsessing with optimisation to the point that we're making development difficult? How about a convention of "make it possible now, and improve performance later"?

@sormy
Copy link

sormy commented Mar 12, 2016

I have simple fix to get dot notation supported in computedFrom annotation: #345
That fix will also enable property name syntax validation and aurelia will throw an error if syntax is not correct.

@davismj
Copy link
Member

davismj commented Mar 15, 2016

Good stuff @jdanyow !

@ggrimbert
Copy link

Hi,

Can you provide an example of the use of computedFrom with dot property ?

I tried computedFrom("MyObject.property") but this does not work.

@ClareBear85
Copy link

@ggrimbert this is the same for me config.computedFrom(['baseContent.ValidFromTime', 'baseContent.ValidToDate', 'baseContent.ValidToTime'])

@sormy
Copy link

sormy commented Apr 7, 2016

FYI, aurelia bindings can't bind normally to Date objects. In my case I have to use and bind to value property of Date object:

if (!Date.prototype.value) {
  Object.defineProperty(Date.prototype, 'value', {
    get: function() {
      return this.valueOf()
    }
  });
}

But that stills fall back to dirty checking to value property of Date object.

PS: I developed aurelia-stats to track digest loop for aurelia, will release soon.

@sormy
Copy link

sormy commented Apr 21, 2016

@ClareBear85, if you have Date objects as properties and you would like to bind them with @computedFrom() then I would like to suggest you https://github.com/sormy/aurelia-date-observer

@Nepoxx
Copy link

Nepoxx commented Oct 3, 2016

Is there a way to make this work with dynamic values?

If I use

  get changed() {
    return this.initialValue == this.model.data[this.model.key];
  }

The value is dirty checked, however if I use $.initialValue == model.data[model.key]; ? 'bar' : 'foo'} , the value is not dirty checked.

@jsobell
Copy link

jsobell commented Oct 3, 2016

${changed ? 'bar' : 'foo'}

Are you trying to avoid this?
If you want to avoid dirty checking you need to intercept every dependency's property setter. That was the original idea behind allowing @computedFrom on functions.

@Nepoxx
Copy link

Nepoxx commented Oct 3, 2016

${changed ? 'bar' : 'foo'} is what I want but this results in dirty checking. I can't use @computedFrom because my values used to compute are dynamic.

@jsobell
Copy link

jsobell commented Oct 3, 2016

@Nepoxx Did you try this with aurelia-computed?

@jsobell
Copy link

jsobell commented Oct 3, 2016

@Nepoxx Also, see this issue: #147

@khenderick
Copy link

What about @computedFrom('initialValue', 'model.data', 'model.key')?

@Nepoxx
Copy link

Nepoxx commented Oct 6, 2016

@khenderick That works but it'll be triggered whenever model.data changes, not when model.data[model.key] changes. It's not a perfect solution but it's definitely an improvement :)

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

Successfully merging a pull request may close this issue.