dustingetz / react-cursor

Immutable state for React.js
1.03k stars 50 forks source link

Support map(), filter(), find() and forEach() on cursors #32

Closed optilude closed 9 years ago

optilude commented 9 years ago

One of the things I miss from Cortex is the ability to use map(), filter(), find(), findIndex(), and forEach() on cursors over lists and keys(), values(), and forEach() on cursors over hashes. These should work like their native equivalents but their return values should be cursors.

For example, I want to pass the currently selected item in a list of hashes down to a child component. The currently selected item is identified by having an attribute selected set to true:

var selected = this.props.someList.find(a => a.selected.val());

With react-cursor, I (think I) have to write:

var _ = require('lodash');

var selectedIndex = _.findIndex(this.props.someList.value, a => a.selected);
var selected = this.props.someList.refine(selectedIndex)

We seem to have operations to mutate the cursor, but only value for accessing it.

dustingetz commented 9 years ago

Would you be interested in sending a PR? I will at least take map and filter, I will have to think about how I feel about the rest of them.

On Wednesday, January 21, 2015, Martin Aspeli notifications@github.com wrote:

One of the things I miss from Cortex is the ability to use map(), filter(), find(), findIndex(), and forEach() on cursors over lists and keys(), values(), and forEach() on cursors over hashes. These should work like their native equivalents but their return values should be cursors.

For example, I want to pass the currently selected item in a list of hashes down to a child component. The currently selected item is identified by having an attribute selected set to true:

var selected = this.props.someList.find(a => a.selected.val());

With react-cursor, I (think I) have to write:

var _ = require('lodash');

var selectedIndex = _.findIndex(this.props.someList.value, a => a.selected); var selected = this.props.someList.refine(selectedIndex)

We seem to have operations to mutate the cursor, but only value for accessing it.

— Reply to this email directly or view it on GitHub https://github.com/dustingetz/react-cursor/issues/32.

dustingetz commented 9 years ago

I thought about it. I think everything you asked for can be implemented as functions, doesn't need to be a method, so you could implement them in your application. I would take a PR with any functions you think are useful, though I would probably put them in a separate namespace or something. Are you interested in taking a whack or do you want us to do it?

On Wednesday, January 21, 2015, Dustin Getz dustin.getz@gmail.com wrote:

Would you be interested in sending a PR? I will at least take map and filter, I will have to think about how I fee about the rest of them.

On Wednesday, January 21, 2015, Martin Aspeli <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

One of the things I miss from Cortex is the ability to use map(), filter(), find(), findIndex(), and forEach() on cursors over lists and keys(), values(), and forEach() on cursors over hashes. These should work like their native equivalents but their return values should be cursors.

For example, I want to pass the currently selected item in a list of hashes down to a child component. The currently selected item is identified by having an attribute selected set to true:

var selected = this.props.someList.find(a => a.selected.val());

With react-cursor, I (think I) have to write:

var _ = require('lodash');

var selectedIndex = _.findIndex(this.props.someList.value, a => a.selected); var selected = this.props.someList.refine(selectedIndex)

We seem to have operations to mutate the cursor, but only value for accessing it.

— Reply to this email directly or view it on GitHub https://github.com/dustingetz/react-cursor/issues/32.

optilude commented 9 years ago

Hi,

I'm really happy to help, but I am ludicrously busy these days so it'd be great if someone else could take it forward. :)

I think it'd be better if these were methods on the cursor rather then separate functions under a different namespace. It feels inconsistent to have splice(), push(), unshift() and so on, which mirror the Array API, but not have map(), filter(), forEach() and so on. It just makes the code more awkward to read.

Will share anything I come up with of course!

optilude commented 9 years ago

Here is a (poorly tested) sketch of how this may be implemented for the array functions using underscore/lodash for assistance. I still think it'd be a lot better if done on the object itself and not in a separate set of functions though.

One particularly good reason for this is if you want to chain them, e.g. cursor.filter(...).map(...) rather than _.map(_.filter(cursor,...), ...).

