Proposal / Question: Make store state attributes (literally) immutable #19

Closed
ngmiller opened this Issue Jan 28, 2015 · 14 comments

Comments

Projects
None yet
3 participants
@ngmiller

First off, thanks for the work on flux-angular. I was able to port our project over to it quite quickly and working with the flux model has been awesome.

Since immutability is desired for data feeding into controllers, have you looked into an option like immutable.js? On top of the potential performance gains for large datasets (i.e. not having to clone on get()), one immediate benefit I can see is a reduction in store getter methods. For instance,

flux.createStore('myStore', {
    a: ...,
    b: ...,
    c: ...,

    (... action handlers, etc)

    exports: {
        getA: function () {...},
        getB: ...
        getC: ...
    }
});

could be reduced to:

flux.createStore('myStore', {
    state: {
        a: ...,
        b: ...,
        c: ...,
    },

   (... action handlers, etc)
});

Then, instead of myStore.getA(), we just do myStore.a and get the immutable ref back. I suppose exports would still be needed for anything more complex than a simple getter method.

Anyway, just a thought.

@sheerun

This comment has been minimized.

Show comment
Hide comment
@sheerun

sheerun Jan 29, 2015

Contributor

Stores are supposed to be mutable. Only exported data must be immutable.

Contributor

sheerun commented Jan 29, 2015

Stores are supposed to be mutable. Only exported data must be immutable.

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Jan 29, 2015

Owner

Hi there!

I think this immutability thing regarding flux can be a bit confusing. The reason React JS has immutability helpers is due to render validation. Instead of checking all values in arrays and objects passed as properties and/or on the state object of the component, it is possible to optimize that process by updating those values with the immutability helper. This allows the render validation of a component to just do a shallow check.

In my experience, when working with flux, it can feel a bit dramatic to force immutability because you get so much control of the flow of your state anyways. You have to work pretty hard to get into trouble with references, but again, that is just my experience :-)

Angular though, is its own machine with the two way databinding. When getting some state from the stores and putting it on the scope of the different controllers you do not want those arrays/objects updating without explicitly triggering a change event from the stores.

One strategy is to use immutability helpers when updating the state in the stores, as you suggest, thus making sure any already bound arrays/objects does not update. They will only then update when the "change" event is triggered. The problem with the immutability helper I believe to be its API. It is not a "method" API, but an object describing what you want to do. That is not something Angular developers are used to and can find it quite off putting I think. So to ease the transition into flux I chose to use cloning, which does the job without having to think about it.

It could be interesting to explore a strategy where we handle this "two way databinding prevention" using immutability when updating the state in the store though. Angular already spends a lot of time with its digest loop, it is not ideal to put more work in there.

How would you guys feel about adding an option that would turn off the cloning? You could try out the immutability thingy and if it feels right, we could even try to implement something that works like the immutability helper, but has a more Angularish API? :-)

Owner

christianalfoni commented Jan 29, 2015

Hi there!

I think this immutability thing regarding flux can be a bit confusing. The reason React JS has immutability helpers is due to render validation. Instead of checking all values in arrays and objects passed as properties and/or on the state object of the component, it is possible to optimize that process by updating those values with the immutability helper. This allows the render validation of a component to just do a shallow check.

In my experience, when working with flux, it can feel a bit dramatic to force immutability because you get so much control of the flow of your state anyways. You have to work pretty hard to get into trouble with references, but again, that is just my experience :-)

Angular though, is its own machine with the two way databinding. When getting some state from the stores and putting it on the scope of the different controllers you do not want those arrays/objects updating without explicitly triggering a change event from the stores.

One strategy is to use immutability helpers when updating the state in the stores, as you suggest, thus making sure any already bound arrays/objects does not update. They will only then update when the "change" event is triggered. The problem with the immutability helper I believe to be its API. It is not a "method" API, but an object describing what you want to do. That is not something Angular developers are used to and can find it quite off putting I think. So to ease the transition into flux I chose to use cloning, which does the job without having to think about it.

It could be interesting to explore a strategy where we handle this "two way databinding prevention" using immutability when updating the state in the store though. Angular already spends a lot of time with its digest loop, it is not ideal to put more work in there.

How would you guys feel about adding an option that would turn off the cloning? You could try out the immutability thingy and if it feels right, we could even try to implement something that works like the immutability helper, but has a more Angularish API? :-)

@ngmiller

This comment has been minimized.

Show comment
Hide comment
@ngmiller

ngmiller Jan 29, 2015

