New connect API with controllerAs? about ng-redux HOT 21 CLOSED

Hi @e-schultz, thanks for opening this issue.

I've been a strong advocate of explicitly requiring $scope, let me explain why. If it's not enforced explicitly, it's still required implicitly in order to unsubscribe. I'm very concerned that if it's not enforced explicitly, people will forget to unsubscribe (and thus create memory leaks), especially if they only have to unsubscribe manually only some of the time. I felt that the only exception I could think of (singleton components, that never get destroyed and therefore don't need to unsubscribe), didn't warrant the headache, since memory leaks are hard to track down.

You've addressed a number of issues, having read through them I think the current API should be able to handle all your cases elegantly, but please provide more feedback if I've misunderstood or are left unsatisfied.

• control binding: the control to which property you bind your selected state has moved to the selector. In your example you could do the following:

$ngRedux.connect($scope, state => ({
  vm: {
    thing: state.thing
  }
}))

That said, based on @wbuchwalter's last commit, I can tell that he has tried to detect whether the passed in $scope is from a controller using controllerAs and if it is, auto-bind to that property. In other words, the example you've shown should actually "just work", but perhaps there's a bug.

• use of connect in a service: would be interesting to hear a little bit more about your use-case, but my initial reaction would be that connect is really intended to connect you components to the store (where subscribing and unsubscribing are essential). Inside of a singleton service I would consider using $ngRedux.subscribe(callback) instead.
• ImmutableJS: connect needs to be able to merge the object returned from the selector with the $scope, the returned object therefore has to be a plain object, however the values nested within it should be allowed to be Immutable. i.e. the following should work:

$ngRedux.connect($scope, state => ({
  vm: {
    thing: Immutable.Map({a:1, b:2, c:3});
  }
}))

Edit: here's an example how you can keep your reselect selector generic and still bind to the controllerAs property of your $scope:

$ngRedux.connect($scope, state => ({
  vm: totalSelector(state)
}))

The controllerAs syntax unfortunately makes things a little more verbose, but like I said @wbuchwalter is trying to find a way to auto-detect whether controllerAs is used, if successful it would remove the need to wrap the returned result from the selector with the controller's property key.

from ng-redux.

@elgerlambert Thanks for the response

The use-case for using it in a service was having a service that is listening to postMessage events, and also wanting to get the most current value of something from the store before doing a dispatch.

Was using RxJs to get the postmessage events, filter things out until I got the one that I wanted - then using a value from the redux state to then dispatch an action.

I think in this case I might be able to use $ngRedux. subscribe, although would it be possible to use reselect here? Don't need it right now, just trying to think through to a case where I might - and how that might work.

I get what you are saying about the "$scope becoming an implicit requirement due to needing to unbind" - and this use case is going to be the most common one, the service-scenario being more of an edge case. Maybe $ngRedux.connect($scope || false,...rest), so in the cases where someone knows that they don't care about $scope - they have that option.

This does bring things back to the auto-bind to $scope - this is where I think there is a bit of a trade-off between auto-magic helping vs explicit control over what happens in response to a change in state.

Maybe there is a bug (either with ng-redux, or my code - going to play around in a smaller scale demo later today to see if I can figure out what the issue is).

Basically, went to upgrade to rc-4 due to the ngRedux rc-1 not playing nice with minification, and everything went boom - and led to a bit of refactoring. The issue that I had was when things were not working quite as expected - it felt like there was more mental-magic I needed to keep track of to figure out 'why isn't this getting bound as I expect', instead of it rather explicitly being in front of me.

Instead of

$ngRedux.connect(selector, cbState => {
  this.x = cbState.x;
})

and if in my view {{vm.x}} didn't get what I was expecting - there was very little mental overhead to track down where to look.

Now with things being pushed onto scope automatically,

$ngRedux.connect($scope, cbState => {
  return {
    vm: cbState.x
  }
})

I'm now wondering - am I looking at this.vm.x? this.x? $scope.vm.x? $scope.x? and then also needing to go back and double check 'what did I put as my controllerAs as?'- which is something I'm used with considering when working with templates, but when needing to debug issues with my controller left me scratching my head a bit.

Although your response got me thinking of where the issue might have been with my code - I'll see if I can get it working or not, and if I still have issues - I'll let you know, and see if I can get a quick demo/test case showing the issue.

Even so, I'm still unsure of how I feel about the tradeoff of the convenience auto-binding to $scope vs the loss of some explicit control.