I'm also not entirely sure whether it makes more sense for map() and filter() to return a native array or a new cursor. I think the most expected behaviour would likely be to return a cursor, but I don't know how to do that with the pattern below.

/*jshint globalstrict:true, devel:true, newcap:false */
/*global require, module, exports, document, window */
"use strict";

var _ = require('lodash');

module.exports = {

    /**
     * Like _.find(), but operates on a cursor of an array and visits a cursor
     * with fn(), returning a refined cursor.
     */
    find: function(cursor, fn) {
        var idx = _.findIndex(cursor.value, function(item, index) {
            return fn(cursor.refine(index), index, cursor);
        });

        if(idx < 0) {
            return undefined;
        }

        return cursor.refine(idx);
    },

    /**
     * Like _.findIndex(), but operates on a cursor of an array and visits a
     * cursor with fn().
     */
    findIndex: function(cursor, fn) {
        return _.findIndex(cursor.value, function(item, index) {
            return fn(cursor.refine(index), index, cursor);
        });
    },

    /**
     * Like _.map(), but operates on a cursor of an array and visits a cursor
     * with fn(). Returns a native array (not a cursor).
     */
    map: function(cursor, fn) {
        return _.map(cursor.value, function(item, index) {
            return fn(cursor.refine(index), index, cursor);
        });
    },

    /**
     * Like _.filter(), but operates on a cursor of an array and visits a cursor
     * with fn(). Returns a native array (not a cursor).
     */
    filter: function(cursor, fn) {
        var r = [];
        _.forEach(cursor.value, function(item, index) {
            var c = cursor.refine(index);
            if(fn(c, index, cursor)) {
                r.push(c);
            }
        });
        return r;
    },

    /**
     * Like _.forEach(), but operates on a cursor of an array and visits a cursor
     * with fn(). Returns cursor.
     */
    forEach: function(cursor, fn) {
        _.forEach(cursor.value, function(item, index) {
            return fn(cursor.refine(index), index, cursor);
        });
        return cursor;
    }

};
dustingetz commented 9 years ago

Quick update: We have not forgotten about this, we are still debating this internally.

dustingetz commented 9 years ago

Here is where my thought process is so far. I am deep diving specifically into map as I think it is the most interesting and the most useful.

in scala, map is type List[T] => List[U]. for cursors, there are two types i can think of that might work

Let's examine a use case and figure out the type

// generic type is: Cursor[List[T]] => List[U]
function map_cursor_list (cur, f) {
  return _.map(cur.value, function (v, i) {
    return f(cur.refine(i));
  }); 
}

// concrete type is: Cursor[List[String]] => List[DOM.Input]
map_cursor_list(form_cur, function (field_cur) {
  return <input type="text" value={field_cur.value} onChange={fieldCur.set} />;
});

So this is the former type. The type signature is not really map, its just sort-of-like-map-but-not-really.

If we changed it to be the latter type, we'd start having weird things like React.DOM.input inside of cursors. I can see a couple issues with that. 1) why are we storing DOM in application state? 2) what even happens if you store a complex reference type like that inside a cursor? As of today it would break the cursors, only "JSON" values are permitted to be in a cursor (though that could be addressed).

So how to do it without map?

reduce(range(form_cur.value.length), function (acc, i) {
  var field_cur = form_cur.refine(i);
  return acc.push(<input type="text" value={field_cur.value} onChange={fieldCur.set} />);
}, []);

That's not very nice to write in application code. Is there a better way?

It's definitely a useful function, I just don't think it's map and I have no idea what to call it.

dustingetz commented 9 years ago

Actually, this could be done in application code, I don't think this is so bad at all, and it is very explicit as to what it is doing, and it doesn't require that we name it.

return _.map(form_cur.value, function (v, i) {
    var field_cur = form_cur.refine(i);
    return <input type="text" value={field_cur.value} onChange={field_cur.set} />;
}); 
dustingetz commented 9 years ago

At this point we don't intend to make these changes. Does that prevent you from using react-cursor? If there is significant demand we would probably revisit this decision but so far @optilude is the only person to ask for it.