aurelia / templating-binding

An implementation of the templating engine's Binding Language abstraction which uses a pluggable command syntax.
MIT License
32 stars 26 forks source link

fix(InterpolationBinding): handle array mutation #27

Closed jdanyow closed 9 years ago

jdanyow commented 9 years ago

Fixes aurelia/framework#77

jdanyow commented 9 years ago

@EisenbergEffect this PR is not quite ready. I need to add tests. Wanted to see what you thought first. It's the bare minimum changes to fix https://github.com/aurelia/framework/issues/77

It does not support every scenario- here are some examples:

${arr}
${foo}
export class Contrived {
  arr = [1,2];
  foo = 'hello';

  doSomething() {
    // This will be interpolated (the fix covers this)
    this.arr.push(3);

    // This will be interpolated (the original implementation covers this)
    this.arr = [4,5,6];

    // This will NOT be interpolated.  The array subscription is on the original array instance
    // I did not add logic to track property changes and create new array subscriptions.
    this.arr.push(7);

    // Also, the current code lazily disposes the array subscription meaning it gets disposed with
    // everything else in the toDispose array (rather than the moment when a new array instance
    // is assigned to the property)
  }

  doSomethingElse() {
    // This will be interpolated (the original implementation covers this)
    this.foo = ['a','b','c'];

    // This will not be interpolated because the current logic only checks for an array instance
    // once, when "connect" is called.
    this.foo.push('d');
  }
}  

thoughts?

EisenbergEffect commented 9 years ago

What is the complexity involved in solving those two issues mentioned above? Is it a major headache or something that won't fit into the current paradigm? Or just need a bit more time to finish it up?

jdanyow commented 9 years ago

Definitely doable- I just wanted to make sure I wasn't over-thinking this and adding unnecessary overhead.

EisenbergEffect commented 9 years ago

Please move ahead. Let me know when you want me to review.

jdanyow commented 9 years ago

ready for review- beefed up the test coverage.

heads up- if you happen to try karma start --browsers IE, don't worry about the failing tests, I will send a separate PR for an issue in SetterObserver where it uses != instead of !== to detect changes: https://github.com/aurelia/binding/blob/master/src/property-observation.js#L28