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 observe lists #249

Closed
davismj opened this Issue Nov 30, 2015 · 23 comments

Comments

Projects
None yet
10 participants
@davismj
Member

davismj commented Nov 30, 2015

In other words.

@computedFrom('filter', 'list')
get filteredList() {
  // filter the list
}
list.splice(index, 1); // filteredList doesn't recompute

@jdanyow jdanyow added the enhancement label Dec 4, 2015

@andrevlins

This comment has been minimized.

Show comment
Hide comment
@andrevlins

andrevlins Dec 7, 2015

there are any workaround this?

andrevlins commented Dec 7, 2015

there are any workaround this?

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Dec 7, 2015

Member

one workaround would be to write this.list = this.list.slice(0).splice(index, 1) instead of this.list.splice(index, 1),

Member

jdanyow commented Dec 7, 2015

one workaround would be to write this.list = this.list.slice(0).splice(index, 1) instead of this.list.splice(index, 1),

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Dec 8, 2015

Member

@andrevlins So this is actually a bad design practice. Observing lists via splice is working, and modifying a list with another variable through value converters is working. Thus if you want a computed list you should bind the list and add a value convert to modify it.

However, I think we're agreed that this should eventually work, too, @jdanyow? Since there might be a case where you'd want to access the modified list in a viewModel rather than simply bound to the view.

Member

davismj commented Dec 8, 2015

@andrevlins So this is actually a bad design practice. Observing lists via splice is working, and modifying a list with another variable through value converters is working. Thus if you want a computed list you should bind the list and add a value convert to modify it.

However, I think we're agreed that this should eventually work, too, @jdanyow? Since there might be a case where you'd want to access the modified list in a viewModel rather than simply bound to the view.

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Dec 13, 2015

Member

@davismj I agree, the best way to do this would be with a value converter.

There are other enhancement requests for @computedFrom so I think we should leave this open and revisit when we refactor that component.

Member

jdanyow commented Dec 13, 2015

@davismj I agree, the best way to do this would be with a value converter.

There are other enhancement requests for @computedFrom so I think we should leave this open and revisit when we refactor that component.

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Dec 28, 2015

Member

related to #149

Member

jdanyow commented Dec 28, 2015

related to #149

@AlbertoMonteiro

This comment has been minimized.

Show comment
Hide comment
@AlbertoMonteiro

AlbertoMonteiro Jul 15, 2016

Using computedFrom with checked.bind to a list also doest not work

app.ts

import { computedFrom } from "aurelia-framework";

export class App {
    selectedOptions = [];
    options = [1, 2, 3, 4];

    @computedFrom("selectedOptions")
    get selectedItemsSeparetedWithComma(): string {
        return this.selectedOptions.join(", ");
    }
}

app.html

<template>
  <ul>
    <li repeat.for="item of options">
      <label><input type="checkbox" model.bind="item" checked.bind="$parent.selectedOptions" >${item}</label>
    </li>
  </ul>
  <div>${selectedItemsSeparetedWithComma}</div>
</template>

selectedItemsSeparetedWithComma is never fired when selectedOptions change.

AlbertoMonteiro commented Jul 15, 2016

Using computedFrom with checked.bind to a list also doest not work

app.ts

import { computedFrom } from "aurelia-framework";

export class App {
    selectedOptions = [];
    options = [1, 2, 3, 4];

    @computedFrom("selectedOptions")
    get selectedItemsSeparetedWithComma(): string {
        return this.selectedOptions.join(", ");
    }
}

app.html

<template>
  <ul>
    <li repeat.for="item of options">
      <label><input type="checkbox" model.bind="item" checked.bind="$parent.selectedOptions" >${item}</label>
    </li>
  </ul>
  <div>${selectedItemsSeparetedWithComma}</div>
</template>

selectedItemsSeparetedWithComma is never fired when selectedOptions change.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Jul 15, 2016

Member

Using @computedFrom with any array mutation will not work.

Member

davismj commented Jul 15, 2016

Using @computedFrom with any array mutation will not work.

@AlbertoMonteiro

This comment has been minimized.

Show comment
Hide comment
@AlbertoMonteiro

AlbertoMonteiro Jul 15, 2016

