GraftJS / graft

full-stack javascript through microservices
http://graft.io
MIT License
227 stars 15 forks source link

graft().where #5

Closed mcollina closed 10 years ago

mcollina commented 10 years ago

I think we all agree we should have a graft().where() thing, heavily inspired by highland.

I would like to have it before nodeconf, let's see :).

AdrianRossouw commented 10 years ago

i was just going to add this as .filter: https://www.npmjs.org/package/through2-filter

and then add a couple of variations on that, mostly inspired by highland and http://lodash.com/docs

mcollina commented 10 years ago

Filter remove stuff from the pipeline, while where move it to the passed stream, it's an if-else.

AdrianRossouw commented 10 years ago

as per lodash/underscore : filter is for matches, reject is for the opposite.

where is a special case of filter. you could have a whereNot reject, i guess.

mcollina commented 10 years ago

I'm not against filter ;). It's just another stuff.

where change the course of the request, and send it to the passed stream. filter let some request pass.

where({ a: '42' }, stream).where(....).pipe(defaultHandler)

whereas

filter({ a : 42 }).pipe(anotherHandler)

I see filter and reject for validation-like components.

AdrianRossouw commented 10 years ago

right now, i see filter as :

graft.filter(function() { /* condition */ })

and for the moment I am seeing where as

graft.where({key: value});

which is to say just being :

 graft.where(function() {
    return (chunk['key'] == value) // && every other match
});

I did consider having it be branching based on predicates, but I suspect that keeping it shallow is going to make things more pluggable. I can see the default handler being appealing though.

I spent a long time playing with this mixin for underscore, and while I loved it, it was very difficult to get my team to use it successfully. That makes me kind of averse to designing the api too far down this line without understanding the repercussions properly first.

Here's the related highland.js issue : https://github.com/caolan/highland/issues/90

AdrianRossouw commented 10 years ago

btw, this doesn't preclude us from being able to do a branch/fork method/pipehandler/filterfn/mixin. i'd just like us to consider what it means for the rest of the API before baking it into everything.

adding it as a feature of the where() later is easy, stepping back from that decision means breaking everything written with it.

mcollina commented 10 years ago

I've added basic where support.

The cool thing we can even release it as its own NPM module.

This is the way for using it (straight from the unit tests):

  var first;
  var second;

  beforeEach(function() {
    first = where(graft());
    second = graft();
  });

  it('must route matching messages to the second instance', function(done) {
    first
      .where({ hello: 'world' }, second)
      .on('data', function() {
        done(new Error('not expected'));
      });

    second.on('data', function(req) {
      expect(req.msg).to.eql({ hello: 'world', name: 'matteo' });
      done();
    });

    first.write({ hello: 'world', name: 'matteo' });
  });
mcollina commented 10 years ago

Any opinons?

AdrianRossouw commented 10 years ago

I love the concept behind the wherer pattern. Something like that needs to be part of the basic api.

I think it just needs to be a bit cleaner. I don't like having to wrap things multiple times.

 where(another(some(graft())))

Maybe something like :

var graft = Graft()
  .pipe(where) // or
  .pipe(another.plugin) // or
  .plugin(some);

i'm also not convinced about the conditional split thing, because I still think it is too early to make a call like that for the base library.

So I think we should have the traditional filter/reject/where, and then you can have something like :

var where = Graft.where;
var graft = Graft().pipe(branch);

graft.branch(where({condition:'value'}), matched, unmatched);
mcollina commented 10 years ago

I love the concept behind the wherer pattern. Something like that needs to be part of the basic api.

yes. I'm thinking about moving it to the Graft object itself. It does not require much changes and should not add complexity (Graft is fairly simple now).

where(another(some(graft())))

I quite like it, coming from the level gang :). But I guess your concerns are right.

Currently it's just a neat trick to have a cleaner syntax, but this works too:

first
      .pipe(where())
      .where({ hello: 'world' }, second)
      .on('data', function() {
        done(new Error('not expected'));
      });

    second.on('data', function(req) {
      expect(req.msg).to.eql({ hello: 'world', name: 'matteo' });
      done();
    });

    first.write({ hello: 'world', name: 'matteo' });

(which is the same as yours, I can just remove the other one).

i'm also not convinced about the conditional split thing, because I still think it is too early to make a call like that for the base library.

This is needed as the starting point of the library, it's the basic use-case. We need to figure this out before nodeconf, which is kind-of close.

graft.branch(where({condition:'value'}), matched, unmatched);

This is really nice, however I don't like the unmatched component because then the result of branch() is null, e.g. branch is not a transform stream.

If we use:

graft.branch(where({ condition: 'value' }), matched).pipe(unmatched)

it's nice and pipeable.

So, how about having a generic function:

graft.branch(function(req) { return true; }, matched).pipe(unmatched)

And a shortcut for graft.where().

I would like to move these to the Graft instance itself, instead of a separate file.

Recap for my proposal

  1. graft.branch that accepts a function, but it is still a Transform.
  2. graft.where as a shortcut for graft.branch(where({ ..}), matched).pipe(unmatched)
  3. all of that in the main Graft object.
AdrianRossouw commented 10 years ago

Recap for my proposal

  1. graft.branch that accepts a function, but it is still a Transform. :+1:
  2. graft.where as a shortcut for graft.branch(where({ ..}), matched).pipe(unmatched) :heart:
  3. all of that in the main Graft object. :+1:

I need to add that we really do need to have the minimum higher order things in place too.

  1. filter
  2. map
  3. reduce

Where, in lodash and underscore, is just an alias to .filter(obj) instead of .filter(fn).

It just switches to these:

_.iteratee _.matches

mcollina commented 10 years ago

I think we can leave the high-order stuff to highland. At that point I hope graft sublimated away.