canjs / can-connect

Model layer utilities for every JavaScript framework! Assemble real-time, high performance, restful data connections.
https://canjs.com/doc/can-connect.html
MIT License
29 stars 16 forks source link

Unhandled rejection in can/map/map #482

Open chasenlehara opened 5 years ago

chasenlehara commented 5 years ago

The following line of code always creates a new Promise:

https://github.com/canjs/can-connect/blob/63f35ab12f670489a30dd655584e41472d0bd63a/can/map/map.js#L815

When the user calls .save() on an instance and doesn’t pass in an error handler, the Promise is still created, and if it errors then an unhandledrejection event is dispatched.

I believe that line should be changed to something like:

if (success !== undefined || error !== undefined) {
  promise.then(success, error);
}

This will make it so the user can still pass in success or error. If they only pass in success, then the unhandled rejection event will still be created. If they don’t pass in either, then it will be up to them to catch the error from the Promise that’s returned.


For context, I found this while upgrade can-connect-feathers to use QUnit 2. That package has tests that check errors in different scenarios. QUnit 2 will fail a test that causes unhandled rejections, so the line above was causing some tests to fail even when they were working correctly.

Here’s an example of a test that fails in QUnit 2:

        session.save()
        .then(function (res) {
            console.log('res', res);
        })
        .catch(function (err) {
            assert.equal(err.name, 'NotAuthenticated', "got back error message: "+err.name);
            done();
        });

This test should pass because it catches the error, but it fails because QUnit 2 listens for any unhandled rejections. There’s an unhandled rejection because the promise.then(success, error); line creates a new Promise that’s rejected, but there’s no error handler passed to .save().