Add support for ES6 style async boot scripts (2.x) by lehni · Pull Request #252 · strongloop/loopback-boot

lehni · 2017-07-24T02:17:48Z

Description

This PR allows boot scripts to be defined in the ES6 async fashion, without the need for passing callbacks:

module.exports = async function(app) {
   ...
};

Related issues

None

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-07-24T02:17:49Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2017-07-24T02:17:55Z

Can one of the admins verify this patch?

slnode · 2017-07-24T02:17:55Z

Can one of the admins verify this patch?

slnode · 2017-07-24T02:17:55Z

Can one of the admins verify this patch?

bajtos · 2017-07-26T06:30:07Z

@slnode ok to test

bajtos

Hello @lehni, thank you for the pull request.

First of all, please add a test to verify your implementation.

Secondly, we usually try to support any function returning a promise, not only async functions. The trick is to check whether the function call returned an object containing a then method.

var cb = function(err) {
  debug('Async function finished %s', f.path);
  done(err);
};

debug('Starting function %s', f.path);
var result = f.func(app, cb);
if (result && typeof result.then === 'function') {
  result.then(ok => cb(), cb);
  return;
} else if (f.func.length < 2) {
  debug('Sync function finished %s', f.path);
  done();
}

bajtos · 2017-07-26T06:35:43Z

lib/executor.js

