valueConverter doesn't observe array-valued object properties

[davismj]

I'm filtering a list of items of various colors based on an array of selected colors:

repeat.for="item of items | filter: { property: 'color', value: selectedColors }

But toView is not triggered when I add or remove colors to the selectedColors array.

[davismj]

Specifically, I was adding items to the array using

<input type="checkbox" checked.bind="selectedColors" value="red" />

[davismj]

However, it does work as expected if I do this:

this.selectedColors = this.selectedColors.concat(["red"]);

Which I guess makes sense, as it is Object.observeing.

[davismj]

Different use case, also doesn't work.

repeat.for="item of items | filter: selectedColors

[dev-zb]

Tested it in a new skeleton with the same result. But if you add selectedColors.length to the object

filter: { ..., update: selectedColors.length }

it updates on push/etc. Or you could use signal to trigger the update... 😟

[davismj]

I think one solution that might solve it would be to enhance the property observation strategy when the property is an array. So, right here, we would add the following

} else if (obj[propertyName] instanceof Array) {
    return new SetOrModifyArrayObserver(this.dirtyChecker, obj, propertyName);
}

Which would combine the behaviors of the SetterObserver, setting the subscriptions on the object, notifying on referential changes, and the ModifyArrayObserver, notifying on record add/removes.

What do you think, @EisenbergEffect and @jdanyow? Is this a viable strategy?

[davismj]

@z-brooks Thanks for checking, dude! I'm always finding out that my issues are due to my own faults in code, so it's relieving to have the review!

Also, nice workaround using .length. I'll use that until we resolve this issue.

[jdanyow]

hmm this should work- I had enhanced the repeat not too long ago to add support for this very feature. There are tests for this scenario here: https://github.com/aurelia/templating-resources/blob/master/test/repeat-integration.spec.js#L388

@davismj could you put together a plunker that reproduces this? If you feel like looking into the issue, the repeat should be hitting this piece of code:

https://github.com/aurelia/templating-resources/blob/master/src/repeat.js#L180

What it's doing is "unwrapping" the expression so that it observes items for mutations instead of the array returned from the value converter (which wouldn't be worth observing)

[davismj]

Will do.

[davismj]

Sorry for the wait @jdanyow, see below.
http://plnkr.co/edit/QqXNOgOdbWVtItoIomzT?p=preview

[jdanyow]

no worries- are you sure this is the same issue? I thought the original issue had to do with the repeat?

There's no bug in the plunker- pushing an item into an array doesn't trigger a property change so the binding doesn't update.

[davismj]

Nothing to do with the repeat, the array in question was the array property passed into the valueConverter as in the example.

The problem is that the binding should update. If we're setting up a property observer, we probably care about the value of the property. If the property is an array, the most important value change of the property will be add/removes.

I think it would be worthwhile to add a new type of property observer for arrays that adds the best of property observation and array observation, since in the above plunkr, the behavior is quite unexpected to the developer, don't you?

[jdanyow]

Totally understand why you expect it to update- put simply, something changed... update the binding.

The thing is, the binding system has never eagerly observed collection mutation. There's no logic (outside of the repeat) that checks this part of the expression evaluated to an array/map/set/etc... observe for mutation... Well that's a little bit of a lie- there's one spot that has a hack to support this feature request but it only handles arrays and it doesn't really support a production use case. How often is someone really going to use string interpolation with a raw array? We should probably remove it because the logic to make it work is fairly complicated and ultimately makes this one scenario inconsistent with the way things are handled everywhere else in the binding system.

Adding logic to observe collection mutation everywhere an expression (or part of an expression in this case) evaluates to an array/map/set/etc would be a pretty radical change with serious perf implications. Would need to defer to @EisenbergEffect.

Hope all that makes sense. Apologies for the wall of text.

[davismj]

Adding logic to observe collection mutation everywhere an expression (or part of an expression in this case) evaluates to an array/map/set/etc would be a pretty radical change with serious perf implications.

I think the most important thing to note here is that the only thing holding us back is that it would be a radical change. My guess is that recursively observing properties of the object, including array objects, is expected.

[EisenbergEffect]

We can't do deep observation of collections by default. That would be very, very bad. I'm willing to say it could potentially ruin Aurelia's performance and jeopardize the framework.

@jdanyow Would there be some way we could enhance value converters so developers could possibly provide an arbitrary observable, that whenever it changed would signal the result of the converter had changed? If that were possible, a developer could write a filter converter that filtered the array and also deeply watched the filter property on each item. This would be a way to handle this solution with precisely the amount of observation desired and not more. Thoughts?

