bluejava / zousan

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

Catch errors in the constructor #7

Closed kealthou closed 5 years ago

kealthou commented 7 years ago

It doesn't seem to be part of Promises/A+, but browsers, bluebird, q and so on's promises do this. I would like Zousan to catch errors and reject the promise, too.

paulocoghi commented 6 years ago

I'm going through exactly the same situation.

When using native Promises in NodeJS, inside a Zousan's promise, even using try/catch and catching the error, Zousan print errors to console using console.log.

But, with exactly the same code, but using a native promise, using a code with try/catch that generates an error, nothing is printed to console, since we are catching and solving the error.

watnab commented 6 years ago

TestCase:

new Zousan(function () { throw 123; }).catch(console.log), void 0; //Zousan
new Promise(function () { throw 123; }).catch(console.log), void 0; //Native Promise

Result:

watnab commented 6 years ago

TestCase:

Zousan.reject(234).catch(console.log), void 0; //Zousan
Promise.reject(234).catch(console.log), void 0; //Native Promise

Result:

watnab commented 6 years ago

TestCase:

Zousan(console.log); //Zousan
Promise(console.log); //Native Promise

Result:

watnab commented 6 years ago

The change of 'Zousan.reject' may avoid upsettings Zousan:

Zousan.reject = function(err) { var z = new Zousan(); z.reject(err); return z; }
->
Zousan.reject = function(err) { var z = new Zousan(); z.c=[]; z.reject(err); return z; }
watnab commented 6 years ago

The change of the constructor may catch errors.

function Zousan (func) {
    if(func) {
        var me = this;
        func(
            function(arg) { me.resolve(arg); },
            function(arg) { me.reject(arg); });
    }
}
->
function Zousan (func) {
    if (!(this instanceof Zousan)) throw new Error("Call with the new keyword!");
    if (arguments.length === 0) return; //Zousan allows this case.
    if (!(func && "call" in func)) throw new Error("The argument must be a function!");
    var me = this;
    try {
        func(
            function(arg) { me.resolve(arg); },
            function(arg) { me.reject(arg); });
    } catch (err) { soon( function() { me.reject(err); } ); }
}

The caught error should be rejected soon(but asynchronously).

watnab commented 6 years ago

"z.c=[]" will cause "fotget catching the rejection", too.

Zousan.reject = function(err) { var z = new Zousan(); z.c=[]; z.reject(err); return z; }
->
Zousan.reject = function(err) { var z = new Zousan(); soon(function() { z.reject(err); }); return z; }
bluejava commented 6 years ago

Thanks a ton @watnab for this thorough research and code suggestions! I don't think Zousan HAS to behave exactly like the browser native promises, but I also think much of this behavior is useful (in particular the auto-rejection on thrown constructors).

I will take a hard look at this this weekend and likely incorporate some of these suggestions. Excellent work @watnab - THANKS!

watnab commented 6 years ago

I understand that Zousan intends to be lightweight.

So I do NOT think the constructor must have heavy validations.

The intention of validations above is to detect whether the error comes from inside of func or not. However I do NOT think Zousan must detect whether inside or not. If the error comes from outside of func, it may only mean a misusage.

BITespresso commented 5 years ago

All tests passed but there seems to be something wrong with the test scripts:

  872 passing (13s)
Test Complete. All tests PASSED
> zousan-compliance-testing@1.2.1 testExtensions /home/travis/build/bluejava/zousan/test
> mocha extensions
/home/travis/build/bluejava/zousan/test/node_modules/mocha/bin/mocha:19
process.argv.slice(2).forEach(arg => {
                              ^^^
SyntaxError: missing ) after argument list
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:413:25)
    at Object.Module._extensions..js (module.js:448:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:471:10)
    at startup (node.js:117:18)
    at node.js:951:3

Could you have a look at it? Thanks!

Michael

bluejava commented 5 years ago

Hi Michael,

It appears the environment you are running those in doesn't support arrow functions.. ? It looks like Mocha requires that support.

BITespresso commented 5 years ago

Hi Glenn,

thanks for the reply. Actually I didn't do anything in my environment. All I did is create a pull request which triggered the test and build scripts that you have set up in Travis CI. So unfortunately there is nothing I can do from my side. :-(

Michael

bluejava commented 5 years ago

Oh, I see! My apologies. I'll take a look - thanks much for the heads up!

BITespresso commented 5 years ago

Great! All tests passed after a new commit.

Thanks Glenn!

BITespresso commented 5 years ago

@watnab could you please have a look at this new pull request https://github.com/bluejava/zousan/pull/12. I considered all cases that you have previously mentioned.

watnab commented 5 years ago

you have previously mentioned.

Sorry, I withdraw comments below:

https://github.com/bluejava/zousan/issues/7#issuecomment-415394963 2018-08-23T12:21:21Z

Zousan.reject = function(err) { var z = new Zousan(); z.c=[]; z.reject(err); return z; }

Because "z.c=[]" will suppress report and it will cause "fotget catching the rejection".


https://github.com/bluejava/zousan/issues/7#issuecomment-415622638 2018-08-24T01:27:03Z

catch (err) { soon( function() { me.reject(err); } ); } The caught error should be rejected soon(but asynchronously). Synchronous rejection will upset Zousan.

