vvo / chainit

Turn an asynchronous JavaScript api into an asynchronous chainable JavaScript api
Other
12 stars 4 forks source link

Allowing aborting chain #3

Closed ysono closed 10 years ago

ysono commented 10 years ago

The aim of this change is to allow aborting chain.

The change is to read the output of the custom callback and if it's a certain value, e.g. false, the rest of the chain is abandoned.

ysono commented 10 years ago

The first commit is just refactoring.

I thought rather than the commonplace false, some obvious value like 'chainit abort' could be returned from the custom callback. However this would expose the chainit implementation to the user.

vvo commented 10 years ago

Hello can you explain when would we want to stop the chain? I don't understand if it's something that you need or if it's something that you might need.

You need to add tests for the changes you made:

And launch all the tests

About the refactoring: thank you for removing unused fnName. Aside from that I don't see any added value from renaming cb to callback and moving code to the bottom.

I will take the fnName removing but won't take other refactoring.

You should distinguish feature pull requests from refactoring pull requests, otherwise it will be harder for me to collaborate.

thank you.

ysono commented 10 years ago

I'd put it in the "need" category.

Using an example from a project on with we're both working, dragAndDrop.js is a chain that does not stop when something goes wrong in the middle of it. Currently, the get-around is to use nested ifs, as in close.js, which gets verbose quickly.

To use the ability to abort a chain, you can add this as the last parameter of each method except the last. Returned value of false stops the chain.

var handleResponse = function(err, result) { return err === null; };

Now, the caveat is you would still need nested ifs if you need to transfer data between the chained methods, as seen in the above close.js or a simpler example of isSelected.js. You could abstract it away with a helper, but I'm not convinced the reduction of verbosity in this case helps with clarity.

However, seeing that in the non-chained API a user would have the choice to stop executing a subsequent method, whereas in the chained API she does not, I think it's worth putting it in.

As for the refactoring, note taken. I shouldn't have put in the shifting to the bottom. I will go ahead and revert all that except fnName, so the final diff is clear. I appreciate the help, as I'm clearly new here at github.

ysono commented 10 years ago

I still need to add tests.

ysono commented 10 years ago

The user is still responsible for adding call to the post-chain function just before the return false in the custom callback.

myObj
    .method1(function(someError) {
        if(someError) {
            callOnFailure();
            return false;
        }
    })
    .method2()
    .method3(function() {
        callOnSuccess();
    });
vvo commented 10 years ago

@ysono I think what you did is similar to #5

As you said you will still need to use nested ifs to get detailed results on what is going on.

Also when you cancel the chain, it is cancelled for every other request, thus it means your test file is completely canceled.

Maybe what's doing in #5 will help you achieve something similar?

vvo commented 10 years ago

@ysono Think that we need a better way to do that, maybe a this.__stopChain(); or chainit.stop(chain). So that webdriverjs can use it internally and expose it has a custom command.