-      });
+      };
+      if (isAsync) {
+        f.func(app).then(cb.bind(null, null)).catch(cb);


If cb(null) throws an error, then .catch() will invoke cb again with this error - that's not correct IMO. You should pass the error-handling-callback as the second argument of .then():

f.func(app).then(cb.bind(null, null), cb);

bajtos · 2017-07-26T06:45:39Z

This is a great feature, I am looking forward to get it added! 👍

lehni · 2017-07-27T12:38:37Z

@bajtos I will work on this improvements and unit test for them. In the meantime, can you tell me which branch I should work on? Since there are two active branches (2.x and master), and I am still using 2.x for LB 3 (master doesn't seem to load mixins correctly, I meant to create a seperate issue for this), I guess I'll work on 2.x first and then port to master when it landed?

bajtos · 2017-07-27T14:57:01Z

I guess I'll work on 2.x first and then port to master when it landed?

Yes please, this works best for me.

lehni · 2017-07-28T02:29:31Z

@bajtos I've pushed a new version based on your input, along with some unit tests. PTAL

lehni · 2017-07-28T12:08:04Z

Two more things:

I needed to use an anonymous function instead of the =>, because eslint was complaining about it. I tried to update eslint to a version that supports it, but that triggered a whole row of other linting issues, so I stuck with the function for now
The tests reference the Promise object. I am not sure if that's fine? If not, then I can keep it all to thenable objects.

bajtos

I needed to use an anonymous function instead of the =>, because eslint was complaining about it. I tried to update eslint to a version that supports it, but that triggered a whole row of other linting issues, so I stuck with the function for now

We are trying to keep support for Node.js 0.x versions in the 2.x branch for backwards compatibility, ES6 constructs cannot be used here. This applies to most 2.x branches in loopback projects (strong-remoting, loopback-datasource-juggler, etc.). See strongloop/loopback#2807 (comment), you may want to read through the discussion that lead us to the current approach.

The tests reference the Promise object. I am not sure if that's fine? If not, then I can keep it all to thenable objects.

Sure, that's fine.

bajtos · 2017-07-28T12:31:27Z

test/fixtures/simple-app/boot/promise.reject

+// License text available at https://opensource.org/licenses/MIT
+
+module.exports = function(app) {
+  return Promise.reject('Promise.reject()')


Our practice is to reject Promises with Error instances.

return Promise.reject(new Error('Promise.reject()'));

Also please add .js extension to the filename. I am surprised your new tests are passing despite this problem?

I am using a bit of a workaround with scriptExtensions set to '.reject' for this test, so the failing boot script does not get loaded in all other tests, but I can then explicitly request it for that single test and produce the rejection. It's not super clean but I couldn't think of another way to achieve this goal without creating another full app (similar to the simple-app folder)... If I add .js, then it would be loaded in all boot script tests, and fail each time.

I hope it's fine?

Actually, I think I have a better idea for how to control this. I'll do the same to test for sync exceptions also.

bajtos · 2017-07-28T12:35:07Z

lib/executor.js

+      result = f.func(app, cb);
+    } catch (err) {
+      debug('Sync function failed %s', f.path, err);
+      error = err;


AFAICT, error is not used. Can we remove it please?

Also when the function exported by a boot script fails, the error should be reported back/up to the caller of boot(app). I think you should call return done(err) here.

It was late last night... My bad. Will address it now.

bajtos · 2017-07-28T12:45:15Z

@lehni the CI is failing on Node.js 0.10, see https://travis-ci.org/strongloop/loopback-boot/jobs/258394078. It may be caused by the fact that Node.js 0.10 does not have a built-in Promise object, in which case you can add bluebird as a dev-dependency and use bluebird Promise instead of the built-in one in the newly added boot scripts. Also because your new code is silently swallowing errors thrown by boot scripts, the CI output is not making the problem obvious.

lehni · 2017-07-28T14:34:45Z

@bajtos oops, yeah that's why the errors didn't make sense to me probably. I will look into both issues right now. Stil learning :)

lehni · 2017-07-28T15:45:21Z

@bajtos I think this is it. I responded to all your feedback and added a test for errors thrown from boot scripts. And it looks like it's passing all tests!

bajtos

Almost there :) I'll fix cb && cb() myself to speed the process up.

bajtos · 2017-07-31T13:44:14Z

lib/executor.js

+    try {
+      var result = f.func(app, cb);
+      if (result && typeof result.then === 'function') {
+        result.then(function() { cb && cb(); }, cb);


cb is always defined, the check cb && is redundant.

bajtos · 2017-07-31T13:45:31Z

test/executor.test.js

+  describe('with boot script returning a rejected promise', function() {
+    before(function() {
+      // Tell simple-app/boot/reject.js to return a rejected promise
+      process.rejectPromise = true;


It would be probably simpler to create a different application (a set of instructions) for each of these error cases. Having said that, your approach with process flags is ok too.

bajtos · 2017-07-31T15:33:41Z

Landed, thank you for the contribution!

bajtos · 2017-07-31T15:34:22Z

Released as loopback-boot@2.26.0. Enjoy 🎉

lehni · 2017-07-31T15:38:14Z

Very cool! That went faster than I'd imagined. Thanks!

lehni · 2017-07-31T16:58:23Z

Shall I port this to #253 now?

slnode · 2017-07-31T16:59:03Z

Can one of the admins verify this patch?

lehni · 2017-08-01T03:24:24Z

@bajtos the reason for cb && cb() was in this comment above:

    var cb = function(err) {
      done(err);
      // Make sure done() isn't called twice, e.g. if a script returns a
      // thenable object and also calls the passed callback.
      cb = null;
    };

I believe it's still valid?

bajtos · 2017-08-03T08:39:04Z

the reason for cb && cb() was in this comment above:
(...)
I believe it's still valid?

@lehni Ouch! You are right. I think a better solution is to set cb to a no-op function instead of null. Would you mind contributing this change in a new pull request? It would be great to add a unit-test with a script returning both a thenable object and calling the callback.

Shall I port this to #253 now?

Yes please 👍

lehni · 2017-08-16T14:02:01Z

@bajtos I will wait for #260 before resolving #253, and will then roll both #252 and #260 into it.

bajtos suggested changes Jul 26, 2017

View reviewed changes

bajtos added the feature label Jul 26, 2017

bajtos self-assigned this Jul 26, 2017

bajtos mentioned this pull request Jul 26, 2017

Add support for ES6 style async boot scripts #253

Merged

2 tasks

bajtos suggested changes Jul 28, 2017

View reviewed changes

bajtos reviewed Jul 31, 2017

View reviewed changes

Add support for ES6 style async boot scripts

5132609

bajtos approved these changes Jul 31, 2017

View reviewed changes

bajtos merged commit 7f34b0d into strongloop:2.x Jul 31, 2017

lehni mentioned this pull request Aug 15, 2017

Do not call callbacks twice in async boot scripts #260

Merged

2 tasks

klarkc mentioned this pull request Jun 17, 2018

Add support for es6 modules in boot scripts #280

Merged

2 tasks

bajtos mentioned this pull request Jul 16, 2018

Allow boot scripts to return promise instead of accepting a callback arg #192

Closed

Conversation

lehni commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

slnode commented Jul 24, 2017

Uh oh!

slnode commented Jul 24, 2017

Uh oh!

slnode commented Jul 24, 2017

Uh oh!

slnode commented Jul 24, 2017

Uh oh!

bajtos commented Jul 26, 2017

Uh oh!

bajtos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 26, 2017

Uh oh!

lehni commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Jul 27, 2017

Uh oh!

lehni commented Jul 28, 2017

Uh oh!

lehni commented Jul 28, 2017

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lehni Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 28, 2017

Uh oh!

lehni commented Jul 28, 2017

Uh oh!

lehni commented Jul 28, 2017

Uh oh!

bajtos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 31, 2017

Uh oh!

bajtos commented Jul 31, 2017

Uh oh!

lehni commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lehni commented Jul 31, 2017

Uh oh!

slnode commented Jul 31, 2017

Uh oh!

lehni commented Aug 1, 2017

Uh oh!

bajtos commented Aug 3, 2017

Uh oh!

lehni commented Jul 24, 2017 •

edited

Loading

bajtos left a comment •

edited

Loading

lehni commented Jul 27, 2017 •

edited

Loading

lehni Jul 28, 2017 •

edited

Loading

bajtos left a comment •

edited

Loading

lehni commented Jul 31, 2017 •

edited

Loading