from ng-redux.

Hmm, I guess the fact that your second snippet (with the new api) doesn't lead to the same result as the snippet before it, best demonstrates the point you're making. Should have been as follows (since this simply === $scope.vm in your example):

$ngRedux.connect($scope, state => {
  return {
    vm: {x: state.x}
  }
})

I personally consider auto-unbinding much more then a convenience for the reasons stated before. We've discussed allowing this to be passed in instead of $scope and leaving unbinding up to the consumer when using this. That would give you the following two options. You could write:

$ngRedux.connect($scope, state => {
  return { controllerAsKey: { key: stateSlice } }
})

Or

let unsubscribe = $ngRedux.connect(this, state => {
  return { key: stateSlice }
})

$scope.$on('$destory', unsubscribe)

Given the choice I would personally choose the former over the latter. And like I said, I strongly feel people are going to forget or ignore to unsubscribe, which concerns me.

On a side note, I think it would be better if $scope were passed in as the last argument, since the context is usually passed in as the last argument, but that's a different discussion.

For completeness this is what it would look like with the old callback api:

let unsubscribe = $ngRedux.connect(state => {
  return { key: stateSlice }
}, result => {
  this.key = result.key
})

$scope.$on('$destory', unsubscribe)

React-redux has a third argument which gives the consumer control over how the state, actions and (in angular terms) the $scope should be merged. This could perhaps offer an alternative, although it's a rather verbose alternative in and of itself.

p.s. I really wished that this allowed us to hook into the controllers $destroy lifecycle event somehow, but unfortunately that doesn't appear to be possible.

from ng-redux.

p.s. I really wished that this allowed us to hook into the controllers $destroy lifecycle event somehow, but unfortunately that doesn't appear to be possible.

I agree with that - with Angular pushing for controllerAs syntax, and the motto of "$scope is bad", seems weird that the moment we need to do anything with the destruction of a controller - we need to bring it back in.

The issue I seem to be running into, is something like this:

import {
  createSelector
}
from 'reselect';

let accountNumberSelector = n => n.session.get('accountNumber');

let theSelector = createSelector([accountNumberSelector], accountNumber => {

  return {

    accountNumber

  };
});
export default class AccountSelectController {
  constructor($scope, $ngRedux, accountSelectActions, $interval) {


    $ngRedux.connect($scope, this.mapStateToScope)


    $interval(() => {
      console.log(' i-scope', $scope, this)
    }, 5000);

    this.accountSelectActions = accountSelectActions;

  }


  select(form, accountNumber) {
    if (form.$invalid) {
      return false;
    }
    console.log('before dispatch');
    console.log('this', this);
    this.accountSelectActions.setAccountNumber(accountNumber + 'change');
    console.log('after dispatch');
    console.log('this', this);
  }

  mapStateToScope(state) {
    var x = {
      accountSelect: {
        ...theSelector(state)
      }
    };

    console.log('Trying to map to state',x);
    return x;
  };
}

In this case - even if accountNumber has an initial state - it's not getting reflected on initial load. if I have a template with {{{accountSelect.accountNumber}}} - nothing is showing up.

If I type in xyz, it will change to show xyzchange - which is expected.

In the case that I was able to get the initial state to reflect on the UI - the first update would get reflected, but then everything else would break after.

My submit events would stop firing, because $scope.accountSelect.select would no longer be available.

Not sure if it's something with the _.assign(target,slice) that is causing the issue in ng-redux, or something in the angular lifecycle with $scope - but it seems like one is clobbering the other.

The previous behavior, despite needing to explicitly handle the scope unbinding events myself - seemed to be far more predictable and understandable.

from ng-redux.

Just upgraded a coworkers sample app to use rc4 (pr) - and wasn't too bad of a process, and things are working as expected - think I need to dig into my other app to figure out what is going on.

Don't have any benchmark though - but it does seem like the performance is a little bit worse when comparing this to the previous version. I'm wondering if this is because no matter what - the bindToScope is getting hit all the time - where before with being able to do $ngRedux.connect(reselect,cb) - the CB would only get hit if that slice of state changed.

For example, in the TopPointsController - the scope is getting re-bound 3 times just during the initial load. Switching between data sources causes it to get hit 4 times.

With the previous version (PR here with the count logger), on the initial load - the TopPointsController binding happens twice, and then switching between the data sources - it's getting hit twice