Hi guys, thanks for replying.

@sheerun I think we're on the same page here. My main motivation for this proposal was to reduce the workload on both the framework (not having to explicitly clone an entire object during a store get()) and the developer (not having to write a list of getters for each data point in the state). The state of the store would still be (and should be) mutable through action dispatching. My title is a bit misleading.

@christianalfoni We may be thinking of the same point from two different perspectives :). I was thinking more from the read / clone perspective. Ultimately, the move to an immutable-by-default data library (like immutable.js) would just make it easier to reason about the data integrity at different point in the flow (as flux-angular already does with cloning) but with less overhead on flux-angular's part. State reads would be a simple attribute lookup (as in myStore.a) that controllers could modify without fear of affecting other references to the store state, and state writes would potentially have to be handled in the same way that a language like Clojure handles them - through explicit and noticeable (i.e. set!) API calls. The developer would just have to know that the data coming from stores is in the immutable.js (or some other) ecosystem.

I hope I understood your response correctly and I think I agree. Please excuse me if I missed the point :) - I'd certainly be willing to experiment with some API changes

Hi guys, thanks for replying.

@sheerun I think we're on the same page here. My main motivation for this proposal was to reduce the workload on both the framework (not having to explicitly clone an entire object during a store get()) and the developer (not having to write a list of getters for each data point in the state). The state of the store would still be (and should be) mutable through action dispatching. My title is a bit misleading.

@christianalfoni We may be thinking of the same point from two different perspectives :). I was thinking more from the read / clone perspective. Ultimately, the move to an immutable-by-default data library (like immutable.js) would just make it easier to reason about the data integrity at different point in the flow (as flux-angular already does with cloning) but with less overhead on flux-angular's part. State reads would be a simple attribute lookup (as in myStore.a) that controllers could modify without fear of affecting other references to the store state, and state writes would potentially have to be handled in the same way that a language like Clojure handles them - through explicit and noticeable (i.e. set!) API calls. The developer would just have to know that the data coming from stores is in the immutable.js (or some other) ecosystem.

I hope I understood your response correctly and I think I agree. Please excuse me if I missed the point :) - I'd certainly be willing to experiment with some API changes

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Jan 30, 2015

Owner

Hi again :-)

Hm, okay, lets discuss some more so I am sure what you mean.

I was thinking of the React.addon.update function that gives you a:

// this being the store in this case
update(this, {
  $set: {
    someState: 'hello'
  }
});

So your store method would use something similar to this to update the state values of the store, making sure that whenever an object/array was changed the object itself and array itself would also change. This would ensure that already bound arrays/objects would not be updated by the two way databinding. You would have to emit a change event and grab that state again to actually update the scopes. But this does not of course prevent controllers from mutating existing objects/arrays and affecting the others... that would require discipline.

But as you said, I guess we looked at this from two different perspectives :-) I suppose you wanted to use the immutable-js API instead and use these value holders to avoid any mutation done after an object/array has been grabbed from the store?

That is a way to go, but I do not see how you could do this and still reference them directly? Well.. I see how you could reference them directly myStore.a, but you would probably not want to use an immutable value holder in your templates? Like myObject.get('b') in your template? Or maybe that is okay?

Please comment :-)

Owner

christianalfoni commented Jan 30, 2015

Hi again :-)

Hm, okay, lets discuss some more so I am sure what you mean.

I was thinking of the React.addon.update function that gives you a:

// this being the store in this case
update(this, {
  $set: {
    someState: 'hello'
  }
});

So your store method would use something similar to this to update the state values of the store, making sure that whenever an object/array was changed the object itself and array itself would also change. This would ensure that already bound arrays/objects would not be updated by the two way databinding. You would have to emit a change event and grab that state again to actually update the scopes. But this does not of course prevent controllers from mutating existing objects/arrays and affecting the others... that would require discipline.

But as you said, I guess we looked at this from two different perspectives :-) I suppose you wanted to use the immutable-js API instead and use these value holders to avoid any mutation done after an object/array has been grabbed from the store?

That is a way to go, but I do not see how you could do this and still reference them directly? Well.. I see how you could reference them directly myStore.a, but you would probably not want to use an immutable value holder in your templates? Like myObject.get('b') in your template? Or maybe that is okay?

Please comment :-)

@ngmiller

This comment has been minimized.

Show comment
Hide comment
@ngmiller

ngmiller Jan 30, 2015

Hey @christianalfoni - Yes, this is more along the lines of what I'm thinking.

