13

I'm working on an async script loader using bluebird and I'm struggling to pass an error up to where I can catch it.

When a file is loaded I'm calling my method named declare like this:

  declare("storage", [
    {"name": 'util', "src": '../src/util.js'}
  ], function (util) {
    var storage = {};
    //...stuff with util
    return storage;
  });

With declare being:

declare = function (name, dependency_list, callback) {
   var resolver;

   // digest promises returned for each module
   function digestDependencyArray(my_dependency_array) {
     var i, len, response_array;

     len = my_dependency_array.length;
     for (i = 0, response_array = []; i < len; i += 1) {
       response_array[i] = my_dependency_array[i];
     }

     return Promise.all(response_array);
   }

   // resolve request promise
   function resolveDependencyArray(my_fullfillment_array) {
      var return_value = callback.apply(window, my_fullfillment_array);

      // window.exports must be used when integrating commonjs modules
      if (!return_value) {
        return_value = window.exports;
      }

      resolver(return_value);
   }

   // START: set callback to (resolved) callback or new promise
   my_lib.callback_dict[name] = my_lib.callback_dict[name] ||
      new Promise(function (resolve) {
        resolver = resolve;
        if (dependency_list.length === 0) {
          return resolver(callback.apply(window));
        }

        return request(dependency_list)
          .then(digestDependencyArray)
          .then(resolveDependencyArray)
          // DON'T CATCH HERE...
          .catch(console.log);
      });
  };

This all works fine except I would like to not have the catch statement at this point, because the error handling should be done in a different module (the console.log is just a flag).

Question:
How would I propagate an error occuring in my declare method to a higher promise chain? I had hoped adding a catch handler on my declare calls would help, but this breaks the whole script - I assume because I'm returning the module from my declare call vs a valid promise response.

Thanks for any tips!

EDIT:
I'm calling declare from this:

 request([{"name": "foo", "src": "path/to/foo.js"}])
   .spread(foo) {

 })
 .catch(function (e) {
   console.log(e);
 })

request will load the file inside a promise which gets resolved when the file is loaded and run the file content as callback, which then calls the above declare method. Somehow my error is lost on the way (code here). Let's see if I can catch it somewhere...

Edit 2:
Changing to this inside declare:

function resolveDependencyArray(my_fullfillment_array) {
  var return_value = callback.apply(window, my_fullfillment_array);

  if (!return_value) {
    return_value = window.exports;
  }
  return return_value;
}

function handler() {
  if (dependency_list.length === 0) {
    Promise.resolve(callback.apply(window));
  } else {
    return request(dependency_list)
      .then(digestDependencyArray)
      .then(resolveDependencyArray)
      .catch(function (e) {
        reject(e);
      });
  }
}

clappjs.callback_dict[name] = clappjs.callback_dict[name] || handler();

While I get no errors, requesting multiple modules does not work because modules are returned undefined, so:

request(["foo", "bar", "baz"]).spread(function (foo, bar, baz) {
 console.log(foo);  // undefined
 console.log(bar);  // {} OK
 console.log(baz);  // undefined
});

versus a new Promise properly returning the file once it's loaded.

frequent
  • 27,643
  • 59
  • 181
  • 333
  • 1
    I guess you orginally wanted to do `new Promise(function(resolve, reject) { … .catch(reject); }` However, that's an antipattern :-/ – Bergi Aug 18 '14 at 21:42

2 Answers2

23

You need to rethrow the error!

.catch(function(e) {
  console.log(e); // calling it as a method, btw
  throw e;
})

You also might try to use tap, or return the promise that you have in the chain before adding .catch(console.log).


Also, you are using the manually-construct-promise antipattern, while you actually should never need to call the Promise constructor. Just use the promise that you already have! It seems that you want to do this:

my_lib.callback_dict[name] = my_lib.callback_dict[name] || (
  dependency_list.length === 0
  ? Promise.resolve()
  : request(dependency_list)
      .then(digestDependencyArray)
      .then(resolveDependencyArray) // don't call a global `resolver()`
                                    // just `return` the value!
);
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • 1
    Thanks for the anti-pattern example. One question regarding the throw: It seems I'm throwing, but I'm catching the error nowhere else. See my edit above. – frequent Aug 19 '14 at 07:41
  • Yeah, right, in the original code with the `Promise` constructor it's an unhandled rejection, as nothing happens with that promise. You'd need to call `reject(e)` instead of throwing. However, in the fixed code, the rejected promise is assigned to the `callback_dict`, from where it should propagate when that promise is `request()`ed. Or doesn't it work like that? – Bergi Aug 19 '14 at 09:04
  • the reject solved it for me. I'm not sure I understand the snippet, because I set a `new` Promise per file I'm loading. If the file has been loaded before the Promise is resolved and the return value is given back (at `clappjs.callback_dict[name]`). If it needs to be loaded and has no dependencies, I can just set the new promise to resolved right away. If it has dependencies, those need to be loaded first and included in the eventual resolve. So I don't see why I can do this without a "new" Promise. – frequent Aug 19 '14 at 21:06
  • Yeah, somewhere you typically will want to create a `new Promise`, usually for loading the file. But not in this place, where you either create a fulfilled promise when there are no dependencies (`Promise.resolve()`) or you just *compose* the promise that `request()` returns to a new one by calling `.then()` on it. Don't use the `Promise` constructor here! – Bergi Aug 20 '14 at 08:45
  • Tried again (see edit2 above), but I'm not getting good results, because some requested modules are returned `undefined`, while using my original code always returns the correct modules. I'm ok for now, will investigate if I have time and post a full example, too if you want to take another jab at it. Thanks so far! – frequent Aug 20 '14 at 11:08
  • In the case that you don't use the `new Promise` constructor, you should either omit (like in my second snippet) the catch or rethrow from it - there is no `reject()` you can call. – Bergi Aug 20 '14 at 11:33
1

Providing request([]) returns [] and callback happily accepts [], then declare() should simplify as follows :

declare = function(name, dependency_list, callback) {
    if(!my_lib.callback_dict[name]) {
        my_lib.callback_dict[name] = request(dependency_list).then(function(arr) {
            return Promise.all(arr);
        }).then(function(arr) {
            return callback.apply(window, arr) || window.exports;
        });
    }
    return my_lib.callback_dict[name];
};

Then make sure callback returns something falsy, eg null for condition(s) where window.exports should be used.

I may have made one or two involuntary assumptions, but I'm sure something close to the above should do the job.

Personally, I would handle any errors back in the call.

declare(name, dependency_list, callback).then(handleSucces).then(null, handleError);

By handling success and failure in separate .thens, any uncaught error thrown in handleSuccess will also be caught by handleError.

Roamer-1888
  • 19,138
  • 5
  • 33
  • 44