from ng-redux.

Looking at it a little further, in the old version - we have some middleware/actions that get dispatched for an HTTP request.

While the HTTP request does update the application state - it's not updating a slice of state that we care about yet. So the state callbacks dont' always get triggered.

With RC4 - every action that gets dispatched, makes every bindToState get called again - even if we don't care about it in that particular component.

While in this particular demo app - could be some other areas for room for improvement with how immutable is being used, and how/where we are calculating some of the totals - even with that in mind, I think this could turn into an issue with larger applications.

from ng-redux.

I just realised something painful, in the context of the controllerAs discussions I was thinking of the syntax associated with a directive, similar to the counter example. This blinded me to the fact that you can of course(!) do the following:

<div ng-controller="AccountSelectController as x"></div>
<div ng-controller="AccountSelectController as y"></div>

This breaks if you're required to define the controllerAsKey as part of your selector, which (if it's not possible to derive the controllerAsKey when given the $scope) the current API does.. 😢

from ng-redux.

@elgerlambert ahh --- which is probably why in my other app, things were going weird - as I was using ui-router, and controllerAs for a controller there, and in the nhl-example, things were doing what was expected (although with other concerns), and that's because that is more directive/component based.

from ng-redux.

The current "bindToScope" is behind the same shallowEqual check as the callback was, so performance shouldn't be effected; both would be called just as frequently..

hmm, although.. since you're required to nest the result and wrap it in the controllerAsKey the "shallow"Equal may very well not pick up on the fact that the stateSlice hasn't changed.. would have to double check, but it sounds like more 😢

from ng-redux.

In the nh-example app, it's using directives with controllerAs, so I didn't need to wrap things with the controllerAs key like I was needing to do in my other application.

I can understand the desire for passing in $scope to automatically handle the unbinding, but the automatic binding to scope for the stateSlice and optionally actions feels like it's going a step too far and taking away a certain level of flexibility and control.

Although I think if it was simply documented that "Hey, should unbind this explicitly" - then it might not be that bad. Been talking with a few people here that are interested in the library, and they seem to agree with the "auto-unbinding events on scope destroy is nice, but making it a requirement and auto-binding to scope seems weird" - we are all pretty used to having to do

$scope.$on('$destroy',()=> { unbindStuff(); })

and needing to remember an extra line of code isn't worth the tradeoffs this seems to be causing.

from ng-redux.

Even before digging into the performance comment above, it's quite clear to me that having to wrap your selector results with the controllerAsKey is not an option. It either needs to be auto-detected when given a $scope (with the downside of being automagical) or you should simply be able to pass in this and unbind yourself, which would look something like the following:

let unsubscribe = $ngRedux.connect(state => ({
  todos: state.todos,
  users: state.users
}), this)

$scope.$on(‘$destroy’ unsubscribe)

Given a controllerAs: vm the above would provide you with {{vm.todos}} and {{vm.users}} in your template/view. $ngRedux.connect(reselect, this) would also work.

I'm disappointed having to concede the ability to ensure every subscription is also teared down, but in light of angular's controllerAs syntax and the recommendation to write components this way I'm afraid there might be no other option. At least not until angular makes it possible to hook into a component's lifecycle via this somehow.

from ng-redux.

Sounds like @wbuchwalter's feature to auto-detect the controllerAsKey is working in your nh-example app (when using directives with controllerAs), since that would explain why you don't have to wrap your results. But apparently that trick doesn't hold up when using controllerAs in ui-router like your other app. There's also the scenario to consider whereby the controllerAs is defined within the template. Although an attempt could be made, I doubt at this moment that a robust solution can be found to auto-detect the controllerAsKey.

Does the API suggested in my previous comment look good to you @e-schultz?
Btw, to avoid confusion, the snippet above is actually the same/shorthand for:

let unsubscribe = $ngRedux.connect(state => {
  return {
    todos: state.todos,
    users: state.users
  }
}, this)

$scope.$on(‘$destroy’ unsubscribe)

from ng-redux.

@elgerlambert it looks good, wondering how this would work with mapDispatchToScope, would it be

$ngConnect.connect(stateMap, dispatchMap /*optional, {} or func*/, this || $scope)

might need to do some checking of arguments length + type checking to get this to work for

• $ngConnect(stateMap,this)
• $ngConnect(stateMap,$scope)
• $ngConnect(stateMap,dispatchMapAsFunc, this || $scope)
• ... etc