But this does not of course prevent controllers from mutating existing objects/arrays and affecting the others... that would require discipline.

Only if the controllers are allowed to have references that can mutate the data within the store, right? If I understand correctly, flux-angular gets around this issue via cloning so that each controller can update its local copy without fear of affecting another controller's data. My hope is that using something like immutable.js as the underlying data layer would give flux-angular that behavior for free.

But, I also now realize that it would make data access and manipulation in a controller a little more verbose since immutable.js requires myObject.get(...) and myObject.set(...) to ensure the originating object is not affected and that a completely new value is returned. The persistent data structures within immutable.js do make that operation quite efficient though - assuming they are taking full advantage of structural sharing.

So the end result would have store reads be functionality equivalent to how they are now in flux-angular, just with a little more verbose operations in the controller, and store writes would need to completely overwrite the previous immutable value before emitting a change event. (There may be some use cases where that's not necessary - especially if the store has a reasonable level of granularity between its state components)

I could see some people being turned off by the extra object.get() and object.set() calls needed in a controller, so if this were to happen, it would probably have to be an opt-in feature through an angular config routine.

To be sure, I'm not advocating the manipulation of stores via a direct action done by a controller. Ensuring state is only updated at the store level via an action is definitely the right way to go.

I need to sign off so I hope I covered everything. Let me know if I'm going crazy :)

Hey @christianalfoni - Yes, this is more along the lines of what I'm thinking.

But this does not of course prevent controllers from mutating existing objects/arrays and affecting the others... that would require discipline.

Only if the controllers are allowed to have references that can mutate the data within the store, right? If I understand correctly, flux-angular gets around this issue via cloning so that each controller can update its local copy without fear of affecting another controller's data. My hope is that using something like immutable.js as the underlying data layer would give flux-angular that behavior for free.

But, I also now realize that it would make data access and manipulation in a controller a little more verbose since immutable.js requires myObject.get(...) and myObject.set(...) to ensure the originating object is not affected and that a completely new value is returned. The persistent data structures within immutable.js do make that operation quite efficient though - assuming they are taking full advantage of structural sharing.

So the end result would have store reads be functionality equivalent to how they are now in flux-angular, just with a little more verbose operations in the controller, and store writes would need to completely overwrite the previous immutable value before emitting a change event. (There may be some use cases where that's not necessary - especially if the store has a reasonable level of granularity between its state components)

I could see some people being turned off by the extra object.get() and object.set() calls needed in a controller, so if this were to happen, it would probably have to be an opt-in feature through an angular config routine.

To be sure, I'm not advocating the manipulation of stores via a direct action done by a controller. Ensuring state is only updated at the store level via an action is definitely the right way to go.

I need to sign off so I hope I covered everything. Let me know if I'm going crazy :)

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 1, 2015

Owner

Haha, you are definitely not going crazy :-) Let me create a branch of the current version. I will play around a bit with immutable.js first, just to see how we can integrate it with the current "getter methods". We might get away with something beautiful here :-)

I will keep you posted on the progress and then you can just clone the branch to check it out.

Thanks for the discussion!

Owner

christianalfoni commented Feb 1, 2015

Haha, you are definitely not going crazy :-) Let me create a branch of the current version. I will play around a bit with immutable.js first, just to see how we can integrate it with the current "getter methods". We might get away with something beautiful here :-)

I will keep you posted on the progress and then you can just clone the branch to check it out.

Thanks for the discussion!

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 1, 2015

Owner

I have been playing around with it for a bit. Currently there are some API challenges as any changes to the "state map" returns a new map. So an example:

.store('MyStore', function () {
  return {
    state: {
      foo: 'bar'
    },
    handlers: {
      changeFoo: 'changeFoo'
    },
    changeFoo: function () {
      this.state = this.state.set('foo', 'someNewValue'); // Kinda quirky
    }
  };
});

Could of course change this to:

    changeFoo: function () {
      this.set('foo', 'someNewValue');
      this.push('items', {id: 1}); 
    }

Creating an API like that could even auto-emit events. So the this.set('foo', 'someNewValue') could emit a 'foo' event. So a complete setup would be:

.store('MyStore', function () {
  return {
    state: {
      foo: 'bar'
    },
    handlers: {
      changeFoo: 'changeFoo'
    },
    changeFoo: function () {
      this.set('foo', 'bar2'); // Auto emits event
    }
  };
})
.controller('MyCtrl', function ($scope, MyStore, flux) {
  $scope.$listenTo(MyStore, 'foo', function () {
    $scope.foo = MyStore.get('foo'); // bar2
  });
  flux.dispatch('changeFoo');
});

