Immutable.js support

[tlrobinson]

Immutable.js has a much more familiar API for most JavaScript developers. Would replacing Mori with Immutable.js be feasible/desirable? Or perhaps allowing users to swap in their preferred immutable data collections?

[jmreidy]

I've been thinking a lot about this, and I'm not a huge fan of tying Fluxy to a single immutable data structure implementation - there's bound to be more coming out over time. While I prefer Mori's functional semantics, I don't think someone should be turned away from Fluxy if they prefer Immutable.

That said, the next step is to extract the dependency on Mori and instead code to an interface that will allow people to plug in any immutable implementation. I think there'd be out of the box support for Mori and Immutable.js, since those are currently the most common and could provide a nice reference for future interface adapters.

This change could dovetail nicely with the changes required for batching updates (#19).

[raulmatei]

I'm +1 this one. Even if I understand the benefits of immutable data structures, I'd love to see an approach with no dependency, plain JavaScript data structures.

[chikamichi]

:+1: as well :)

[svnlto]

yeap. :+1: happy to help out.

[jmreidy]

So I just took a pass at this functionality. The new PR above would allow for using ANY type of functional / immutable data structure library (Mori and Immutable.js are implemented in the PR).

Please let me know what you think. Most specifically - should a default collection type be specified? Can the code be cleaned up? If you think it can be improved, there's new tests for everything, so feel free to pull the branch down and work on cleaning up the code.

[raulmatei]

To me it looks great, I'll play around with it this evening. Thanks!

[raulmatei]

Using Immutable as proxy I'm not able to get all data from a store and transforming to JS by invoking MyStore.getAsJS(); with no key... it still works when using with Mori.

When initialazing the app I have some initial data that is passed to Fluxy.start method

Fluxy.start({
  UserStore: {
    'id': 1,
    'name': 'user.name',
    'email': '[email protected]',
    'type': 'demo'
  }
});

In a view I have to display all this data and I'm doing:

function getUserState() {
  return {
    user: UserStore.getAsJS()
  };
}

