bluejava / zousan

A Lightning Fast, Yet Very Small Promise A+ Compliant Implementation
MIT License
127 stars 13 forks source link

Fix issue #7 #13

Closed BITespresso closed 5 years ago

BITespresso commented 5 years ago

I'd like to propose there changes as fixes for issue #7

Please have a look at it.

watnab commented 5 years ago

I found no problem.

Though I could not explain the reason why there are 2 judgements.

reject: function(reason)
    if(clients) //*Synchronous judgement on whether the rejection will be caught or not.
        soon(function() {
            rejectClient(clients[n],reason);
    else
        soon(function() {
            if(!me.handled) {   //*Asynchronous judgement on whether the rejection will be caught or not.
                You upset Zousan

// set promise as "handled" to suppress warning for unhandled promises may be // set promise as "handled" to suppress warning for unhandled rejections.


BTW: If there is an API to withdraw the console output, Zousan may have the code below:

reject: function(reason)
    if(!me.handled) {
        me.output = console.log("You upset Zousan");

then: function(onF,onR)
    if(typeof onR === "function")
        this.handled = true;
        if (this.output) { this.output.withdraw(); delete this.output; }
BITespresso commented 5 years ago

I found no problem.

Thanks!

Though I could not explain the reason why there are 2 judgements.

There are two reasons, why I added a second judgement for the uncaught rejection:

// set promise as "handled" to suppress warning for unhandled promises may be // set promise as "handled" to suppress warning for unhandled rejections.

Thanks! Fixed.

BTW: If there is an API to withdraw the console output, Zousan may have the code below:

Very clever idea, but I'd rather leave this to the native implementation in Chrome. Firefox does not have this feature of a "dynamic" error message.

watnab commented 5 years ago

I had tested new Zousan(function (resolve, reject) { reject(123); }).then(console.log) with 25dbca7. However I could not reproduce that if(clients) would return true. However it returns true in the case of var z = new Zousan(); z.then(console.log); z.reject(123); . I think you found something, however I could not get it.


I misunderstood that if(clients) will return true when the rejection will be caught. However true only means that then(onF, onR) had been called while state was pending regardless of whether onR is a function or not. It keeps returning false in the case of new Zousan(function (resolve, reject) { reject(123); }).catch(console.log) even though the rejection will be caught. It may be that this.c should be set (like this.handled) in then even if the state is not pending.

then: function(onF,onR)
    if(this.state === STATE_PENDING)
    else
        if(typeof onR === "function")
        {
            this.handled = true;
            if (! this.c) this.c = [ /*empty*/ ];
            //The warning should be suppressed because `onR` will catch the rejection.
            //However no more clients should be added to the array because the state is not pending.
            //If the array already exists, it should not be replaced with a new array.
        }
reject: function(reason)
    soon(function() {
        if(me.c) execute clients
        else "You upset Zousan"
watnab commented 5 years ago

I found no problem. Though I could not explain the reason why there are 2 judgements.

I do not mean that the synchronous judgement must be removed. However I'm afraid something can slip through 2 judgements at different times.

bluejava commented 5 years ago

Thanks for all your input and the PR guys. After reading your comments and thinking about it more I agreed with your conclusions regarding an asynchronous rejection on Zousan.reject. I also deprecated the suppressUncaughtRejectionError flag as that functionality is better served by overriding the Zousan.warn property which I think is more clear and more powerful. I've added some tests to ensure the warning behaves as expected. (will publish to NPM tonight)