The issue is nesting though, but Angular and EventEmitter2 already has syntax for that. So:

.store('MyStore', function () {
  return {
    state: {
      level1: {
        foo: 'bar',
        level2: {
          foo: 'bar'
        }
      }
    },
    handlers: {
      changeFoos: 'changeFoos'
    },
    changeFoos: function () {
      this.set('level1.foo', 'bar2'); // Auto emits event
      this.set('level1.level2.foo', 'bar2'); // Auto emits event
    }
  };
})
.controller('MyCtrl', function ($scope, MyStore, flux) {

  // We listen to all events emitted from level1 and down
  $scope.$listenTo(MyStore, 'level1.*', function () {
    $scope.foo = MyStore.get('level1.foo'); // bar2 (triggered twice)
  });

  // We listen to a specific change in state tree
  $scope.$listenTo(MyStore, 'level1.level2.foo', function () {
    $scope.foo2 = MyStore.get('level1.level2.foo'); // bar2
  });

  flux.dispatch('changeFoos');
});

I think this might be a way to go. It makes everything immutable with a simple API and you can still filter what events you listen to with less syntax. The store only needs to implement: this.set, this.push etc. that converts to immutable js and then triggers events. Okay... I just got a bit excited. What do you think? :-)

Owner

christianalfoni commented Feb 1, 2015

I have been playing around with it for a bit. Currently there are some API challenges as any changes to the "state map" returns a new map. So an example:

.store('MyStore', function () {
  return {
    state: {
      foo: 'bar'
    },
    handlers: {
      changeFoo: 'changeFoo'
    },
    changeFoo: function () {
      this.state = this.state.set('foo', 'someNewValue'); // Kinda quirky
    }
  };
});

Could of course change this to:

    changeFoo: function () {
      this.set('foo', 'someNewValue');
      this.push('items', {id: 1}); 
    }

Creating an API like that could even auto-emit events. So the this.set('foo', 'someNewValue') could emit a 'foo' event. So a complete setup would be:

.store('MyStore', function () {
  return {
    state: {
      foo: 'bar'
    },
    handlers: {
      changeFoo: 'changeFoo'
    },
    changeFoo: function () {
      this.set('foo', 'bar2'); // Auto emits event
    }
  };
})
.controller('MyCtrl', function ($scope, MyStore, flux) {
  $scope.$listenTo(MyStore, 'foo', function () {
    $scope.foo = MyStore.get('foo'); // bar2
  });
  flux.dispatch('changeFoo');
});

The issue is nesting though, but Angular and EventEmitter2 already has syntax for that. So:

.store('MyStore', function () {
  return {
    state: {
      level1: {
        foo: 'bar',
        level2: {
          foo: 'bar'
        }
      }
    },
    handlers: {
      changeFoos: 'changeFoos'
    },
    changeFoos: function () {
      this.set('level1.foo', 'bar2'); // Auto emits event
      this.set('level1.level2.foo', 'bar2'); // Auto emits event
    }
  };
})
.controller('MyCtrl', function ($scope, MyStore, flux) {

  // We listen to all events emitted from level1 and down
  $scope.$listenTo(MyStore, 'level1.*', function () {
    $scope.foo = MyStore.get('level1.foo'); // bar2 (triggered twice)
  });

  // We listen to a specific change in state tree
  $scope.$listenTo(MyStore, 'level1.level2.foo', function () {
    $scope.foo2 = MyStore.get('level1.level2.foo'); // bar2
  });

  flux.dispatch('changeFoos');
});

I think this might be a way to go. It makes everything immutable with a simple API and you can still filter what events you listen to with less syntax. The store only needs to implement: this.set, this.push etc. that converts to immutable js and then triggers events. Okay... I just got a bit excited. What do you think? :-)

@ngmiller

This comment has been minimized.

Show comment
Hide comment
@ngmiller

ngmiller Feb 1, 2015

@christianalfoni Yeah this looks good! I really like the ability to listen for changes on a specific state value. I've been thinking the auto-emit functionality would be a nice way to go as well. I actually wasted 10 or 15 minutes yesterday pulling my hair out just because I forgot to end my store action handler with this.emitChange() :)

If you push your branch up I'd be willing to give it a try!

ngmiller commented Feb 1, 2015

@christianalfoni Yeah this looks good! I really like the ability to listen for changes on a specific state value. I've been thinking the auto-emit functionality would be a nice way to go as well. I actually wasted 10 or 15 minutes yesterday pulling my hair out just because I forgot to end my store action handler with this.emitChange() :)