https://github.com/bluejava/zousan/issues/7#issuecomment-415627817 2018-08-24T02:00:39Z

Zousan.reject = function(err) { var z = new Zousan(); soon(function() { z.reject(err); }); return z; }

https://github.com/bluejava/zousan/pull/11#issuecomment-447696213 2018-12-17T01:26:55Z

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

Because "asynchronous judgement" may be more better way to avoid EXTRA report than "asynchronous rejection".

BITespresso commented 5 years ago

@watnab Thanks for your update!!

Zousan.reject = function(err) { var z = new Zousan(); z.c=[]; z.reject(err); return z; }

Because "z.c=[]" will suppress report and it will cause "fotget catching the rejection".

My impression is, that this will not do anything else than the current Zousan code already does, because after new Zousan the clients array is already empty (and just left uninitialized for performance reasons). Therefore, z.c=[] won't change the behavior.

#7 (comment) 2018-08-24T01:27:03Z

catch (err) { soon( function() { me.reject(err); } ); } The caught error should be rejected soon(but asynchronously). Synchronous rejection will upset Zousan.

I completely understand why you argue in favor of an asynchronous rejection. However, I had the feeling that especially in the case of Zousan.reject() a resolved Promise should be returned (i.e. not a 'soon' resolved promise). The same applies (but in a less strict way) for me in the case of an exception in the constructor. This is the reason why I decided against an asynchronous rejection, as you suggest.

#7 (comment) 2018-08-24T02:00:39Z

Zousan.reject = function(err) { var z = new Zousan(); soon(function() { z.reject(err); }); return z; }

In this case Zousan.reject() would return a Promise in the PENDING state rather than a REJECTED promise which does not feel correct to me.

#11 (comment) 2018-12-17T01:26:55Z

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

Because "asynchronous judgement" may be more better way to avoid EXTRA report than "asynchronous rejection".

My gut feeling is that the promise resolution and rejection should be handled synchronously. The only reference I can currently give is the implementation in these common promise polyfills for old browsers, which both call the resolver function synchronously:

In summary I had the impression that it would be better to explicitly suppress a warning in cases where it does not make much sense to me. Those being Zousan.reject() and an exception in the Zousan resolver function.

Of course you might just leave the warning as it is in those cases as well. That's a personal option I'd rather leave for Glenn to decide.

BTW: The most intelligent way to handle these warnings is done by Chrome. The warning actually is a dynamic message on the console which disappears once you added a .catch to a previously uncaught promise. But this is beyond reach for custom code.

watnab commented 5 years ago

z.c=[] won't change the behavior.

With the code z.c=[];, I intended to suppress extra report. However it is a wrong way because it will suppress also reports which can make sense. So I withdraw the comment explicitly.


Zousan v2.3.3 supposes that the rejection will be caught when the Promise has the menber c.

function Zousan(func)
    //this.c = [];  //*commentouted.

reject: function(reason)
    var clients = this.c;
    if(clients) //*judge whether the rejection will be caught or not.
    else
        if(!Zousan.suppressUncaughtRejectionError && global.console)
            global.console.log("You upset Zousan. Please catch rejections: ", ...
watnab commented 5 years ago

Sorry, I withdraw comments below:

I intended to withdraw not only the comment "2018-08-23T12:21:21Z" (suppress report) but also comments "2018-08-24T01:27:03Z", "2018-08-24T02:00:39Z" and "2018-12-17T01:26:55Z" (asynchronous rejection).


Because "asynchronous judgement" may be more better way to avoid EXTRA report than "asynchronous rejection".

Now I think "asynchronous judgement" may be more better way.


especially in the case of Zousan.reject()

A user of Zousan may remember to catch the rejection when he calls Zousan.reject() directly. However he may forget it when he gets a Promise with a function FOO of a library BAR and the auther of BAR changes FOO to return rejection with Zousan.reject() in some cases.


https://github.com/taylorhakes/promise-polyfill/blob/master/src/index.js

function finale(self) {
    Promise._immediateFn(function() { //*asynchronous
      if (!self._handled) { //*judgement
        Promise._unhandledRejectionFn(self._value); //*report

The code above may mean "asynchronous judgement".

BITespresso commented 5 years ago

@watnab Thanks for pointing me in the right direction! Indeed the asynchronous judgement was the solution that should address the issues.

Please have a look at my latest proposal: https://github.com/bluejava/zousan/pull/13

bluejava commented 5 years ago

Hey guys - work is still in progress here. After more thought, I agree with your assessment that the better option is to delay the rejection rather than set the c array. I merged BITespresso's PR (Thanks Michael!) but am making a couple small changes to it. (and adding a warning test section)

Do keep in mind that the uncaught rejection warning is a "best effort" - I don't think its possible to display exactly 1 warning for each uncaught rejection (which is the ideal).

I only get tiny pockets of time to work on this - but it IS very important to me, as Zousan is a core part of every project I am working on these days. Thanks for helping make it better guys!

BITespresso commented 5 years ago

Great news! Thanks for letting us know.

bluejava commented 5 years ago

This work was committed a couple months ago - closing this issue.

Thanks again for your contributions!