Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,32 @@ module.exports = function(grunt) {

// Whenever the "test" task is run, first clean the "tmp" dir, then run this
// plugin's task(s), then test the result.
grunt.registerTask('test', ['clean', 'loopback_angular', 'nodeunit']);
grunt.registerTask('test:sync', ['clean', 'loopback_angular', 'nodeunit']);

// need a special config and fixture to test the async case
grunt.registerTask(
'test:async',
'Configure the fixture and options for the async test and run it.',
function() {

grunt.config.set('loopback_angular', {
services: {
options: {
input: 'test/fixtures/app-async.js',
appReadyEvent: 'ready',
output: 'tmp/async_app'
}
}
});

grunt.config.set('nodeunit', {
tests: ['test/async/*_test.js']
});

grunt.task.run('clean', 'loopback_angular', 'nodeunit');
});

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.

It's generally better to keep the tests focused on one thing. This setup runs all tests in async mode again, even though it should be enough to run only one scenario.

Considering the additional complexity caused in app-async.js, I strongly prefer to create a new test case that tests the async mode only and is never run synchronously.

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.

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. What would you test differently with a different test case aimed solely at the async case?

In other words, the fixture changes, but what needs to be tested for the sync app === what needs to be tested for the async app, no?

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.

OK, so I added an async-specific test.

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...)


grunt.registerTask('test', ['test:sync', 'test:async']);

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?


// By default, lint and run all tests.
grunt.registerTask('default', ['jshint', 'test']);
Expand Down
74 changes: 74 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,80 @@ See the official
[LoopBack AngularJS SDK documentation](http://docs.strongloop.com/display/DOC/AngularJS+JavaScript+SDK#AngularJSJavaScriptSDK-Gruntplugin)
for the description of tasks provided by this Grunt plugin.

## Async loading of LoopBack App

If your LoopBack app (i.e. the `input` option for this grunt task) needs to perform asynchronous operations before this task runs (like fetch a set of metadata that you want to introspect to build your models), you must export and specify an `appReady` method that registers one or more callback functions and calls them when it is done building/configuring LoopBack.

EXAMPLE: your `app.js` might look something like this...

```
var loopback = require('loopback'),
app = module.exports = loopback();

//do something asyncronously that gets me some model metadata I can use later
var myAsyncThing = require('my-async-model-building-module');

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)
app.emit('ready');

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

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


```

And then your Gruntfile would look like this…

```
module.exports = function(grunt) {
grunt.initConfig({
loopback_angular: {
services: {
options: {
input: './app.js',
appReadyEvent: 'ready',
output: '../client/app/scripts/lb-services.js'
}
}
},
docular: {
groups: [
{
groupTitle: 'LoopBack',
groupId: 'loopback',
sections: [
{
id: 'lbServices',
title: 'LoopBack Services',
scripts: [ '../client/app/scripts/lb-services.js' ]
}
]
}
]
}
});
// Load the plugin that provides the "loopback-angular" and "grunt-docular" tasks.
grunt.loadNpmTasks('grunt-loopback-angular');
grunt.loadNpmTasks('grunt-docular');
// Default task(s).
grunt.registerTask('default', ['loopback_angular', 'docular']);
};
```

## Contributing

See [CONTRIBUTING.md](CONTRIBUTING.md)
Expand Down
29 changes: 22 additions & 7 deletions tasks/loopback_angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ module.exports = function(grunt) {
function runTask() {
/*jshint validthis:true */

// need this to accomodate an input file that builds the loopback
// interface asynchronously
var done = this.async();

// Merge task-specific and/or target-specific options with these defaults.
var options = this.options({
ngModuleName: 'lbServices',
Expand All @@ -50,16 +54,27 @@ module.exports = function(grunt) {
grunt.fail.warn(err);
}

options.apiUrl = options.apiUrl || app.get('restApiRoot') || '/api';
var completeTask = function() {
options.apiUrl = options.apiUrl || app.get('restApiRoot') || '/api';

grunt.log.writeln('Generating %j for the API endpoint %j',
options.ngModuleName,
options.apiUrl);
grunt.log.writeln('Generating %j for the API endpoint %j',
options.ngModuleName,
options.apiUrl);

var script = generator.services(app, options.ngModuleName, options.apiUrl);
var script = generator.services(app, options.ngModuleName, options.apiUrl);

grunt.file.write(options.output, script);
grunt.file.write(options.output, script);

grunt.log.ok('Generated Angular services file %j', options.output);
grunt.log.ok('Generated Angular services file %j', options.output);

done();
};

if (options.appReadyEvent) {
//register with the app's ready event
app.on(options.appReadyEvent, completeTask);
} else {
completeTask();
}

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.

The grunt task is intended to be a thin wrapper, the code not specific to grunt should live in loopback-angular, so that it can be shared by both grunt-loopback-angular and loopback-angular-cli. The logic dealing with asynchronous application bootstrap should be implemented in loopback-angular, not here.

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.

On the second though, the API of loopback-angular generator accepts an (initialized) app object, I suppose it makes sense to leave the actual job of require-ing and loading the app to the caller.

Please ignore the comment asking the async logic to loopback-angular core.

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.

right, the grunt task had to change for this feature as it was assuming all setup was being done synchronously, resulting in missing ngResource stubs.

}
};
43 changes: 43 additions & 0 deletions test/async/loopback_angular_async_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

var grunt = require('grunt');
var parse = require('loopback-angular/parse-helper');

/*
======== A Handy Little Nodeunit Reference ========
https://github.com/caolan/nodeunit

Test methods:
test.expect(numAssertions)
test.done()
Test assertions:
test.ok(value, [message])
test.equal(actual, expected, [message])
test.notEqual(actual, expected, [message])
test.deepEqual(actual, expected, [message])
test.notDeepEqual(actual, expected, [message])
test.strictEqual(actual, expected, [message])
test.notStrictEqual(actual, expected, [message])
test.throws(block, [error], [message])
test.doesNotThrow(block, [error], [message])
test.ifError(value)
*/

// Note: the tests are depending on targets configured in Gruntfile.js

exports.loopback_angular = {
setUp: function(done) {
// setup here if necessary
done();
},

async_app: function(test) {
var script = grunt.file.read('tmp/async_app');

// the value "lbservices" is the --module-name default
test.equal(parse.moduleName(script), 'lbServices');

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.

This is not async-specific, please remove.

// the value "/rest-api-root" is hard-coded in fixtures/app.js
test.equal(parse.baseUrl(script), '/rest-api-root');

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.

This relies on the fact that app.set('restApiRoot') is called from the next tick in the app-async.js and that the default value is /api. Could you please extend the comment to explain that?

test.done();
}
};
21 changes: 21 additions & 0 deletions test/fixtures/app-async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
var loopback = require('loopback');
var app = loopback();

module.exports = app;

//simulate the case where there are async operations that the app does before it's ready to have its lb-services generated
process.nextTick(function() {

// Setup default datasources for autoAttach()
app.dataSource('db', { connector: 'memory', defaultForType: 'db' });
app.dataSource('mail', { connector: 'mail', defaultForType: 'mail' });

// Attach all built-in models
loopback.autoAttach();

// Configure REST API path
app.set('restApiRoot', '/rest-api-root');

//let anything that cares (require'ing code) know the app is ready
app.emit('ready');
});