bluejava / zousan

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

FIX: Issue #7 Catch errors in the constructor #11

Closed BITespresso closed 5 years ago

BITespresso commented 5 years ago

I'd like to propose these changes as a fix for https://github.com/bluejava/zousan/issues/7.

Regards, Michael

watnab commented 5 years ago

Was Zousan.suppressUncaughtRejectionError omitted in ddc72c3 ?

watnab commented 5 years ago

In https://github.com/bluejava/zousan/issues/7#issuecomment-415622638 , I did not consider the case below: var p = new Zousan(function(res,rej){ rej("Sync"); }); p.catch(console.log);

It may be that function(arg) { me.reject(arg); } should be function(arg) { soon( function() { me.reject(arg); } ); }.

watnab commented 5 years ago

It may be that Zousan.prototype.reject should be something like :

var that = this; // The keyword "this" should not be in the anonymous funciton below:
soon(function() {
    var clients = that.c;
    if(clients)
        for(var n=0, l=clients.length;n<l;n++)
            rejectClient(clients[n],reason);
    else
        if(!Zousan.suppressUncaughtRejectionError && global.console)
            global.console.log("You upset Zousan. Please catch rejections: ", reason,reason ? reason.stack : null)
});
BITespresso commented 5 years ago

In #7 (comment) , I did not consider the case below: var p = new Zousan(function(res,rej){ rej("Sync"); }); p.catch(console.log);

It may be that function(arg) { me.reject(arg); } should be function(arg) { soon( function() { me.reject(arg); } ); }.

I don't see the need for this change. What is actually wrong with a synchronous rejection in the Zousan constructor?

var p = new Zousan(function(res,rej){ rej("Sync"); }); p.catch(console.log);

Result:

Sync
Zousan {state: "fulfilled", v: undefined}

and

p;

Result:

Zousan {state: "rejected", v: "Sync"}
BITespresso commented 5 years ago

Was Zousan.suppressUncaughtRejectionError omitted in ddc72c3 ?

Yes, intentionally. I didn't see the point in catching an exception on the one hand and having the option not to catch it. But it's up to Glenn to decide if he likes it.

watnab commented 5 years ago

Yes, intentionally.

Thank you for your answer.

What is actually wrong with a synchronous rejection in the Zousan constructor?

I will try to answer. It's NOT definitely wrong. It may be a decision. https://github.com/bluejava/zousan#suppressuncaughtrejectionerror-flag

By default, Zousan will log a message to the console if a promise is rejected and that rejection is not "caught".


Native Promise of Chrome 71.0.3578.98 (in Windows 10, 64bit) : var p = new Promise(function(res,rej){ rej("Sync"); }); p.catch(console.log); results

Sync
Promise {<resolved>: undefined}

var p = new Promise(function(res,rej){ rej("Sync"); }); results

undefined
Uncaught (in promise) Sync //* hilighted as error.
  • ALSO, Firefox Quantum 64.0 reports uncaught exception: Sync .
  • HOWEVER, Microsoft Edge 42.17134.1.0 does NOT report "uncaught rejection".
  • It may be that Promise does NOT HAVE TO report "uncaught rejection".
  • It may be that original Zousan intends to report "uncaught rejection".
  • Original Zousan will report "uncaught rejection" with a synchronous rejection in constructor even if the following code catches the rejection actually. Because Zousan will judge synchronously whether the rejection is caught or not before the execution of the following code. It may be that the judgement should be "as soon as possible" not "synchronously".
  • I supposed the reason why you omitted the code may be the extra report for synchronous rejection. However, it turned out to be wrong.
  • To avoid the extra report, asynchronous judgement may be a way.
  • To avoid all reports, omitting code may be a way. However breaking compatibility could make other users of Zousan confused.
BITespresso commented 5 years ago

Thank you very much @watnab for your good explanation. Actually I did confuse the intention of this "uncaught" rejection and the "catching" of errors from the constructor. Now I got the point that the first one is for not using ".catch(...)" which has of course nothing to do with the try / catch in the constructor.

My fault. I'll have a look at it an your suggestions.

Thanks again!