@davismj how can I make it work?

AlbertoMonteiro commented Jul 15, 2016

@davismj how can I make it work?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Jul 15, 2016

Member

I recommend creating a value converter. Try looking at this article by Rob Eisenberg for more information on value converters.

Member

davismj commented Jul 15, 2016

I recommend creating a value converter. Try looking at this article by Rob Eisenberg for more information on value converters.

@AlbertoMonteiro

This comment has been minimized.

Show comment
Hide comment
@AlbertoMonteiro

AlbertoMonteiro commented Jul 15, 2016

@davismj thanks!

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Oct 14, 2016

@jdanyow, any progress with @computedFrom('someArray') and array mutations? I stumbled upon issue with someArray.push(someElem).

atsu85 commented Oct 14, 2016

@jdanyow, any progress with @computedFrom('someArray') and array mutations? I stumbled upon issue with someArray.push(someElem).

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Oct 14, 2016

@jdanyow, I might be totally wrong as I'm not familiar with binging logic, but could my gut feeling be correct, that this problem might be related to this line that uses ExpressionObserver, but not CollectionObserver?

atsu85 commented Oct 14, 2016

@jdanyow, I might be totally wrong as I'm not familiar with binging logic, but could my gut feeling be correct, that this problem might be related to this line that uses ExpressionObserver, but not CollectionObserver?

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Oct 15, 2016

Member

@atsu85 - the binding system observes property changes- if you put <input value.bind="foo.bar.baz"> in an html template or in @computedFrom('foo.bar.baz'), the binding system will observe the foo property for changes, foo's bar property for changes and bar's baz property for changes. It doesn't check "is the value an array? yes? also observe the array for mutation".

Perhaps we could do that in the @computedFrom use-case or maybe we need an explicit instruction to do so- something like: @computedFrom('foo.bar.baz', { collection: true, expression: 'cart.items' })

Member

jdanyow commented Oct 15, 2016

@atsu85 - the binding system observes property changes- if you put <input value.bind="foo.bar.baz"> in an html template or in @computedFrom('foo.bar.baz'), the binding system will observe the foo property for changes, foo's bar property for changes and bar's baz property for changes. It doesn't check "is the value an array? yes? also observe the array for mutation".

Perhaps we could do that in the @computedFrom use-case or maybe we need an explicit instruction to do so- something like: @computedFrom('foo.bar.baz', { collection: true, expression: 'cart.items' })

@StrahilKazlachev

This comment has been minimized.

Show comment
Hide comment
@StrahilKazlachev

StrahilKazlachev Oct 15, 2016

Contributor

@jdanyow I'm for an explicit option. We should be able to differentiate between property change and changes to the value.

Contributor

StrahilKazlachev commented Oct 15, 2016

@jdanyow I'm for an explicit option. We should be able to differentiate between property change and changes to the value.

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Oct 15, 2016

@jdanyow,

Perhaps we could do that in the @computedFrom use-case or maybe we need an explicit instruction to do so- something like: @computedFrom('foo.bar.baz', { collection: true, expression: 'cart.items' })

I understand, that if I'd need to use @computedFrom("bindableArray.bar.baz") instead of @computedFrom("bindableArray"), then there might be a need to explicitly instruct that CollectionObserver should be used, but when one of the computed properties is actually a array, then I'd expect that computedFrom decorator detects that bindableArray is array and it then additionally uses CollectionObserver, so it could also detect array modifications (such as .push(newElem)) in addition to replacing array instance (that is implemented right now).

Here is an example of my use-case:

<template>
  ${computedGetter.value}
<template>

with

  @computedFrom("bindableArray", "bindableId")
  get computedGetter() {
    // return item from `this.bindableArray` by `this.bindableId`
  }

atsu85 commented Oct 15, 2016

@jdanyow,

Perhaps we could do that in the @computedFrom use-case or maybe we need an explicit instruction to do so- something like: @computedFrom('foo.bar.baz', { collection: true, expression: 'cart.items' })

