Seneca actions and plugins do not support => functions.

[AdrianRossouw]

The way that we currently register callbacks with seneca is not forward compatible with ES6.

The problem is thus :

seneca.add('role:x,cmd:y', (args, done) => {
  let seneca = this; // this won't work any longer.
});

The only thing I can think of is to explicitly pass in the seneca instance as the first argument,
which would break the function signature and all existing seneca code :

seneca.add('role:x,cmd:y', (sen, args, done) => {
   // no more this;  
});

The one trick that we could have used was checking arity with function.length, but I think that
becomes much less reliable with default and ...rest parameters, and would lead to strange and hard to debug errors.

[AdrianRossouw]

this came up before in this issue, i was just making it more explicit:

https://github.com/rjrodger/seneca/issues/150#issuecomment-137552687

[AdrianRossouw]

a good article on these changes: arrow this

[boneskull]

👍 imo async callbacks should not have context.

[rjrodger]

seneca actions are not lambdas, so => is not appropriate

nor are they callbacks - they are action definitions

actions do indeed have context - the current seneca instance

all that said, we should probably allow @AdrianRossouw signature as an option in 0.8, given that it makes => "work" (for some value of work)

I am open to further discussion on this

/cc @mcollina @geek @mcdonnelldean

[AdrianRossouw]

This isn't a blocker, but just something we should also consider.
but, what about the action callbacks?

ie:

seneca.act('cmd=foo', (err, args) => {...} )

I think we're lucky there in that there is already a 'seneca' context available there,
but afaik we are also giving those callbacks 'this.seneca'.

[asaf]

Maybe this will help someone,

I've been suffering from process getting shutdown when some plugin performs act by itself, i.e:

