mikehostetler / amplify

AmplifyJS
http://amplifyjs.com
GNU General Public License v2.0
1.44k stars 144 forks source link

Have the publish method return the answers of subscribers #94

Open louisameline opened 10 years ago

louisameline commented 10 years ago

Hello all,

the publish method currently returns true, or false if a subscriber returned false.

I have thought of a handier behavior where the function would return an array of all answers of subscribers, like :

amplify.subscribe( 'example', function(){ return 'hey' })
amplify.subscribe( 'example', function(){ return ['ho', 8] })
amplify.subscribe( 'example', function(){ return false })
responses = amplify.publish( 'example' })

==> responses == ['hey', ['ho', 8], false]

The goal is easier communication between two or more modules that only communicate via amplify messages, with fewer messages.

When you publish a message, you often expect something back in return (synchronously or not), and that's why I wanted to get a promise immediately returned. Consider how this code :

//In module A :
amplify.subscribe( 'loadRequest', function(requestId){
    $.ajax(
        ...,
        success: function(result){
            //publish a second message to communicate back to module B
            amplify.publish('loaded', requestId, result)
        }
    )
})

//in module B
// The requestId is used to identify the request when it comes back via a pubsub message,
// so we know it's the one we were waiting for
var requestId = random();
amplify.subscribe( 'loaded', function(reqId, result){
    if(reqId === requestId){
        alert 'Alright : ' + result;
    }
});
amplify.publish( 'loadRequest', requestId );

...could be written like this :

//In module A :
amplify.subscribe( 'loadRequest', function(){
    //return a promise
    return $.ajax(
        ...
    )
})

//in module B
var promise = amplify.publish( 'loadRequest' )[0];
promise.done(function( result ){
    alert 'Alright : ' + result;
});

Nicer, no ? Only 1 message and no more requestId.

To preserve the ability to stop the propagation of a message by returning false in a subscriber, I added a helper as first parameter to the subscriber handlers. It goes like this :

amplify.subscribe('example', function(helper, arg1, arg2, ...){
    //to stop propagation
    helper.stopPropagation();
})

I know it is a dead breaking change in the Amplify API, but I thought it was worth proposing anyway because I think it is very very nice. The code of my app looks way better and has shrunk. I won't go back :) But tell me if you see any con to this.

If you considered implementing this, you'd probably want to create a flag in the Amplify config for people who would like to keep the old behavior, for the sake of backward compatibility. I have not included that in my pull request however.

Thank you guys for the good work on Amplify anyway !

louisameline commented 10 years ago

I come back with 2 thoughts.

1) Another use case : the behavior is useful for a module to poll its subscribers about something and get their answers easily, no need for every module to send a dedicated message back. 2) A potential pitfall : in the case of a promise, if more than 2 modules are communicating, sometimes you will want to broadcast it in a message anyway rather than use the return feature. Depends on how you hooked up your modules.

dotnetchris commented 9 years ago

This PR should be rejected. Publishers should have no knowledge of subscribers.

louisameline commented 9 years ago

@dotnetchris The pubsub protocol only specifies unidirectional communication, yes. That does not mean that bidirectional communication is evil and that it "should" be avoided. Especially in the context of Amplify that can power webapps where components are not necessarily loosely coupled.

Bidirectional communication does not go against the unidirectional pattern, it's only an addition. The subscribers may or may not reply something, and the library may or may not expect a reply. You don't have to use it if it goes against your other patterns.

Anyway I don't expect this PR to be accepted since it has a breaking change. It's more food for thought. Thanks.