I understand, that if I'd need to use @computedFrom("bindableArray.bar.baz") instead of @computedFrom("bindableArray"), then there might be a need to explicitly instruct that CollectionObserver should be used, but when one of the computed properties is actually a array, then I'd expect that computedFrom decorator detects that bindableArray is array and it then additionally uses CollectionObserver, so it could also detect array modifications (such as .push(newElem)) in addition to replacing array instance (that is implemented right now).

Here is an example of my use-case:

<template>
  ${computedGetter.value}
<template>

with

  @computedFrom("bindableArray", "bindableId")
  get computedGetter() {
    // return item from `this.bindableArray` by `this.bindableId`
  }
@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Oct 15, 2016

Member

@atsu85 As mentioned above, I haven't seen a case where this couldn't be solved with a value converter, which is much more efficient. So instead you might have:

<template>
  ${bindableArray | filter: bindableId}
</template>
export class FilterValueConverter {
  toView(array, id) {
    // return item from `array` by `id`
  }
}

I've even written a small library that does this for you here: http://foursails.github.io/bouncer/

Member

davismj commented Oct 15, 2016

@atsu85 As mentioned above, I haven't seen a case where this couldn't be solved with a value converter, which is much more efficient. So instead you might have:

<template>
  ${bindableArray | filter: bindableId}
</template>
export class FilterValueConverter {
  toView(array, id) {
    // return item from `array` by `id`
  }
}

I've even written a small library that does this for you here: http://foursails.github.io/bouncer/

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Oct 15, 2016

Member

@jdanyow As the person who originally opened this issue, I recommend we close it, as I don't see this as a valuable enhancement. What do you think?

Member

davismj commented Oct 15, 2016

@jdanyow As the person who originally opened this issue, I recommend we close it, as I don't see this as a valuable enhancement. What do you think?

@atsu85

This comment has been minimized.

Show comment
Hide comment
@atsu85

atsu85 Oct 15, 2016

@davismj, my example was a lot simplified from real use-case. I also need the result of computedGetter in other methods of the class.

atsu85 commented Oct 15, 2016

@davismj, my example was a lot simplified from real use-case. I also need the result of computedGetter in other methods of the class.

@Thanood

This comment has been minimized.

Show comment
Hide comment
@Thanood

Thanood Oct 15, 2016

Is it intended that the value converter (jsonValueConverter) in this gist does not update when I change the array?
https://gist.run/?id=82c48de63159f1b1d39c2658ab748bf7

Thanood commented Oct 15, 2016

Is it intended that the value converter (jsonValueConverter) in this gist does not update when I change the array?
https://gist.run/?id=82c48de63159f1b1d39c2658ab748bf7

@carusology

This comment has been minimized.

Show comment
Hide comment
@carusology

carusology Feb 23, 2017

Even if the official solution is "use a value converter", an error message would be much more valuable than the current approach of "try it in futility for an hour, google it and find this issue." Can @computedFrom do a typeof check to see if the observed property is an array, and throw an error if so?

carusology commented Feb 23, 2017

Even if the official solution is "use a value converter", an error message would be much more valuable than the current approach of "try it in futility for an hour, google it and find this issue." Can @computedFrom do a typeof check to see if the observed property is an array, and throw an error if so?

@dkent600

This comment has been minimized.

Show comment
Hide comment
@dkent600

dkent600 Jun 27, 2017

Value converters don't work at all for this use case.

dkent600 commented Jun 27, 2017

Value converters don't work at all for this use case.

@davismj davismj closed this Nov 6, 2017

@zakjan

This comment has been minimized.

Show comment
Hide comment
@zakjan

zakjan Nov 6, 2017

Observing array length with @computedFrom("array.length") works, is it officially supported?

zakjan commented Nov 6, 2017

Observing array length with @computedFrom("array.length") works, is it officially supported?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Nov 6, 2017

Member

Yep! That is a completely viable strategy. See https://github.com/aurelia/binding/blob/master/src/observer-locator.js#L159

This will not observe changes to array elements, however. So if you do list[1] = 'new value';, it will not recompute.

Member

davismj commented Nov 6, 2017

Yep! That is a completely viable strategy. See https://github.com/aurelia/binding/blob/master/src/observer-locator.js#L159

This will not observe changes to array elements, however. So if you do list[1] = 'new value';, it will not recompute.

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