The relevance of onResult / onError

[sebastian-zarzycki]

Hi, long time Flex/Parsley developer here. Delighted to have found out about Commangular.

However, after some tests, I'm kind of confused as to how it work. My main problem lies with asynchronous commands. As far as I could tell, currently the whole framework revolves around returning the value from execute() in command. Why? The thing is, that if you have synchronous operations, then you probably don't need to use any external framework, you can just call some functions one after the other. The great power of framework should be handing async Commands. Unless I'm mistaken, this is kind of flawed right now, because the framework assumes that whenever the promise is returned, the command is complete (?), it just applies some additional logic in the background to wait for the promise when commands are processed as sequence. Currently, I'm just receiving a promise in onResult and I have to unpack it manually, but this is done long after the command is deemed to be completed, so kills the point of doing so at all.

If I'm returning a promise, I would expect that it gets unpacked and resolved as a result parameter of onResult, and only after returning something from onResult the command would actually complete - because most of the logic will actually be there, and from there the result matters, not just a result of an async call (that's how Parsley works). When I intercept a Command with @After, I can see that it gets called whenever something is returned from execute and not when something is returned from onResult. So, either I'm using this wrong, or these limitations actually exist - and in current form, it just nullifies the sense of using the framework altogether. Am I doing something wrong here / will it be changed in near future? I'm happy to help/test.

[yukatan]

Hi,

I really don't understand exactly your problem. In Commangular all the commands are executed as async commands. I mean that the result of any command are wrapped in a promise. The next command in the sequence will be executed when the promise from the preceding command is resolved. The onResult is executed allways with the result resolved from the promise returned from the command. If this is not happened then it is a bug and it would be helpful if you paste the code you are executing.

When you are executing an interceptor like @After you are intercepting the execute method of the command and the onResult of the command is executed after the @After interceptor. This is a design decision that allow you to intercept the return from the execute method and change it, so the onResult will be invoked with a diferent result if you changed it in the interceptor code.

I will check this night the onResult with a promise, maybe I broke something not tested.

[sebastian-zarzycki]

The next command in the sequence will be executed when the promise from the preceding command is resolved.

I'm sorry, but I think this approach doesn't bring any merit to the framework existence. Most async commands do launch external calls, but what they do afterwards, is important. Therefore it makes little sense to consider command as "finished" as soon as promise is resolved. Take a look at Parsley's http://www.spicefactory.org/parsley/docs/3.0/manual/ (sections 7.1.5 and 7.1.6). If commands specify onResult and onFault, the command should be finished only AFTER one of these is processed (and possibly return a result). I would love for Commangular to treat Commands like this.

The onResult is executed allways with the result resolved from the promise returned from the command. If this is not happened then it is a bug and it would be helpful if you paste the code you are executing.

It isn't for me, must be a bug.

commangular.create('Command',['$log',function($log) {
    return {
        execute: ['$http',function($http) {
           return $http.get('https://localhost/sample.json');
        }],
        onResult : function(result) {
            $log.log(result);
        },
        onError : function(error) {
            $log.log(error);
        }
    }
}]);

onResult returns an unwrapped promise from a call, not the actual call result. This is in Angular 1.3.1.

When you are executing an interceptor like @After you are intercepting the execute method of the command and the onResult of the command is executed after the @After interceptor. This is a design decision that allow you to intercept the return from the execute method and change it, so the onResult will be invoked with a diferent result if you changed it in the interceptor code.

Same story. You assume that the command only cares about executing a call, and not doing anything else with the result. This is a very strong assumption and probably not right for most projects. Besides, isn't intercepting execute a job for @Around interceptor?

If you want to preserve this functionality, maybe @After should be just named @AfterExecute? I would imagine that @Before is called before command and @After is called after the whole command is processed, not just it's execute() part. Similarly @AfterThrow should be called, if there's an error within onResult.

Anyway, I think it would be very beneficial to treat commands as completed only after the onResult/onError execution (if they're specified, if not, just after execute()). Otherwise the onResult method is useless, because if command is considered completed at this point, the next command is fired, then the next command wouldn't receive any data that would be the result of onResult manipulation. This would mean that 95% of all the Command logic would have to land in interceptors, which, I think we both agree, isn't the right way to architect the application. onResult/onError should be the integral part of Command, not just afterthought handlers, working independently of the whole mechanism. I'm open for discussion and happy to help, I've designed a lot of application architecture with Parsley.

[sebastian-zarzycki]

Commangular with changes above would truly rise Angular to Parsley level.

[yukatan]

Ok the onResult was executed with the promise. you were right. I solved it in the last commit I think I lost some code on a git reverse :). Get the last code and it is solved, I added a test too.

About the command lifecycle I think I explain something wrong. The command is considered complete after the onResult method is executed. When the onResult method completes then the next command is called. The next command is executed with the result from the execute method (resolved if it is a promise) and not with the result of the onResult method (maybe this is what you want??).

About the @After maybe you are right and it is better to have @AfterExecution and @After I have to think about that.

Feel free to "commit request" anything and if it is ok it will be added

[sebastian-zarzycki]

Great, thanks for quick reaction! Will try the fix later.

The next command is executed with the result from the execute method (resolved if it is a promise) and not with the result of the onResult method (maybe this is what you want??).

Yes, pretty much this. I would like to return result from onResult, too. If I return something from onResult, then this should be used as result for next command, not the direct result of execute. If no onResult is specified, then obviously the value returned from execute should be used.

I will test later, if method when next method is firing, whether it's actually after onResult in previous method (I'm not entirely sure).

About the @After maybe you are right and it is better to have @AfterExecution and @After I have to think about that.

I would like that in one form or another. Because it's not exactly consistent right now - from the interceptor point of view, I'm unable to attach interception after the async command is actually completed (so after onResult is finished). @After feels like a natural name for this, and if someone needs to modify something between execute and onResult, then @AfterExecution would make perfect sense.

[sebastian-zarzycki]

For the time being, I'm not proficient enough with your code, to suggest fixes, you will probably do this far better. But I'm happy to help with all the testing.

[sebastian-zarzycki]

I was able to test recent build. Promises are indeed properly unwrapped and I can also confirm that next command fires after onResult of previous one.

It would be really cool to have @After working as described above, with possible addition of @AfterExecute. This, and returning result from onResult / catching throws from onResult too (for interceptors).

[yukatan]

Ok, perfect. About the @After, it is a broken change and needs some code rewriting, so I'll need more time, but it would be added on release 0.9.0 before the end of this year

[sebastian-zarzycki]

That's great, thanks! Will returning the result from onResult as lastResult and catching throws from onResult in @AfterThrowing also available in 0.9.0?

[yukatan]

Yes, I've to think about the implementation, but it has sense for me.

[sebastian-zarzycki]

Any new developments? We're far into 2015 now :)

[yukatan]

clone the branch commangular-fixes-5. The result from onResult is now the result from the command. I've added some test but check if it works.

Im working on change the @After behaviour

[sebastian-zarzycki]

Great news! I'll wait patiently for the release, but happy to test any beta builds.

[yukatan]

Check the last release 0.9.0 The @After interceptor now is executed after the onResult command method. There is a new @AfterExecution interceptor that is executed before the onResult.

The command result is the result from the onResult method if it exits.

Let me now if you find any problem or bug

[sebastian-zarzycki]

Works great - thanks!

[yukatan]

Perfect :) then I close the issue