var ComponentName = React.createClass({
  displayName: 'ComponentName',

  getInitialState() {
    return getUserState();
  }
/* ... the rest of the component logic and rendering ... */

The store looks like:

var UserStore = Fluxy.createStore({
  name: 'UserStore',

  getInitialState: function () {
    return {
      user: null
    };
  }

/* ... the rest of the store logic */

Is it designed to work this way? Or I've just got to far with querying a store without a key?

Thanks, Raul.

[jmreidy]

@raulmatei Yep, right now you need to supply a key. We could change the behavior to return the state without a key. See [https://github.com/jmreidy/fluxy/blob/immutable_support/lib/Store.js#L140](line 140 of Store.js).

[jmreidy]

Oh and if @raulmatei or someone else wanted to put together an example with Immutable.JS, I'd be happy to add it.

[raulmatei]

@jmreidy you're right, the problem with the pull request #32 is actually a problem in the implementation, it throws errors when trying to bundle it with Browserify Error: Cannot find module 'mori' from '/dev/fluxy-test/node_modules/fluxy/lib/collections'. This is happening because Browserify needs to check if all require() calls can be resolved, so this will always fail with the above error if one of the libs is not installed:

var ProxyMap = {
  mori: function () {
    return require('./MoriProxy'); // this has `require('mori);`
  },
  immutable: function () {
    return require('./ImmutableProxy'); // this has `require('immutable');`
  }
};

Installing them is not a viable solution, you'll end up with both mori and Immutable in the bundle, and, probably, just one is actually needed.

One solution will be to pass the ProxyName exported object to Fluxy.setCollectionProxyType:

var ImmutableProxy = require('fluxy/lib/collections/ImmutableProxy');

Fluxy.setCollectionProxyType(ImmutableProxy);

this will also require changes in: https://github.com/jmreidy/fluxy/blob/immutable_support/lib/collections/Proxy.js:

/* ... */

module.exports = {
  test: function () {
    return _proxyType.displayName;
  },

  _setProxyType: function (type) {
    var interface;
    var Proxy = type;

    _proxy = undefined;
    _proxyType = type.displayName;

    if (Proxy) {
      _proxy = Proxy;
      interface = _proxy.Interface;
    }

    return interface;
  },

/* ... */

the above can be also cleaned, but it will make it easier to share implementation of the interface with different immutable libraries or even a vanilla implementation, by using modules:

var Immutable = require('immutable');

module.exports = {
  displayName: 'Immutable',
  Interface: Immutable,
  get: function (obj, strKey) { /* ... */ },
  getIn: function (obj, keys) { /* ... */ },
  updateIn: function (coll, keys, value) { /* ... */ },
  assoc: function (map, key, value) { /* ... */ },
  assocIn: function (map, keys, value) { /* ... */ },
  toJS:  function (value) { /* ... */ },
  fromJS: function (value) { /* ... */ },
  toVector: function (target) { /* ... */ },
  conj: function (target, value) { /* ... */ },
  count: function (coll) { /* ... */ },
  pop: function (coll) { /* ... */ },
  peek: function (coll) { /* ... */ },
  equals: function (a, b) { /* ... */ }
};

What do you guys think?

Raul.

[jmreidy]

This looks like a definite improvement to me.

[jmreidy]

OK all. @raulmatei updated the implementation and it's looking even better. Would anyone else care to code review or provide feedback? Thanks!

[wmertens]

I took a stab at converting the TodoMVC example to use Immutable to get the hang of Fluxy, see #37.

Things of note:

• store.$equals doesn't exist any more, nor any of the others. Since from the React elements you need to check state equality and you don't typically have the Fluxy object in scope I added a $equals method to the store which calls the proxied $.equals.
• It is very confusing that in Store.js, $ refers to the Proxy object but on the store object, $ refers to the immutability library.
• I think it's a waste of resources to put the entire state under a single key. I removed the todos key and instead put key/value pairs directly on the state object. I could be wrong.
• With immutable-js, some of the code becomes very simple (e.g. deleting completed is a simple filter) but it necessitates using store._updateState to set the new state (I suppose this follows from removing the todos root). However this doesn't make it notify the watchers.
• I handled the above by changing store.set() to work with keys set to null and also making get() return the entire state on null.
  • (BTW, shouldn't the notifications be batched into a requestAnimationFrame callback)?
• I don't understand why the initial state should be a js object that gets converted. It seems much more logical to simply provide the exact immutable initial state object that is desired.
• Example app uses React 0.11 and browserify. I switched to 0.12 and webpack with webpack-hot-loader which is a lot more fun to develop with.
• I changed the example app to only use immutable-js, perhaps the todostore should come in an immutable-js and a mori version. Ideally all the immutable library-specific code stays in the stores, right?
• Maybe the example shouldn't convert the state into a JS object and just pass it down the chain. Since the objects are deeply converted, you can just pass the result of get(key). Not sure if that uses less resources or more.
• The example should mixin PureRenderMixin.

[jmreidy]

@wmertens this is so awesome. Thanks so much for putting it together! Can we split the PR into changes to the library to support Immutable, and a separate set of changes that ADDS an immutable example instead of replacing the current mori based on?

[wmertens]

Happy that you like it, I'll see about putting this in two commits. I suppose I'll simply copy the example and leave the old one as-is.

What are your thoughts on the $equals that I needed to add and the slight change in semantics for get() and set() allowing (g/s)etting everything?

There's some other things I mentioned on the comment in the immutable-js issue, I would be interested in your thoughts.

[raulmatei]

Hi guys,

Is this going to be merged and pushed to NPM, or is still under review?

[jmreidy]

@raulmatei I'll look to get this in by the end of the week. Thanks for the bump on this.

[raulmatei]

Sounds good!

[jmreidy]

OK. Immutable support has been added to master. We need to overhaul the docs (e.g. Readme) before I can push as 0.5 to NPM though. Would anyone like to volunteer to take that on? :-D

[raulmatei]

I can update the docs for the immutable proxy setup, as it's something that I've been working on. I haven't reviewed yet the changes @wmertens made, since the Immutable was added, I'm using my own fork on 3 or 4 projects and it's working great... that's why I asked if it can be merged.

[jmreidy]

That would be awesome! Thanks @raulmatei