[SEMVER-MAJOR] Refactor for modular and pluggable design by raymondfeng · Pull Request #181 · strongloop/loopback-boot

raymondfeng · 2016-04-08T15:13:52Z

connect to strongloop-internal/scrum-loopback/issues/1136

The current boot process consists of three steps:

load
compile
execute

It hard-codes logic to handle various artifact types:

data source
model
mixin
middleware
boot script

The problems with the current design/implementation are:

lack of extensibility
no clean separation of different types of artifacts

The PR proposes the following changes:

refactor logic of processing artifacts into their own classes
introduce Container as the main class for bootstrapping and build a registry of handlers during boot to organize them by a hierarchy denoted by path
adopt middleware like registration and invocation
- container.use(path, handler)
- container.run(context, done)
allow more phases during boot

- explore dependency injection

Connect to #188

Update 16/11/16:

needs review by @raymondfeng, maybe @strongloop/squad-epitome can help out?

bajtos · 2016-04-12T07:58:50Z

Hey @raymondfeng, the proposal looks good from high-level view. I have few questions for you:

How do you envision supporting browserify scenario, where there are two steps run in different environments:

the first step (compile + browserify) is run in Node.js on a dev machine (or build server) and creates a set of instructions for the second step,
and the second steps executes these instructions in a browser.

In the current version, bootstrapping of models has two phases: in the first phase, all models are defined in juggler's model registry. In the second phase, which is run after datasources are created, the models are attached to datasources and registered with app's RemoteObject instance to expose them via public (REST) API. Does your proposal take this into account?
It is important to preserve the order in which different artifacts are created. For example, to attach models to datasources, datasources must be already setup; boot scripts should be generally run at the very end, when everything else is already set up. How do you propose to define this ordering?

adopt middleware like registration and invocation
container.use(path, handler)
container.run(context, done)

What is the purpose of this feature? Could you please provide examples where one would use this feature and how the code will look like?

allow more phases during boot

Again, could you please show some examples of how this would be used?

build a registry of handlers during boot to organize them by a hierarchy denoted by path

From your proposal, it is not clear to me what these paths will be used for. Could you please elaborate a bit more? How will these paths relate to the need of having a well-defined order in which handlers are executed?

superkhau · 2016-04-14T20:51:19Z

While we're at it, @davidcheung brought up a good point about config.json vs config.js inconsistencies with our configurations. We should have the new loopback-boot respect base.js files too (config.js, model-config.js, datasources.js) etc.

See strongloop/loopback#2103

pbhadauria2000 · 2016-04-19T16:53:29Z

Can you also make the DYNAMIC_CONFIG_PARAM regex to a bit flexible. Something like /(.)${(\w+)}(.)/ and then you can do configVariable = match(1) + configVariable + match(3)

raymondfeng · 2016-04-26T22:54:26Z

Connect to #188

raymondfeng · 2016-05-02T22:47:03Z

@pbhadauria2000

Can you also make the DYNAMIC_CONFIG_PARAM regex to a bit flexible. Something like /(.)${(\w+)}(.)/ and then you can do configVariable = match(1) + configVariable + match(3)

Can you provide an example?

pbhadauria2000 · 2016-05-03T04:35:10Z

@raymondfeng

I would like to load sampleData files for memory datasource from different folders based on variables in config.json. Example: file: "samples{$dataFolder}\product.json". In this example, I can keep the value of the variable "dataFolder" defined in config which could be environment specific.

bajtos · 2016-05-10T13:44:42Z

browser.js

 */