If you push your branch up I'd be willing to give it a try!

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 1, 2015

Owner

Cool! Just have a few more things to figure out and implement and I will push the branch :-)

Owner

christianalfoni commented Feb 1, 2015

Cool! Just have a few more things to figure out and implement and I will push the branch :-)

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 1, 2015

Owner

Okay, been looking a bit more into it. Immutable js is quite big (both in kb and API) and the "toJS" method is really slow compared to just doing normal cloning. I was hoping to just use: "toJS" on everything fetched from the stores so that it could be used as normal.

So a scenario where you have arrays with objects you have to use "toJS" to use that array with templates. But that is slower than cloning actually.

Immutable js shines where you have full control of accessing the values with javascript. Like if you could create LI elements using .map on an immutable List (Like with React JS). That is not possible with Angular since Angulars templates expects normal arrays etc.

So, hm, my conclusion this far is that as far as cloning vs immutability goes I do not really see it as a benefit with Angular? Please correct me if I am overlooking something here.

That said! Investigating this led us to the set/get API and automatic event emitting. I think that is worth getting in here nevertheless.

Owner

christianalfoni commented Feb 1, 2015

Okay, been looking a bit more into it. Immutable js is quite big (both in kb and API) and the "toJS" method is really slow compared to just doing normal cloning. I was hoping to just use: "toJS" on everything fetched from the stores so that it could be used as normal.

So a scenario where you have arrays with objects you have to use "toJS" to use that array with templates. But that is slower than cloning actually.

Immutable js shines where you have full control of accessing the values with javascript. Like if you could create LI elements using .map on an immutable List (Like with React JS). That is not possible with Angular since Angulars templates expects normal arrays etc.

So, hm, my conclusion this far is that as far as cloning vs immutability goes I do not really see it as a benefit with Angular? Please correct me if I am overlooking something here.

That said! Investigating this led us to the set/get API and automatic event emitting. I think that is worth getting in here nevertheless.

@ngmiller

This comment has been minimized.

Show comment
Hide comment
@ngmiller

ngmiller Feb 1, 2015

Size and dependency hell is definitely a concern with JS projects. With the template issue, dropping immutable.js and keeping the set/get API along with auto-emit is a plus for me :) Thanks for taking the time and energy!

ngmiller commented Feb 1, 2015

Size and dependency hell is definitely a concern with JS projects. With the template issue, dropping immutable.js and keeping the set/get API along with auto-emit is a plus for me :) Thanks for taking the time and energy!

@ngmiller

This comment has been minimized.

Show comment
Hide comment
@ngmiller

ngmiller Feb 1, 2015

Should auto-emit be opt-in via an angular provider? From a flux / "philosophical" perspective, it seems ok to just let stores emit after every action handler, but there may be a use case I'm not considering

ngmiller commented Feb 1, 2015

Should auto-emit be opt-in via an angular provider? From a flux / "philosophical" perspective, it seems ok to just let stores emit after every action handler, but there may be a use case I'm not considering

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 1, 2015

Owner

Have to play around with it a bit, but I am initially thinking that if you use the mutation methods this.set, this.push etc. it will emit the event for you. Though just pointing with: this.foo = "bar" will not emit an event... anyways, have to play some more with it :-)

Thanks again for the discussion! Great stuff!

Owner

christianalfoni commented Feb 1, 2015

Have to play around with it a bit, but I am initially thinking that if you use the mutation methods this.set, this.push etc. it will emit the event for you. Though just pointing with: this.foo = "bar" will not emit an event... anyways, have to play some more with it :-)

Thanks again for the discussion! Great stuff!

@christianalfoni

This comment has been minimized.

Show comment
Hide comment
@christianalfoni

christianalfoni Feb 21, 2015

Owner

Hi guys,

I have been working on a lib: https://github.com/christianalfoni/immutable-store. I have written an angular article which uses this lib to create a pretty cool way to use Angular. I thought I could start a discussion on that article and see what parts of that might be good to put into flux-angular.

Will send you an invite as soon as the article is out!

Owner

christianalfoni commented Feb 21, 2015

Hi guys,

I have been working on a lib: https://github.com/christianalfoni/immutable-store. I have written an angular article which uses this lib to create a pretty cool way to use Angular. I thought I could start a discussion on that article and see what parts of that might be good to put into flux-angular.

Will send you an invite as soon as the article is out!

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