Made callbacks optional and return results from function called.

[tlivings]

This is to enable a case where, for example, the underlying function has an optional callback and returns and/or returns and object such as the request object in the case of http core libraries.

[jasisk]

Hah.

[tlivings]

Oops - es6. Will fix.

[tlivings]

I've removed optional callbacks due to ambiguity on how to evaluate failure. Circuit breaker without a callback is better through a separate API (as @totherik mentioned in slack).

[shaunwarman]

Added erik via new reviewer feature just because he linked that article yesterday ;). LGTM. Probably time to update travis.yml

[tlivings]

He already was and probably took himself off ;) how do you think he learned about that feature :P or maybe it was coincidence.

[totherik]

Perhaps, I'm not seeing the use cases here but I see there may be gaps with how one can trip the breaker/report errors. Currently it supports only accepting functions that return error in the first argument position. I could see adding support for both EventEmitters that report using the 'error' event and promises (both of which could be expressed as specialized cases of the vanilla execute function).

Can you share a code snippet of how you want your client code to look so I can understand the use-case better?

[tlivings]

const circuit = Levee.createBreaker(Wreck.request);

const request = circuit.run(options, (error, response) => {
    ...
});

request.on('socket', ...);

And various other instrumentation I made add to request if I were using the library directly.

[totherik]

This looks really purpose-built for the Wreck pattern wherein even though it returns a request (EventEmitter) object, internally it normalizes emitted errors to be returned via the callback.

The same wouldn't hold true for vanilla http requests (which follows the same callback + return value pattern) and presumably if we're going to bake in support for this pattern we'd want levee to be able to handle those returned EventEmitters and their associated errors.

Are there other use-cases of return-and-callback that this fulfills where the returned value isn't an EventEmitter or doesn't emit an error that should be handled by levee?

[totherik]

Unfortunately, Wreck isn't even a specialized case of http due to the fact that if we treat the request the same as we would using http (as an EventEmitter) we would double-count errors (if I'm reading it correctly).

[tlivings]

Core http returns request object as well. I'd bet there are other APIs that return a value even though they take a callback.

[totherik]

Core http returns request object as well.

Yep, I mentioned that. It's still a pretty specialized use-case, so I'd like to understand other usage scenarios to weigh whether or not the additional complexity is worth it. AFAIK there is nothing preventing this from working today, so I'd like to see a demonstrable need to change the API.

[totherik]

Also, I don't see how proposed change could support http:

const circuit = Levee.createBreaker(Http.request);
const request = circuit.run(options, (response) => {
    // The `http` API doesn't support an "errorback" and there's nothing that
    // reconciles this situation so is the first arg sometimes an error and sometimes
    // a `response`?
    ...
});

request.on('error', ...); // Not handled by Levee, what do I do?

[tlivings]

It doesn't solve for APIs that don't take errback, but it keeps levee from erasing the underlying library's return type, which levee shouldn't do.

[totherik]

it keeps levee from erasing the underlying library's return type

Only if you choose to use a library that implements the "callback + return value" pattern and pass the lib function directly. This also should work just fine...

const circuit = Levee.createBreaker((options, callback) => {
    const request = Http.request(options, (response) => {
        // Response validation, status code checking, etc?
        callback(null, response);
    });
    request.once('error', callback);
    // do whatever with request...
    request.end();
});

circuit.run({/* options */}, (err, response) => {
    // ...
});

I'm not convinced that the "callback + return value" pattern is general enough that it makes sense to complicate the other use cases, especially since it won't work with http even though it looks and smells like it should.

[totherik]

Again, though, I'm all in for looking at how we can accommodate Event Emitters and Promises/async functions! 😄

[tlivings]

Yes, well that's why I removed optional callback from this change (title needs to be updated). All this change does is ensure that the result of the function being called by run is returned.

[totherik]

I feel like we're having two different conversations. 😄

I know what this change does and I've shared that even though you see it as a minor change I see it as a possible source of significant confusion because 1) it fills such a narrow use-case, 2) is able to be solved with the existing API, and 3) is eerily similar to other patterns yet different enough to not actually work with those pattern (e.g. http).

[tlivings]

I think we are talking about the same thing - but I think wrapping an API and making the decision to ignore what it may return, regardless of use case, may be too opionated.

[totherik]

It doesn't intentionally wrap anything therefore it necessarily can't establish an opinion. It accepts a function to invoke and upon invocation provides that function with context and a done callback. This is a pattern that pervasive throughout the JavaScript community. If you choose to short-circuit that by passing a library function directly, that's your choice (and it will correctly handle thousands of libraries that use the standard callback pattern).

Conversely, there is one (that I know of) example of using both a callback and return value. I would argue that supporting that one, narrow, use-case (to the exclusion of similar-but-not-quite use-cases) would most certainly be too opinionated.

One thing that I do have an opinion on is that, in my opinion, you must be trolling me. 😅

[tlivings]

I'm really not!

If you pass a function to run, and it doesn't return the result of that function, is that not a gap?

I agree that generally functions that take call backs do not return a value, but this seems to take the approach of enforcing that.

[shaunwarman]

[jasisk]

unsubscribe.

[tlivings]

‎(ﾉಥ益ಥ）ﾉ ┻━┻

[jasisk]

┬─┬◡ﾉ(° -°ﾉ)