[davismj]

At the end of the day, the goal is to pass named arguments into the HTML bindings. I'm using object notation to achieve that, but perhaps there's a better way.

I know there is a syntax along the lines of array | filter:value:property:method for passing in multiple arguments, but the problem I'm having with this approach is that sometime one or both of property or method will not be included.

Perhaps one solution would be to enhance this syntax? In fact, we could potentially use JavaScript object notation and simply not read the object as an object literal?

[davismj]

Also, just along the same lines, I ran into the following question on StackOverflow, where the developer tries to use @computedFrom on an array. Which is basically the same issue (and I've updated the plunkr. Just FYI.

[jdanyow]

I think array | filter:value:property:method and array | { filter, value, property, method } will work equally well in the current binding system. The problem is when filter / value / property / method is an array, mutating the array does not cause the binding to reevaluate because no properties changed. This forces developers to use this.value.push('something'); this.value = this.value.slice(0); to trigger the property change.

Maybe we could add something like array | filter:$mutates(value):property:method as an explicit instruction to the binding system that we need to observe collection mutation.

@davismj / @EisenbergEffect / @z-brooks what do you think?

[davismj]

I personally think it's too verbose. If the dev is writing array | filter:value where value is an array, he will 100% of the time want array observation. If any behavior is less appropriate, it would be observing a reassignment of the property.

I would also note that array | filter:value:property:method does not accommodate named variables, for cases where arguments are optional.

[jdanyow]

Sorry, I wasn't clear, what I meant was array | filter:value:property:method and array | { filter, value, property, method } will have the same change detection characteristics.

In terms of the verbosity, I totally understand, was just trying to offer some sort of workaround. The problem is we can't change the binding system to check for Array/Map/Set/Promise/Observable/etc at each point in the expression tree without taking a performance hit, especially if we eagerly observe the collection for mutation at all those points. (only mentioning Promise/Observable because a similar request is being discussed in another issue).

[davismj]

The problem is we can't change the binding system to check for Array/Map/Set/Promise/Observable/etc at each point in the expression tree without taking a performance hit

If a dev always writes array | filter:$mutates(value) on every array, would it not have approximately the same performance implication that automatically creating a collection observer would?

[EisenbergEffect]

What if we handled this issue and the issue of computed properties with a similar solution? What if we start by expanding the syntax for computedFrom? Here's a few things we might do:

Dependent on a simple property change:
@computedFrom('firstName', 'lastName')

Dependent on property change and array value mutations:
@computedFrom('lineItems[]')

Dependent on property change, array value mutations and array item property:
@computedFrom('lineItems[].price')

Perhaps a nice TypeScript version via to-stringing function bodies of lambdas, using special known args.
@computedFrom<Order>($index => this.lineItems[$index].price)

Then, once this basic capability is in place, we could build value converter updates on top of it by introducing a new, specially-recognized ObservableValue type. Value Converters could return this to provide not only the converted value, but also metadata about how that converter might need to update.

export class FilterValueConverter {
  toView(value, config) {
    return new ObservableValue(
      value.filter(x => x[config.property] === config.value), //the converted value
      '$value[].' + config.property //the observation expression(s) to indicate when to refresh
    );
  }
}

@jdanyow What do you think? Perhaps ObservableValue could even be something recognized in other places to provide control to end users? Am I crazy? These are our two biggest limitations so I was trying to see if we could kill two birds with one stone and maybe even open up some new possibilities we hadn't thought of in other places.

[timfish]

If we're to use ValueConverters for filtering, being able to observe the properties we're filtering by is quite essential. Is signal the least hacky way I can get round this until the above is implemented?

[jdanyow]

@timfish give this binding behavior a try: https://gist.run/?id=61b94e889c95d4336de7

aurelia/templating-binding#86

[timfish]

@jdanyow I was hoping to specify a property that is observed on each item in the array which would cause my filter value-converter to be re-evaluated. Along the lines of what you would get with
@computedFrom('lineItems[].price')

You have made me realise quite how powerful binding behaviours can be though and I put together this: https://gist.run/?id=b5de591668766a0923209507b03f9fac which allows me to do:
repeat.for="item of items | filter & array:'name'"

I haven't had a chance to look at the internals of aurelia yet, so I don't know the implications of much of what I'm doing here but it seems to work in that basic example...

function observeProperty(obj, property) {
  this.standardObserveProperty(obj, property);
  let value = obj[property];
  if (Array.isArray(value)) {
    this.observeArray(value);
    for(let each of value){
      this.standardObserveProperty(each, this.propertyName); 
    }
  }
}

export class ArrayBindingBehavior {
  bind(binding, source, property) {
    binding.propertyName = property;
    binding.standardObserveProperty = binding.observeProperty;
    binding.observeProperty = observeProperty;
  }

  unbind(binding, source) {
    binding.observeProperty = binding.standardObserveProperty;
    binding.standardObserveProperty = null;
    delete binding.propertyName;
  }
}

[akircher]

@jdanyow @EisenbergEffect
I have also run into the same problem as @timfish many times. If there is a value converter on an array of objects - and one of the object property changes - the expected behavior is for the value converter to be rerun.

While I understand performance considerations for arbitrarily deep observation by default. Could we get away with just one level deep by default for arrays? I would venture to say that a property changing on an object in an array is just as common a use-case as directly modifying an array with .pop(), .shift(), etc.

I suggest that aurelia have a built-in & deep or & nested binding behavior that would signal the binding engine to observe more than one level down. It could take an optional parameter of how many levels/nestings down it will observe. To make it even more performant there could be an optional second parameter in which you give the property name so it only looks for that one property: so either & deep:1 or & deep:1:'price' would work for the @timfish example (with the latter being more efficient).

Is this feasible? If performance is still an issue even at only one level deep for arrays, maybe you could force the developer to use & deep:1 for observing properties in an array of objects but again this is a very common use case so it would be better if we could avoid this boilerplate and do it by default.

[akircher]

By the way here are the workarounds @jdanyow currently suggests:

http://stackoverflow.com/questions/32017532/observe-property-on-an-array-of-objects-for-any-changes

But I think we can come up with something even better!

[timfish]

Although it's possible to fix this with binding behaviours, its a bit hacky and just a stopgap until we have something more permanent. @EisenbergEffect suggestion to return something from the value converter should be the long term goal.

[akircher]

@timfish binding behaviors seem more elegant to me than introducing new syntax to value converters. Also what if I want to do property observation on an array of object without a valueConverter? This is a common scenario for me and I wouldn't want to create a value converter just to handle it.

[AdamWillden]

This is is already a very common requirement and working 'around it' feels wrong. I agree a permanent solution is required and I like the idea of using the ObservableValue as @EisenbergEffect described above

[akircher]

Thinking about this more, I withdraw my & deep binding behavior proposal. Its flaw is that it depends on the consumer of the array knowing how it will be modified. This works if you define and modify the array yourself but is problematic if you consume someone else's array.

That being said, I still think we can do better than ObservableValue. I feel that it goes against the aurelia philosophy of being unobtrusive. Is there some combination of decorators, binding behaviors, or method hooks that could achieve the same effect without making us pass around aurelia-specific constructors?

[akircher]

Also minor suggestion on the @computedFrom syntax. Both Mongo and Couch 2.0 both use lineItems.price rather than lineItems[].price for specifying deep fields i.e., the array is handled implicitly. Unless there is a reason to deviate I would prefer a consistent syntax.

[fopsdev]

i also ran into this.
my use case: having a editable (.bind) list displaying cached entities from a singleton. then in the same view i display a similar list with the same entities. i would now expect the property to change in both the lists upon editing.

maybe a stupid idea but why not decorate all the properties which should be observed on property level inside the class constructor:

        @observe()
        this.myString = 'foo';

        // observes array for adds and removes as it is right now
        @observe()
        this.myArray = [];
        // sample for additionally observes array for changes on props and nested props
        @observe({properties:['name','age','manager.name']})
        this.myArray = [];

[EisenbergEffect]

Closing since this was initially a question and we are tracking various considerations for improvements elsewhere.

[JSeligsohn]

@EisenbergEffect Can you link to those various considerations?

[javadbat]

@EisenbergEffect what happen to this topic ?
how can we enable deep observe for one of my object or trigger change after change it ?

[arnonuem]

This is what i did:

assignedRoles = [];

addRole( role ) {
    // begin workaround
    let oldAssignments = this.assignedRoles;
    this.assignedRoles = [];
    this.assignedRoles = oldAssignments;
    // end workaround

    //This triggers the attached value converter (but only when using the workaround).
    if ( !_.includes( this.assignedRoles, role ) ) {
      this.assignedRoles.push( role );
    }
}

This triggers my ValueConverter twice always but at least it is triggered when adding a second item to the array. Since its just a ValueConverter which causes no sideEffects to my business code I don´t care that its triggered twice.

This was the easiest workaround to me. Hope this issue will be solved soon.