chainyjs / chainy

The chainy core + autoloader plugin
https://github.com/chainyjs/chainy/wiki
Other
79 stars 4 forks source link

Figure out a way to remove the requirement of the chain's data as the first argument #17

Open balupton opened 10 years ago

balupton commented 10 years ago

Let's take the set action:

require('chainy').require('log')
    .addExtension('set', 'action', function(value, newValue){
        return newValue
    })
    .create()
    .set('blah')
    .log()  // blah

It needs to accept that value argument, as there is no way to turn it off.

Unfortunately, the new args taskOption added in v1.5.0 of Chainy, won't cover it, as:

Chainy.require('log')
    .addExtension('set', {
        type: 'action',
        taskOptions: {
            args: []
        }
    }, function(value, newValue){
        return newValue
    })
    .create()
    .set('blah')
    .log()  // null

Will output null as no arguments would be received.


This is something that I was considering about when releasing v1.5.0, however, v1.5.0 is still and improvement and offers cool stuff, despite this oversight.


What we probably need to do, is add a new special argument, like Chainy.injectArgumentsAsArguments, and Chainy.injectArgumentsAsArgument, then change the default action args to [Chainy.injectChainDataAsArgument, Chainy.injectArgumentsAsArguments].

This should maintain a lot of backwards compat, introduce some power, and accomplish the use case. With args: [] being changed to args: [Chainy.injectArgumentsAsArguments] instead.


The other option is to allow for args: false to be a special case, which means don't apply the action default arguments, but still apply the task options default arguments.


Another approach, which is similar to the args: false suggestion, which I initially coded before v1.5.0 but then dropped, was a new task extension type, and a new chain.task(method, opts). That will be the same as the action extension, but without the argument requirement. However, after implementation it proved to be an anti-pattern to a sort, as the user should have control over the argument insertion for the most part, and could cause confusion for implementors about which type they want. However, considering v1.5.0 is out with new args handling, this could be worth reconsideration as the anti-pattern may be subverted by the new args handling.


Need to consider this and weigh it up.

balupton commented 10 years ago

So I've added the special argument options, as well as added the ability to send user arguments to .action. It works well: https://github.com/chainyjs/chainy-core/commit/49954488fd7d3a1e010a0c5d8df965e24b45c4e9

Allowing:

require('chainy').require('log')
    .addExtension('set', {
        type: 'action',
        taskOptions: {
            args: [Chainy.injectArgumentsAsArguments]
        }
    }, function(newValue){
        return newValue
    })
    .create()
    .set('blah')
    .log()  // blah

This leaves the question of whether or not the task extension type makes sense. This is something I will need to evaluate in practice.

The task extension type would change would change the above to:

require('chainy').require('log')
    .addExtension('set', 'task', function(newValue){
        return newValue
    })
    .create()
    .set('blah')
    .log()  // blah

The problem I see is that this could be an anti-pattern against the chain waterfall flow dynamic that chainy does so well.


An alternative could be if you call an extension with an underscore at the start, it behaves as a task. This seems dangerous with the new work in that commit. But would allow more control to the user rather than the extension developer.


The task dynamic has some interesting abilities.

balupton commented 10 years ago

So I've been thinking about this a lot the past few months, and I think I've come to the following solution:

var Chainy = require('chainy'),
  $ = Chainy.$, // alias for injectChainData
  $$ = Chainy.$$ // alias for injectExpandedChainData

Chainy.create()
  .addExtension('set', 'task', function(value){return value})
  .addExtension('map', 'task', function(value, iterator){return value.map(iterator)})
  .set([5, 10])
  .map($, function(value){return value*2})
  .log($)  // [10, 20]
  .log($$)  // 10, 20

That much is definite, however I need to do up more examples of this, especially around the complexities of say redis database integration and usage.

Another question is (which the redis stuff will showcase), is how does one indicate whether or not we care about over-riding the chain's data. This could be solved via a few ways, e.g.

// Via a special argument
Chainy.create()
  .set(5)
  .set(10, Chainy.dontSetChainData)
  .log($)  // 5

// Via a method alias
Chainy.create()
  .set(5)
  ._set(10)
  .log($)  // 5

// Via a store plugin to reset the chains data when it is over-written, with last option
Chainy.create()
  .set(5).store()
  .set(10).restore()
  .log($)  // 5

// Via a store plugin to reset the chains data when it is over-written, with naming
Chainy.create()
  .set(5).store('five')
  .set(10).restore('five')
  .log($)  // 5

// Via a store plugin as the only way to update chain data
Chainy.create()
  .set(5).store()
  .set(10)
  .log($)  // 5

// Via store plugin as the only way to update chain data with $() helper, but can do storage with .store() helper, features names too
Chainy.create()
  .set(5).$('five')
  .set(10).store('ten')
  .log($)  // 5
  .restore('ten')
  .log($)  // 10
  .restore('five')
  .log($)  // 5

Remaining questions:

/cc @JedWatson keen to get your feedback on this comment's proposal, it could really change the way we write javascript code