I'll try and dig into the nhl-example a bit more later today and see if I can verify/pinpoint the cause of the stateMap being called more than I think it should be, but my hunch is that it's because the stateMap is now being called on all state changes, instead of a slice of a state change.

Although maybe I'm misunderstanding how the bindToScope is working - is every time the stateMap called, it's getting applied? or only if the shallow check detects a change?

so if I had

$ngRedux.connect(mapState)

let mapToState = (state) {
    state => {
      return {
        test: 'alwaysTheSame'
      }
    }

the bindToScope would run it on the first dispatch, but on every other dispatch - even though mapToState is always hit, the actual binding wouldn't be re-applied unless that return value changed?

If that is the case, then maybe my concern isn't the issue I thought it was.

from ng-redux.

Hey! I don't have much time today to look into that, will definitely do tomorrow, but I get your concerns.
@e-schultz regarding your last question, yes mapToState should be called on every dispatch but _.assign should only be called if the result from mapToState is different from the previous call.

from ng-redux.

Yeah that's what I was going for! Like you said, should be doable by checking the number of arguments and type checking dispatchMap if it's provided.

Your last remark is correct, the binding should only be re-applied if the value changes.

from ng-redux.

@wbuchwalter @elgerlambert thanks for the feedback and discussion.

Just trying to think now through the using in a service scenario.

if passing in $ngConnect(state=>{...state}, this) - I think this can still work depending on how you wrote the service, although I'm going to look into using .subscribe directly and see if that would be a better solution for that use case.

One thing I am thinking is is having an obvious extension point for providing your own bindToScope if the need is there.

Would something like below work?

// custom bindToState
class SomeController {
  constructor($ngRedux) {
    $ngConnect(bindState => toState => {
      this.x = toState.stuff;
    });
  }
}

// default behavior and use ngRedux
class SomeOtherController {
  constructor($ngRedux) {
    $ngConnect(state => {
      x: state.stuff
    }, this);
  }
}

although not sure how needed this would be, and just going with the simpler route first is probably fine. Ironing out the kinks with that first is probably more important than trying to provide a way to override.

from ng-redux.

I've discussed some things with @wbuchwalter in relation to your last comment. I'm gonna let him take it from here. Thanks for the discussion and valuable feedback @e-schultz!

from ng-redux.

Hey!
So yes, the new API is overly optimistic and has too much assumption on how it is going to be consumed.

From what have been said, seems like changing only the first parameter would suffice to handle all cases.

connect would become:
connect([target], [mapState], [mapDispatch])
with target being a plain obect or a function.

• If target is a plain object, the result of mapState and mapDispatch will be merged onto target (same behavior as currently with $scope). No more auto unbinding: It could be detected if target is an instance of $scope, and listen to $destroy in this case, but I think it's more important to keep a consistant behavior at the expense of verbosity.
• if target is a function, it will be passed the results of mapState and mapDispatch as parameters, the user is then free to do whatever he needs with that.

Also to clarify how things works:
mapState is called on every dispatch, and it's result is shallow compared to the previous result. If the result differs, target will receive the new state. This is really similar to selector of the first version, and should not have any additional performance impact.

Also, the reason I put target as first parameter, rather than the last is for consistancy. I understand that it makes more sense semantically to have it last, but as mapDispatch is optional, that would mean target sometimes being the second parameter, sometimes the third, I'm not a big fan of that.

Let me know what you think and if I forgot any use case that wouldn't be properly handled.

from ng-redux.

After playing a bit with the proposed API, I feel @elgerlambert is right about the order of parameters, having the target first feels way less natural when passing a function, since it's parameters will be derived from mapStateand mapDispatch, it's a kind of a mental strain.
I think a good solution, which solve both this issue and the consistency issue is to have:

connect(mapState, mapDispatch)(this)
//or
connect(mapState, mapDispatch)((selectedState, dispatch) => { //... })
//or
connect(mapState)(this)
//or
connect(mapState)(selectedState => this.x = selectedState)

This is the same API as react-redux, and it feels right in every case.

You can follow progress on the bc-1.0.0 branch.

from ng-redux.

@wbuchwalter thanks for taking the time to look into this, and letting us know where things are.

The proposed API looks good - and I like how there is consistency with react-redux

from ng-redux.

Released as part of 2.0.0, thanks!

from ng-redux.