Promises aren't quite right

[yamikuronue]

I had an issue where in my manifest, I had accidentally included a module I did not have in my npm_modules folder (I forgot to --save my dependency). Instead of rejecting, it called my callback with an invalid Server object. I put together some tests in a fork that show that the joi assertions just don't play nicely with Promises: https://github.com/yamikuronue/glue/commit/6f8b833997bb3a344941ae8390d9280d75bf7355

[WesTyler]

joi assertions just don't play nicely with Promises

Unfortunately this is true at the Joi level... I'll try to take a look at the Glue piece of the puzzle later today if no one else beats me to it.

[csrl]

@WesTyler did you ever get a chance to look at this?

[WesTyler]

Err... nope. Being totally honest, I forgot. I'll add it to my to-do list for this weekend though. Sorry about that.

[WesTyler]

Didn't find anything so far. I'll update here if I end up finding anything helpful when I get the chance to dig a bit deeper.

[csrl]

@yamikuronue the commit introduces quite a lot of whitespace issues in the linter.

Then it looks like you moved a bunch of tests into a 'describe', then duplicated them out and modified them to uses promises, but changed the test to no longer catch thrown exceptions.

I'm not sure what you are expecting, but the tests are invalid. Glue.compose throws/asserts if the provided parameters are bogus. This has nothing to do with promises.

[yamikuronue]

@csrl I refactored into a more organized structure after I made my new tests, but all I did was duplicate the error handling logic in the promise mode that was there in the callback mode. Some errors that are thrown when using callback mode don't end up as a rejection and I don't yet understand why. I ran into this in production code originally.

[csrl]

I see. This is resolved by moving the two assert statements to after the Promise is created.

diff --git a/lib/index.js b/lib/index.js
index 402ad47..ea33ce8 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -40,9 +40,6 @@ exports.compose = function (manifest /*, [options], [callback] */) {
     const options = arguments.length === 1 || typeof arguments[1] === 'function' ? {} : arguments[1];
     const callback = typeof arguments[arguments.length - 1] === 'function' ? arguments[arguments.length - 1] : null;

-    Joi.assert(options, internals.schema.options, 'Invalid options');
-    Joi.assert(manifest, internals.schema.manifest, 'Invalid manifest');
-
     // Return Promise if no callback provided

     if (!callback) {
@@ -59,6 +56,9 @@ exports.compose = function (manifest /*, [options], [callback] */) {
         });
     }

+    Joi.assert(options, internals.schema.options, 'Invalid options');
+    Joi.assert(manifest, internals.schema.manifest, 'Invalid manifest');
+
     // Create server

     const serverOpts = internals.parseServer(manifest.server || {}, options.relativeTo);

[csrl]

@SimonDegraeve as you implemented the original promise support in glue, do you have any comment on correctness for this?

[lock[bot]]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.