-exports = module.exports = function bootBrowserApp(app, options) {
+exports = module.exports = function bootBrowserApp(app, options, callback) {


Since this is for LoopBack Next, can we drop support for callbacks and switch exclusively to Promise-based APIs here in loopback-boot?

I want to do this incrementally. The current implementation is ready for LoopBack 3.0. We can further clean it up for LoopBack.next if we decide to drop callback style.

Makes sense.

bajtos · 2016-09-16T08:22:18Z

@davidcheung once #211 is landed, please add a description to 3.0-RELEASE-NOTES.md as part of this pull request (see #181 (comment) for starter).

davidcheung · 2016-09-19T21:00:41Z

@slnode test please

bajtos

@davidcheung I have reviewed the changes as well as I could. I did not review the overall design again, as I have already commented on it earlier.

Besides the comments listed below, this pull request needs an entry in 3.0-RELEASE-NOTES.md describing the breaking changes.

I think it would be worthwhile to describe the new features in 3.0-RELEASE-NOTES too (the modular design and how to build 3rd party boot plugins). In my experience, the process of writing the documentation usually discover missing pieces and/or rough edges. However, I am fine with leaving the description of the new features out of scope of this pull request.

bajtos · 2016-10-11T13:02:11Z

lib/bootstrapper.js

+var debug = require('debug')('loopback:boot:bootstrapper');
+var Promise = global.Promise || require('bluebird');
+
+module.exports = function(options) {


I personally prefer to export Bootstrapper directly.

module.exports = Bootstrapper;

When I look at the code above, I see 2 places with require('...boostrapper').Bootstrapper and only one place calling the exported function directly.

bajtos · 2016-10-11T13:04:30Z

lib/bootstrapper.js

+  try {
+    loadAndRegisterPlugins(self, options);
+  } catch (err) {
+    debug(err);


Please include a message explaining what went wrong, e.g.

debug('Cannot load & register plugins: %s', err.stack || err);

bajtos · 2016-10-11T13:07:26Z

lib/bootstrapper.js

+var path = require('path');
+var pluginLoader = require('./plugin-loader');
+var debug = require('debug')('loopback:boot:bootstrapper');
+var Promise = global.Promise || require('bluebird');


In LoopBack 3.x, we are always using bluebird as our promise implementation. I am proposing to do the same in loopback-boot for consistency.

var Promise = require('bluebird');

bajtos · 2016-10-11T13:11:48Z

lib/bootstrapper.js

+      } else {
+        result = plugin.handler[phase](context);
+        if (typeof Promise !== 'undefined') {
+          Promise.resolve(result).then(function(value) {


Is this trying to implement support for handler functions returning a promise? Here is the version that we use elsewhere in LoopBack (example, it is addressing many subtle edge cases that are handled incorrectly by this code.

if (result && typeof result.then === 'function') { result.then( function onPluginPhaseResolved(value) { done(null, value); }, function onPluginPhaseRejected(err) { debug('Unable to invoke %s.%s()', plugin.name, phase, err); done(err); }); } else { done(null, result); }

bajtos · 2016-10-11T13:14:12Z

lib/bootstrapper.js

+};
+
+function pluginIteratorFactory(context, phase) {
+  return function(plugin, done) {


Please use named functions wherever possible, e.g.

return function executePluginPhase(plugin, done) { // ... };

bajtos · 2016-10-11T13:36:49Z

test/browser.test.js

-      expect(app.dummyComponentOptions).to.eql({ option: 'value' });
-
-      done();
+      var app = executeBundledApp(bundlePath, function(err) {


unhandled err

bajtos · 2016-10-11T13:37:23Z

test/browser.test.js

+    printContextLogs(context);
+    done(err);
+  });
  return app;


An async function that also returns a value in sync way is very weird. Please pass app to the callback.

app.start(function(err) { printContextLogs(context); done(err, app); });

bajtos · 2016-10-11T13:39:46Z

test/executor.test.js

+      }), function(err, context) {
+        if (err) return done(err);
+        expect(app.models.Customer._modelsWhenAttached).
+        to.include('UniqueName');


expect(app..._modelsWhenAttached) .to.include('UniqueName');

bajtos · 2016-10-11T13:41:12Z

test/executor.test.js

+      function(err) {
+        expect(function() {
+          if (err) throw err;
+        }).to.throw(/Cannot find module \'doesnt-exist\'/);


Huh?? This is overcomplicated, just use something like the following:

expect(err && err.message).to.match(/Cannot find module \'doesnt-exist\'/);

bajtos · 2016-10-11T13:42:13Z

test/executor.test.js

      ]);
-
-      // bar finished happens in the next tick
-      // barSync executed after bar finished


I think this change needs to be clearly explained in 3.0-RELEASE-NOTES.md.

davidcheung · 2016-11-11T21:57:18Z

@bajtos can you PTAL, the changes are mostly logically separated into their own commit, but probably still quite difficult to navigate due to size of PR :(

davidcheung · 2016-11-14T15:08:56Z

I think it would be worthwhile to describe the new features in 3.0-RELEASE-NOTES too (the modular design and how to build 3rd party boot plugins). In my experience, the process of writing the documentation usually discover missing pieces and/or rough edges. However, I am fine with leaving the description of the new features out of scope of this pull request.

i have only included one change in the release-note.md, will open another issue to document the modular design, and how to create your own plugin.

bajtos

I have reviewed the new commits, left two nit-picks, the rest looks good to me.

After this patch is landed, could you @davidcheung please upgrade eslint-config-loopback to v5 and fix linter errors?

bajtos · 2016-11-22T13:13:45Z

lib/plugins/model.js

    });

-  var sortedNames = toposort(edges);
+  var _sortedNames = toposort(edges);


We usually don't prefix variable names with _.

bajtos · 2016-11-22T13:14:03Z

lib/utils.js

  var files = tryReadDir(sourceDir);
-  var fixedFile = fixFileExtension(resolvedPath, files, false);
-  return (fixedFile === undefined ? resolvedPath : fixedFile);
+  var _fixedFile = fixFileExtension(resolvedPath, files, false);


We usually don't prefix variable names with _.

bajtos · 2016-11-22T13:18:09Z

@davidcheung also please don't forget to squash the commits before landing!

- refactor logic of processing artifacts into their own classes - introduce Container as the main class for bootstrapping and build a registry of handlers during boot to organize them by a hierarchy denoted by path - adopt middleware like registration and invocation - container.use(path, handler) - container.run(context, done) - allow more phases during boot - boot is now asynchronous

raymondfeng added the #review label Apr 8, 2016

raymondfeng force-pushed the feature/extensibility branch 3 times, most recently from 079b837 to 5c35225 Compare April 19, 2016 00:08

bajtos mentioned this pull request Apr 19, 2016

Spike: make boot more flexible #188

Closed

raymondfeng force-pushed the feature/extensibility branch 10 times, most recently from 3ba32c0 to 1f81231 Compare April 26, 2016 18:17

raymondfeng changed the title ~~[WIP] Initial PoC to refactor loopback-boot (DO NOT MERGE)~~ Initial PoC to refactor loopback-boot Apr 26, 2016

raymondfeng assigned bajtos Apr 26, 2016

superkhau mentioned this pull request Apr 27, 2016

disable boot scripts #191

Closed

raymondfeng force-pushed the feature/extensibility branch 2 times, most recently from 8555f13 to 41c95cd Compare April 28, 2016 06:53

raymondfeng mentioned this pull request May 4, 2016

executor: allow to boot loopback without remotes #184

Closed

raymondfeng force-pushed the feature/extensibility branch from 41c95cd to 51af40f Compare May 4, 2016 22:58

bajtos reviewed May 10, 2016
View reviewed changes

bajtos added the API Connect label May 12, 2016

davidcheung force-pushed the feature/extensibility branch 2 times, most recently from 96356e5 to c032849 Compare September 19, 2016 20:55

davidcheung force-pushed the feature/extensibility branch from c032849 to 251835f Compare September 27, 2016 19:24

davidcheung mentioned this pull request Sep 27, 2016

create loopback-2.x branch, use loopback@3 in master #210

Closed

davidcheung force-pushed the feature/extensibility branch from 251835f to da89ace Compare September 28, 2016 14:27

bajtos suggested changes Oct 11, 2016

View reviewed changes

bajtos changed the title ~~[SEMVER-MAJOR] Initial PoC to refactor loopback-boot~~ [SEMVER-MAJOR] Refactor for modular and pluggable design Oct 11, 2016

bajtos mentioned this pull request Oct 12, 2016

Fix resolution of middleware modules [2.x] #222

Merged

davidcheung mentioned this pull request Oct 12, 2016

[forward-port] Fix resolution of middleware modules #223

Closed

1 task

davidcheung force-pushed the feature/extensibility branch from 22ceee8 to 8138232 Compare October 26, 2016 22:39

pthieu added the epitome-team label Nov 10, 2016

davidcheung force-pushed the feature/extensibility branch 2 times, most recently from eb391c4 to 70af0d3 Compare November 11, 2016 21:40

davidcheung mentioned this pull request Nov 14, 2016

Document refactored design of loopback-boot #230

Closed

3 tasks

bajtos approved these changes Nov 22, 2016

View reviewed changes

bajtos assigned davidcheung and unassigned raymondfeng and bajtos Nov 22, 2016

davidcheung force-pushed the feature/extensibility branch 2 times, most recently from d531771 to 64a62ba Compare November 22, 2016 17:20

davidcheung force-pushed the feature/extensibility branch from 64a62ba to ac1571c Compare November 22, 2016 18:40

davidcheung merged commit f7c9cbc into master Nov 22, 2016

davidcheung removed the #review label Nov 22, 2016

davidcheung deleted the feature/extensibility branch November 22, 2016 19:24

Conversation

raymondfeng commented Apr 8, 2016 • edited by davidcheung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

- explore dependency injection

Uh oh!

bajtos commented Apr 12, 2016

Uh oh!

superkhau commented Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbhadauria2000 commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raymondfeng commented Apr 26, 2016

Uh oh!

raymondfeng commented May 2, 2016

Uh oh!

pbhadauria2000 commented May 3, 2016

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 Sep 16, 2016

Uh oh!

davidcheung commented Sep 19, 2016

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!

Choose a reason for hiding this comment

Uh oh!

bajtos Oct 11, 2016 • 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 Oct 11, 2016 • 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!

davidcheung commented Nov 11, 2016

Uh oh!

davidcheung commented Nov 14, 2016

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!

bajtos commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

raymondfeng commented Apr 8, 2016 •

edited by davidcheung

Loading

superkhau commented Apr 14, 2016 •

edited

Loading

pbhadauria2000 commented Apr 19, 2016 •

edited

Loading

bajtos Oct 11, 2016 •

edited

Loading

bajtos Oct 11, 2016 •

edited

Loading