Skip to content

Support async app loading - #5

Closed
ryedin wants to merge 5 commits into
strongloop:masterfrom
ryedin:async_app_loading
Closed

Support async app loading#5
ryedin wants to merge 5 commits into
strongloop:masterfrom
ryedin:async_app_loading

Conversation

@ryedin

@ryedin ryedin commented May 7, 2014

Copy link
Copy Markdown

This is needed for the cases where the app wants to go fetch metadata during the loopback startup phase. The main use case is to grab data from a remote source in order to use introspection to build the models.

Comment thread README.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please omit the boilerplate code to keep the example short.

Example:

myAsyncThing.run(function(metadata) {
  // Configure LoopBack models and datasources
  app.boot(__dirname);

  //use the model metadata to introspectively register new models
  myAsyncThing.buildModels(metadata);

  // other configuration as scaffolded by `slc lb project`
  app.use(loopback.favicon());
  // etc.
  app.enableAuth();

  //app API is ready to go -- time to alert anyone who cares (lookin' at you grunt-loopback-angular)
  doOnReady();

  if(require.main === module) {
    // start the http server
  }
});

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a problem... update to PR coming

@bajtos

bajtos commented May 9, 2014

Copy link
Copy Markdown
Member

Hi @ryedin, thank you for the pull request. The feature makes sense. See the comments above for things to improve/fix.

@slnode

slnode commented May 9, 2014

Copy link
Copy Markdown

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

@ryedin

ryedin commented May 10, 2014

Copy link
Copy Markdown
Author

PR updated with recommended changes.

@bajtos

bajtos commented May 12, 2014

Copy link
Copy Markdown
Member

Excellent, the patch looks much better now.

I was figuring what's being tested is "everything still works when the loopback app being introspected has been loaded asynchronously", so I thought it made sense to re-run all the tests.

Makes sense. IMO the extra complexity needed to implement this outweigh the benefits.

A side of effect of changing to an event rather than a callback registration function is that it became even harder to test in the context of using the grunt task in multi- mode.

I look to your guidance there... I personally don't think that's a huge issue because I can't imagine a real world case where you'd be running this task against multiple configuration options and outputting multiple outputs for a single app. The problem of course comes from the caching nature of require (once the task runs once, the app has been required and the specified ready event has long gone...)

Here are few possible solutions:

  1. Ensure the app file is not cached. The drawback: the app boostrap is repeated for every configuration, which can add up to a long duration.

    delete require.cache[require.resolve(path.resolve(appFile))];
    app = require(path.resolve(appFile));
  2. Keep track of which app objects were already initialised, e.g. by injecting a flag to the app object. Since the app object is thrown away later, it should be ok to modify it.

    if (options.appReadyEvent && !app.__isReady) {
      //register with the app's ready event
      app.on(options.appReadyEvent, function() {
        app.__isReady = true;
        completeTask();
      });  
    } else {
      completeTask();
    }

Comment thread Gruntfile.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test:sync and test:async are no longer needed, the async test can be defined in the same way as default_options and custom_options:

     loopback_angular: {
        options: {
          input: 'test/fixtures/app.js'
        },
        default_options: {
          options: {
            output: 'tmp/default_options'
          }
        },
        custom_options: {
          options: {
            output: 'tmp/custom_options',
            ngModuleName: 'customServices',
            apiUrl: 'http://custom/api/'
          }
        },
        async_app: {
          options: {
            input: 'test/fixtures/app-async.js',
            output: 'tmp/async_app'
          }
        }
      },

The nodeunit test code can go to test/loopback_angular_test.js.

Mayhap I am missing the point of separating sync & async tests. What benefits do you see in splitting sync and async tests?

@bajtos

bajtos commented Jan 14, 2015

Copy link
Copy Markdown
Member

Hey @ryedin, what's the status of this pull request? Are you still keen to finish it up?

Note that loopback-boot now supports async booting process OOTB, see strongloop/loopback-boot#77 and strongloop/loopback-boot#50

@slnode

slnode commented Jan 14, 2015

Copy link
Copy Markdown

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

@bajtos bajtos added the waiting label Jan 14, 2015
@bajtos

bajtos commented Jan 19, 2015

Copy link
Copy Markdown
Member

This pull request seems to be abandoned, I am closing it. Feel free to reopen when you are ready to update and finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants