Property update notifications happen for all state changes, not only the relevant plucked properties

[Kennethtruyers]

I'm submitting a bug report

• Library Version:
  1.1.0

Please tell us about your environment:

• Operating System:
  Windows 10

• Node Version:
  8.9.3

• NPM Version:
  5.8.0

• JSPM OR Webpack AND Version
  WebPack 3.10.0

• Browser:
  all

• Language:
  all

Current behavior:
Given the following code:

@autoinject()
@connectTo({
  selector: {
    list1: store => store.state.pipe(pluck('list1')),
    list2: store => store.state.pipe(pluck('list2'))
  }
})
export class App {
  store;
  list1 : string[] = [];
  list2 : string[] = [];
  logs : string[] = [];

  constructor(store : Store<any>) {
    this.store = store;
    this.store.registerAction('addList1', addList1);
    this.store.registerAction('addList2', addList2);
  }

  list1Changed(newValue, oldValue) {
    this.logs.push('list 1 changed, newValue and oldValue are ' + (newValue === oldValue ? '' : ' not ') + 'equal' );
  }
  list2Changed(newValue, oldValue) {
    this.logs.push('list 2 changed, newValue and oldValue are ' + (newValue === oldValue ? '' : ' not ') + 'equal' );
  }
}

When we add an item to list1 or to list2 both list1Changed and list2Changed are called

• What is the expected behavior?
  Only list1Changed should be called when an item is added tolist1 (and vice versa for list2)

Gist: https://gist.run/?id=75aadba05ee34f0e9c0edc10da2d917a

• What is the motivation / use case for changing the behavior?
  Avoiding unnecessary updates and screen repaints on all state properties whenever one property changes

[zewa666]

The choice whether you'd like to get notified depends on the actual subscription and how often it fires.
You're creating list1 and list2 subscriptions which pluck parts of the whole state. But by default every next emit from the BehaviorSubject, will cause the Subscriptions to retrigger, no matter what changed as you haven't told what the condition is.

In RxJS you would solve this using the distinctUntilChanged operator.

@connectTo({
  selector: {
    list1: store => store.state.pipe(pluck('list1'), distinctUntilChanged()),
    list2: store => store.state.pipe(pluck('list2'), distinctUntilChanged())
  }
})

There are tons of other crazy operators you can use to mix and match. So it's really up to the consumer (subscription) to decide what he wants as the only sane default is, fire always ;)

[Kennethtruyers]

Thanks for the quick response, this is a good workaround
However, I don't think this is really obvious behavior. It doesn't work that way with normal bindings either.

In the documentation, it mentions With multiple selectors, you get the same change handling per selector.

The per selector here is a bit misleading. It also doesn't make much sense to have two separate methods for change handling if they are going to be triggered by every change any way.

I think the more obvious behavior would be to default to this one and then have a potential override for the previous behavior

[Kennethtruyers]

Another thing that is different from the normal observables is that whenever the change-handler is triggered, the property itself hasn't been updated yet, whereas with a normal observable when you get the change notification, the property will already be set correctly

[zewa666]

I do understand your concerns, still there is a reason why there's the distinctUntilChanged operator and it is not used as default for subscriptions. I wouldn't really see a different way of solving that properly without applying the operator automatically and that does feel too much like black magic for me. Coming from RxJS in other projects it certainly wouldn't feel right to have operators automatically applied to your own subscriptions.

Would you feel like clarifying the docs, and adding the hint for distinctUntilChanged, could be enough here? If so a PR would be welcome to make it easier for you to frame it in your words.

With regards to the changed-handler behavior, this is documented at the end of this section

[Kennethtruyers]

OK, that sounds reasonable, I will have a look at the documentation to see if I can provide an update.

In terms of changing the API, maybe it's an idea to implement something like this:

pick(property : string) => 
    store => store.state.pipe(pluck(property), distinctUntilChanged())

such that you can do the following:

@connectTo({
  selector: {
    myProperty: pick('myProperty')
  }
})

This would abstract the RxJS behavior away, which I think is the main goal of this project.
(naming is just something I came up with to differentiate from the rxjs' pluck)

[zewa666]

great, feel free to modify parts of the docs that do not sound reasonable, we can work on the details in your PR if somethings unclear.

With regards to your helper function. This is indeed a practical way to even further reduce a bit of boilerplate. Providing that out of the box from the framework though feels a bit limiting. The trouble with the implementation of your pick function is that it's not composable. So it feels a bit like re-inventing the wheel to hide the complexity of RxJS.

What would you think of a companion package that could include helpers like these? There are for sure tons of opportunities which make sense in specific cases and just don't suite for sensible defaults nevertheless would provide help for other users. So e.g also various middlewares that might be of use.

The idea of the store plugin is to keep the essential parts with generic defaults so it does not limit the user. Accompanying packages could fill the gap of providing more specific solutions.

[Kennethtruyers]

Yeah, that could be a potential solution.

I think my main gripe with this behavior is that strays from the way Aurelia normally works.
From my point of view, the expectation after working with Aurelia is:

1. A changed handler gets called only when it actually changes
2. When a changed handler is called, the actual object has already changed

This is more about complying with expectations and thus making the transition smoother.

I could come up with a helper package to do 1, do you have any pointers on how you would go about accomplishing 2?

[zewa666]

I do get your point. State handling, and specifically in mix with RxJS, doesn't suite that too well though. So there is a hard fight between conforming with the Aurelia way, yet not sacrificing potential for RxJS based approaches.
There will hardly ever be a clear cut since, let's be honest, two-way bindings in general are countering the idea of centralised state-management. Given that the whole idea of having component references and multi-selectors at all is a bit of a grey zone, but I think we're on a good way to find a proper middle-ground.

With regards to 2, are you talking about the general changedHandler as a name or the individual change handler by selector?

[Kennethtruyers]

What I mean by 2 is for example this:

@autoinject()
@connectTo({
  selector: {
    list: store => store.state.pipe(pluck('list'))
  }
})
export class App {
  store;
  list : string[] = [];

  constructor(store : Store<any>) {
    this.store = store;
    this.store.registerAction('addList', addList);
  }

  listChanged(newValue, oldValue) {
    // this.list does not have the newValue yet, in contrast with normal 
    // aurelia behavior where this.list would have the value when this handler is called
  }
  }
}