this.add( {foo:'bar'}, function( args, done ) {
  this.act( { role: 'another', cmd: 'cmd' } (err, res) => {
     done("some error");
  }
}

Using arrow function causes the context of this.act to reference the plugins api_act, which mark errors as fatal see #112,

Converting to non lambda syntax fixed the whole thing.

Thanks.

[geek]

Closing, this is a non-issue. If you want to use arrow functions you can in places where it makes sense, it doesn't make sense in actions, where we are calling it with seneca as this.

'use strict'

const Seneca = require('seneca')
const seneca = Seneca()

seneca.add({ cmd: 'test' }, function (args, callback) {
  const fn = () => {
    callback(null, { name: this.name })
  }()
})

seneca.act({ cmd: 'test' }, function (err, result) {
  console.assert(result.name === seneca.name)
})

[boneskull]

I'm not so sure this should be so quickly dismissed. The failure of arrow functions to "work" is a side-effect of the design decision, which was to leverage the context object (this) in the "action" function.

If it can be shown that the current design is necessary to the implementation of actions, then there needn't be further discussion. Certainly arrow functions don't make sense if your function has a context. But the question is, "should it?"

It sucks to break an API, but if the API becomes problematic, what has to happen must happen. I don't suspect this issue will just go away. Maybe another issue should be created for this discussion, however.

(Related, Mocha leverages context where it shouldn't. I think originally, the context was seen as a way to simplify writing tests, since it's not always used. But the price is loss of developer control, and confusion about what this is at any given point, or what it should or should not be used for. At any rate, I think ES2015 exposed some of these issues; Seneca is not the only project having them.)

[rjrodger]

You know ... on reflection these are fair points.

Even though @geek is correct - these are not lambdas, it doesn't mean people won't do it anyway - we should honour the principle of least surprise http://www.catb.org/esr/writings/taoup/html/ch11s01.html

Re-opening to start discussion on a backwards compatible way to solve this issue - i.e. make possible the "natural" use of arrow functions.

The key issue is that the context of each action call needs to be track so that you can trace the flow of messages. calls to .act inside actions need to know where they came from - is there another way to do this? that would be a real solution, instead of relying on this

@geek is it as simple as looking at the function signature?

seneca.act( 'a:1', (err,done,seneca) => ... )

[mcdonnelldean]

We need some thinking on this functionality. All of the points above are valid but I would like to add as someone who is pulling stats out of Seneca that the end result of using context is still important.

I'm just a little worried that we are going to change from this:

seneca.act('role:foo', function (err, data) { this.act('role:bar') }

To this:

seneca.act('role:foo', (err, data, seneca) => { seneca.act('role:bar') }

I get that lambda's are hot stuff but these are contextual functions. I don't know how great I feel about changing the signature to leverage a language feature which is being shoehorned in. I'm not convinced
passing the context explicitly is any cleaner looking that using this.

Perhaps the answer is both? If you don't need context you can use a lambda. This won't be available and any further requests you make will break the call chain. If you use a full function you gain context. I like this idea as it makes the usage of function () {} explicit to needing context.

A lot of people may simply not be aware that you can track a 'transaction' through the system because of this functionality.

[geek]

I do think we should support changing the default behavior so that you can disable the binding seneca to the action. Thoughts on doing that, @rjrodger ?

[mcdonnelldean]

@geek But that's an all or nothing scenario.. Not fine grained enough for me.

[boneskull]

@rjrodger

The key issue is that the context of each action call needs to be track so that you can trace the flow of messages. calls to .act inside actions need to know where they came from - is there another way to do this? that would be a real solution, instead of relying on this

Given that something has to use Function.prototype.call or Function.prototype.apply to run the action function:

actionFunction.call(context, args, callback);

one could just:

actionFunction(context, args, callback);

Naming that first parameter seneca is a misnomer, but I don't know what it should be.

For backwards compatibility, you could naively do this:

if (actionFunction.length === 2) {
  actionFunction.call(context, args, callback);
} else {
  actionFunction(context, args, callback);
}

mind you I'm not familiar with the implementation.

[mcdonnelldean]

@boneskull yup thats the crux of the issues. I,d he happy to see a solution similar to the above, sanity checked of course 😊

[AdrianRossouw]

i've taken to passing the ctx as the first argument in my callbacks (ala python style), because it's consistent and it works. It also doesn't interfere with node's callback last style.

I've seen some situation where the function.length can't be relied upon any more due to spread and rest operators btw.

I think that if you are going to break the api, break it in a more future-safe direction at least.

[mcollina]

Sorry to do a lame question but this works:

'use strict'

const seneca = require('seneca')()

seneca.add({
  cmd: 'another'
}, (msg, cb) => {
  cb(null, {
    response: 'hello'
  })
})

seneca.add({
  cmd: 'call'
}, (msg, cb) => {
  msg.cmd = 'another'
  seneca.act(msg, cb)
})

seneca.act({ cmd: 'call' }, console.log)

In all examples in this thread, functions/lambda are defined in the same scope, so the seneca instance is available, even if this gets overwritten.

In plugins it's even better, because the plugin factory function is called with seneca as this.

However, I am obsessed with perf these days, and defining actions as anonymous function is bad because they will be hard to track.

[AdrianRossouw]

it works for seneca.act, but there are a few places the context is (mis?)used like with this.prior. isn't there also a this.ent ?

[mcollina]

I think we are going again to this.prior and this.ent :(. are you using these in all actions? How many of your actions include these?

[AdrianRossouw]

they occur hundreds of times in the last codebase i worked on. With all the different instances of that app, they occur thousands of times. Many of the seneca-extensions also use them.

[mcollina]

how many actions have you got? they are like 10%, 20%? That app is huge :).

The approach I am following is to understand the impact of not fixing it, given that the most common case seneca.act() is supported.

BTW, I do not like passing seneca as this.

Given that we are discussing this topic and we are speaking about breaking APIs, let me throw the other end of the spectrum: augmenting the incoming msg.

seneca.add('a:b', (req, cb) => {
  console.log(req.msg)
  req.act('b:c', cb)
})

[geek]

@mcollina I like the last example. I was thinking of a solution where we don't break the existing default behavior. Here is a proposal:

Add bind option. false disables calling actions with seneca bound to this. The default setting is true, which uses the existing behavior. Any other value is used as this when calling the actions for a seneca instance.

This will allow context to be bound, as well as your last example @mcollina, which doesn't have this set as seneca.

Thoughts?

[AdrianRossouw]

i haven't checked back in a while, but I think that @mcollina 's last version is the best solution here.
it solves every concern I had about the current implementation, and it's also cleaner and easier to document.

[geek]

relates to #288