KoryNunn / kgo

A stupidly easy flow control function.
MIT License
18 stars 3 forks source link

Feasibility of passing values for particular tasks to satisfy function argument order #6

Closed DamonOehlman closed 9 years ago

DamonOehlman commented 9 years ago

Hey Kory,

I definitely like kgo so nice work, and yes you were right. I was wondering how you would feel about tweaking kgo to allow values to be passed in for initial values. For example, I have the following code:

var capture = require('rtc-capture');
var attach = require('rtc-attach');
var kgo = require('kgo');

kgo
('constraints', function(done) {
  done(null, { video: true, audio: true })
})
('opts', function(done) {
  done(null, { plugins: [
    require('rtc-plugin-nicta-ios'),
    require('rtc-plugin-temasys')
  ]})
})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
('dom-insert', ['attach'], function(el) {
  document.body.appendChild(el);
})
.on('error', function(err) {
  console.error('captured error: ', err);
});

While in simple cases, I could easily use bind to defer function execution and pass through initial arguments to a function, though as things get complicated (i.e. I have to pass opts around) then that breaks down and I end up with the above code.

I was wondering if at some stage in the future you saw something like the following working:

var capture = require('rtc-capture');
var attach = require('rtc-attach');
var kgo = require('kgo');

kgo
('constraints', { video: true, audio: true })
('opts', { plugins: [
  require('rtc-plugin-nicta-ios'),
  require('rtc-plugin-temasys')
]})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
('dom-insert', ['attach'], function(el) {
  document.body.appendChild(el);
})
.on('error', function(err) {
  console.error('captured error: ', err);
});

Just thinking out loud really, but interested in your thoughts...

KoryNunn commented 9 years ago

I have been thinking about the best way to approach 'pre filled' parameters for a bit now, and I hadn't thought to do what you're doing, passing them into kgo as tasks.

Immediately, I really like it. Let me just CC in @MauriceButler and @ryanastuart to make sure it's reasonable before I put it in.

DamonOehlman commented 9 years ago

Cool - sounds good :)

MauriceButler commented 9 years ago

We are currently doing the function that callsback with data but the object would make it easier.

The problem with the new syntax will be the current parameter matching.

as in if you pass an array of strings into capture it looks the same as the call

( 'capture', ['constraints', 'opts'])

vs

( 'capture', ['constraints', 'opts'], function(){})

also line 25 saves me hours of debugging tests....

KoryNunn commented 9 years ago

possible soultion:

var capture = require('rtc-capture');
var attach = require('rtc-attach');
var kgo = require('kgo');

kgo
.set('constraints', { video: true, audio: true })
.set('opts', { plugins: [
  require('rtc-plugin-nicta-ios'),
  require('rtc-plugin-temasys')
]})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
('dom-insert', ['attach'], function(el) {
  document.body.appendChild(el);
})
.on('error', function(err) {
  console.error('captured error: ', err);
});

so by being explicit, you can have any data passed, eg:

.set( 'capture', ['constraints', 'opts']) //explicitly data

vs

( 'capture', ['constraints', 'opts'], function(){}) // explicitly dependencies
KoryNunn commented 9 years ago

Also, you could have a top-level set:

kgo
.set({
    constraints: { video: true, audio: true },
    opts: {
        plugins: [
            require('rtc-plugin-nicta-ios'),
            require('rtc-plugin-temasys')
        ]
    }
})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
('dom-insert', ['attach'], function(el) {
  document.body.appendChild(el);
})
.on('error', function(err) {
  console.error('captured error: ', err);
});
DamonOehlman commented 9 years ago

+1 for .set() or possibly .define()

KoryNunn commented 9 years ago

reasons for define vs set?

MauriceButler commented 9 years ago

Yep better I think.

I dont care what it is called.

gbenvenuti commented 9 years ago

How about without the function name, if first param is object then set.. i.e.

kgo({
    'constraints': {video: true, audio: true},
    'opts', { plugins: [
      require('rtc-plugin-nicta-ios'),
      require('rtc-plugin-temasys')
    ]}
})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
....
KoryNunn commented 9 years ago

I'd considered that @gbenvenuti, but wasn't sure it was worth trading readability for characters.

open to opinions.

DamonOehlman commented 9 years ago

@gbenvenuti I think that would work too.

@KoryNunn no real reason for define over set. Can these values be modified during the course of task execution, i.e. once a particular task is running could it modify these values? I don't know why but for some reason I consider things that are set to be updatable, whereas define feels like a single shot definition. Not fussed really either way...

KoryNunn commented 9 years ago

I'd think once a kgo is in flight, would be no good reason to modify the 'scope' other than in the usual, done(error, data) way, which makes me think @gbenvenuti's method is best, since you should know everything that is a 'default' immediately, and you shouldn't be allowed to modify the scope once the tasks begin.

gbenvenuti commented 9 years ago

i like the simplicity of kgo, nice clean one line per task, adding .set/.define messes up the flow

var myopt = {
    // my options opt1/opt2
};

kgo
(myopt)
('task1', ['opt1', 'opt2'], task1)
('task2', ['task1', 'opt1'], task2)
(['task2'], allDoneFunction)
DamonOehlman commented 9 years ago

Agreed - things shouldn't change once in flight. So +1 for @gbenvenuti's API proposal.

KoryNunn commented 9 years ago

Happy with this? this will be a minor version bump.

DamonOehlman commented 9 years ago

I just tried against the defaults branch and I seem to be getting an error:

Uncaught Error: No function provided for task number 0 (0__returnless) kgo.js:26kgoFn kgo.js:26newKgo kgo.js:42.. kgo.js:5s _prelude.js:1e _prelude.js:1(anonymous function) _prelude.js:1

Using this code, which maybe I've got a typo in but I can't see it if I do:

var capture = require('rtc-capture');
var attach = require('rtc-attach');
var kgo = require('kgo');

kgo({
  constraints: { video: true, audio: true },
  opts: {
    plugins: [
      require('rtc-plugin-nicta-ios'),
      require('rtc-plugin-temasys')
    ]
  }
})
('capture', ['constraints', 'opts'], capture)
('attach', ['capture', 'opts'], attach)
('dom-insert', ['attach'], function(el) {
  document.body.appendChild(el);
})
.on('error', function(err) {
  console.error('captured error: ', err);
});
KoryNunn commented 9 years ago

Looks like it's running against the current version. "0__returnless" is an auto-generated taskname, meaning it detected the defaults as a task.

Check it's using the defaults version, and maybe distil it down to a more simple usage if it still breaks.

DamonOehlman commented 9 years ago

Did a full cleanout of node modules and then reinstalled, and yes it is working now. I'm keen to proceed!

KoryNunn commented 9 years ago

v1.2 published.

DamonOehlman commented 9 years ago

Awesome thanks :)