So, a way of delaying the handler being invoked, until the value is set. This is especially handy in circumstances like this:

    listChanged(newValue, oldValue){
        this.recalculateTotals();
    }

    list2Changed(newValue, oldValue){
        this.recalculateTotals();
    }
    recalculateTotals(){
        this.total = this.list.sum() +this.list2.sum();  // this won't work, we'd need to pass in both lists to the method every time
    }

[zewa666]

Hmm yeah I see. All of that stemmed from the general changedHandler, which typically is used when no selector or whatever is provided. That one does not get the old state propagated but instead is fired before setting the new, so that users can poke this.state for back-comparisons. With the rewrite I guess all got changed to execute in this way.

@jmzagorski what do you think about this?

If we're going to do this, it's going to be a breaking change, since the order change might have dramatic effects, e.g. conditionals go wrong, for existing apps.

@Kennethtruyers I think it would make sense to extract this part as an issue for itself, as this one becomes heavily mixed with various things

[Kennethtruyers]

Yeah, completely agree, this is really a separate issue. I will create a separate ticket for it.

I also agree that it's a breaking change, so it would either need to be configurable or very well communicated.

[Kennethtruyers]

Ticket is here: #69

[zewa666]

Great, big thanks for support. Alright so with regards to the other issue lets see how the package comes along I've already some ideas I'd like to contribute

[Kennethtruyers]

Hm, thinking about it now, I think this change is a bit of a hack on top of it and I think there's a cleaner way to do this. With this implementation, you'd get a redundant declaration, because you have to define the plucked properties as values to the selector and then also as properties on the class. I think a nicer way to do this would be using attributes:

@autoinject()
@connectTo()
export class App {
  @state list : string[] = [];  // automatically plucks the property with the same name from the state and binds it
}

I have no idea whether this possible or how to do it though.

[timfish]

The docs for distinctUntilChanged say that:

If a comparator function is not provided, an equality check is used by default.

If every action returns a new state object, aren't the references going to be not equal and detect a change every time?

[Kennethtruyers]

Not really, the root state object may have changed, but the items inside the state will still be the same:

State 1 => 
   Object 1
   List 1

After changing List 1:

State 2 => 
    Object 1 <== Still points at the same object, even though the root has changed
    List 2 <== Only this has changed

[zewa666]

I have no idea whether this possible or how to do it though.

I suspect so. I remember having tried something similar in the begin without success :(

[jmzagorski]

lets see if i am following:

1. Although this is outside the scope of this issue, but in response to your last post: @zewa666 there are a few plugins that did/do property decoration aurelia-redux-plugin and angular-redux. For the aurelia repo from steelsojka, I did remember having issues with babel totally removing my property, but i never dug into it. I will be happy to look into this feature if you think it is worth it (a funny point based on this conversation is the angular repo uses distinctUntilChanged many times in its pipe to the selectors, not saying that is what we have to do, but thought I would point it out.)

2. I was not aware change handlers would fire for other properties, i guess i wrongly assumed subscribe would only watch that particular slice of state and not fire when other changes happened (sorry i have been slow to get up to speed with observables). Can we implement a default equality check before the change handler runs? I am pretty sure that is what aurelia-binding does. It would be a breaking change, unless this is considered a bug. We could put the check in with the infamous Object.entries loop that checks if the handlerName is in this

[Kennethtruyers]

@zewa666 I had a go at creating a decorator for this, but unfortunately got stuck as well. You can decorate the property and rewrite the getter/setter to it pulls from the state-object. I was unable to access the methods on the class the property belongs to though (to call the propertyChanged handler).

Aurelia can do it, because they pass the entire viewmodel through a configuration method and have the viewmodel in scope so they can call the handler

[zewa666]

I think this is maybe something to think about when heading for vNext support. Generally the Store is going to be one of the first plugins thus having the chance to actively shape the APIs.

What we should start to think about is maybe how the perfect, boilerplate free code may look like. Then we can figure out whats doable and what not. I feel like property decorators are going to play a role there but lets not limit us here. Looking forward to seeing your guys suggestions.

[Kennethtruyers]

Just came back to this issue and to answer you question @zewa666 for my personal liking, this would be the cleanest syntax possible:

1. Taking the same name as the property from the state and have distinctUntilChanged by default

    export class ViewModel{
        @state myProperty;
    }

2. Taking a nested name or custom name

    export class ViewModel{
        @state({ prop: 'someObj.someProp'}) myProperty;
    }

3. Customizing the change notifications

    export class ViewModel{
        @state({ behavior: stateBehavior.distinctUntilChanged | stateBehavior.always}) myProperty;
    }

Let me know your thoughts and if I can be of help