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
4 changes: 4 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,14 @@ var addInstructionsToBrowserify = require('./lib/bundler');
* `production`; however the applications are free to use any names.
* @property {Array.<String>} [modelSources] List of directories where to look
* for files containing model definitions.
* @property {Array.<String>} [mixinSources] List of directories where to look
* for files containing model mixin definitions. - defaults to ['./mixins']
* @property {Array.<String>} [bootDirs] List of directories where to look
* for boot scripts.
* @property {Array.<String>} [bootScripts] List of script files to execute
* on boot.
* @property {String|Function|Boolean} [normalization] Mixin normalization format
* can be false, 'none', 'classify', 'dasherize' - defaults to 'classify'.
* @end
*
* @header boot(app, [options])
Expand Down
38 changes: 38 additions & 0 deletions lib/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ module.exports = function compile(options) {

var bootScripts = options.bootScripts || [];
bootDirs.forEach(function(dir) {
dir = path.resolve(dir);
bootScripts = bootScripts.concat(findScripts(dir));
});

Expand All @@ -59,19 +60,40 @@ module.exports = function compile(options) {
var modelInstructions = buildAllModelInstructions(
modelsRootDir, modelsConfig, modelSources);

var mixinSources = options.mixinSources || modelsMeta.mixins || ['./mixins'];
var mixinInstructions = buildAllMixinInstructions(appRootDir, mixinSources, options);

// When executor passes the instruction to loopback methods,
// loopback modifies the data. Since we are loading the data using `require`,
// such change affects also code that calls `require` for the same file.
return cloneDeep({
config: appConfig,
dataSources: dataSourcesConfig,
models: modelInstructions,
mixins: mixinInstructions,
files: {
boot: bootScripts
}
});
};

function buildAllMixinInstructions(appRootDir, mixinSources, options) {
var files = options.mixins || [];
Copy link
Member

Choose a reason for hiding this comment

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

Please add some documentation for options.mixins to index.js.

mixinSources.forEach(function(dir) {
dir = path.resolve(appRootDir, dir);
files = files.concat(findScripts(dir));
});

var mixins = {};
files.forEach(function(filepath) {
var ext = path.extname(filepath);
var name = normalizeMixinName(path.basename(filepath, ext), options);
mixins[name] = filepath;
Copy link
Member

Choose a reason for hiding this comment

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

As I have commented in [https://github.com/loopbackio/loopback-datasource-juggler/pull/229], the mixin name should be normalized here, not in the juggler.

At least these two naming conventions should be supported: international-address.js and InternationalAddress.js.

Copy link
Member

Choose a reason for hiding this comment

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

BTW the same algorithm can be used to provide a default value for model names as discussed elsewhere (could not find the GH issue, sorry).

});

return mixins;
};

function assertIsValidConfig(name, config) {
if(config) {
assert(typeof config === 'object',
Expand Down Expand Up @@ -294,3 +316,19 @@ function loadModelDefinition(rootDir, jsonFile) {
sourceFile: sourceFile
};
}

function normalizeMixinName(str, options) {
var normalization = options.normalization;
if (normalization === false || normalization === 'none') return str;
if (normalization === 'dasherize') {
str = String(str).replace(/[\W_]/g, ' ').toLowerCase();
str = str.replace(/\s+/g, '-');
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using lodash instead of writing the code by hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but for proper normalization, you'd also need underscore.string or something similar. I didn't feel like adding more dependencies to a core component like this.

Copy link
Member

Choose a reason for hiding this comment

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

Adding dependencies to core components is fine, it is better than reinventing the wheel. BTW loopback has both "underscore" and "underscore.string" as dependency.

} else if (typeof normalization === 'function') {
str = normalization(str);
} else { // classify
str = String(str).replace(/[\W_]/g, ' ').toLowerCase();
str = str.replace(/(?:^|\s|-)\S/g, function(c){ return c.toUpperCase(); });
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above, we really should not reimplement this ourselves.

str = str.replace(/\s+/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

This lines seems redundant to me, as any whitespace should have been already processed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which line exactly? and where is the whitespace processed before?

Copy link
Member

Choose a reason for hiding this comment

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

Ignore this comment, I don't know why I thought whitespace was already removed.

}
return str;
}
22 changes: 22 additions & 0 deletions lib/executor.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ function setupDataSources(app, instructions) {
}

function setupModels(app, instructions) {
defineMixins(app, instructions);
defineModels(app, instructions);

instructions.models.forEach(function(data) {
Expand All @@ -145,6 +146,27 @@ function setupModels(app, instructions) {
});
}

function defineMixins(app, instructions) {
var modelBuilder = app.loopback.modelBuilder;
var BaseClass = app.loopback.Model;
var mixins = instructions.mixins || {};
var mixinNames = Object.keys(mixins);
if (modelBuilder.mixins && mixinNames.length > 0) {
mixinNames.forEach(function(name) {
var mixin = tryRequire(mixins[name]);
if (typeof mixin === 'function' || mixin.prototype instanceof BaseClass) {
Copy link
Member

Choose a reason for hiding this comment

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

I find this confusing. If mixin is a loopback model, then it must be a function. If the mixin is not a function, then it won't have a prototype property, right?

Copy link
Member

Choose a reason for hiding this comment

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

I am taking it back. One of my earlier comments mentions that a mixin can be either a function or an object. If I understand the intention correctly, the latter allows the developer to specify a mixin like an object with some properties and these properties are then added to the target model.

I don't know enough about the implementation in juggler, is it necessary that this object has to be an instance of loopback Model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var mixinName = mixin.name || mixin.mixinName || name;
debug('Configuring mixin %s', mixinName);
modelBuilder.mixins.define(mixinName, mixin);
} else {
debug('Skipping mixin file %s - `module.exports` is not a function'
+ ' or Loopback model',
mixins[name]);
}
Copy link
Member

Choose a reason for hiding this comment

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

My understanding of loopbackio/loopback-datasource-juggler#229 is that a mixing can be an object too?

Please add debug statements to make troubleshooting easier, especially when the condition on L148 is false. See defineModels for inspiration.

});
}
}

function defineModels(app, instructions) {
instructions.models.forEach(function(data) {
var name = data.name;
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"browserify": "^4.1.8",
"fs-extra": "^0.10.0",
"jshint": "^2.5.6",
"loopback": "^1.5.0",
"loopback": "^2.2.0",
"mocha": "^1.19.0",
"must": "^0.12.0",
"supertest": "^0.13.0"
Expand Down
42 changes: 42 additions & 0 deletions test/compiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var sandbox = require('./helpers/sandbox');
var appdir = require('./helpers/appdir');

var SIMPLE_APP = path.join(__dirname, 'fixtures', 'simple-app');
var MIXIN_SOURCES = path.join(__dirname, 'fixtures', 'mixins');

describe('compiler', function() {
beforeEach(sandbox.reset);
Expand Down Expand Up @@ -620,6 +621,47 @@ describe('compiler', function() {
instructions = boot.compile(appdir.PATH);
expect(instructions.config).to.not.have.property('modified');
});

it('has a mixinSources option', function() {
var options = { appRootDir: SIMPLE_APP, mixinSources: [MIXIN_SOURCES] };
var instructions = boot.compile(options);
expect(instructions.mixins).to.not.have.property('TimeStamps');
expect(instructions.mixins).to.have.property('Other');
});

it('supports mixin name normalization - classify (default)', function() {
var options = { appRootDir: SIMPLE_APP };
var instructions = boot.compile(options);
expect(instructions.mixins).to.have.property('Example');
expect(instructions.mixins).to.have.property('Foo');
expect(instructions.mixins).to.have.property('TimeStamps');
});

it('supports mixin name normalization - dasherize', function() {
var options = { appRootDir: SIMPLE_APP, normalization: 'dasherize' };
var instructions = boot.compile(options);
expect(instructions.mixins).to.have.property('example');
expect(instructions.mixins).to.have.property('foo');
expect(instructions.mixins).to.have.property('time-stamps');
});

it('supports mixin name normalization - custom function', function() {
var normalize = function(name) { return name.toUpperCase(); };
var options = { appRootDir: SIMPLE_APP, normalization: normalize };
var instructions = boot.compile(options);
expect(instructions.mixins).to.have.property('EXAMPLE');
expect(instructions.mixins).to.have.property('FOO');
expect(instructions.mixins).to.have.property('TIME-STAMPS');
});

it('supports skip mixin name normalization - none', function() {
var options = { appRootDir: SIMPLE_APP, normalization: false };
var instructions = boot.compile(options);
expect(instructions.mixins).to.have.property('example');
expect(instructions.mixins).to.have.property('Foo');
expect(instructions.mixins).to.have.property('time-stamps');
});

});
});

Expand Down
19 changes: 18 additions & 1 deletion test/executor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,15 @@ describe('executor', function() {
done();
}, 10);
});

it('should define `mixins/*` files', function() {
if (app.loopback.datasourceJuggler && app.loopback.datasourceJuggler.mixins) {
var mixins = app.loopback.datasourceJuggler.mixins;
expect(mixins.registry).to.have.property('Example');
expect(mixins.registry).to.have.property('TimeStamps');
expect(mixins.registry).to.have.property('bar'); // mixinName
}
});
});

describe('with boot with callback', function() {
Expand All @@ -217,7 +226,15 @@ describe('executor', function() {
done();
});
});


it('should define `mixins/*` files', function() {
var modelBuilder = app.loopback.modelBuilder;
var registry = modelBuilder.mixins.mixins;
expect(registry).to.have.property('Example');
expect(registry).to.have.property('TimeStamps');
expect(registry).to.have.property('bar'); // mixinName
});

});

describe('with PaaS and npm env variables', function() {
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/mixins/other.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = function(Model, options) {

Model.otherMixin = true;

}
10 changes: 10 additions & 0 deletions test/fixtures/simple-app/mixins/Foo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// When you create a named function, its name will be used
// as the mixin name - alternatively, set mixin.mixinName.

var mixin = function bar(Model, options) {

Model.barMixin = true;

};

module.exports = mixin;
5 changes: 5 additions & 0 deletions test/fixtures/simple-app/mixins/example.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = function(Model, options) {

Model.exampleMixin = true;

}
5 changes: 5 additions & 0 deletions test/fixtures/simple-app/mixins/time-stamps.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = function(Model, options) {

Model.timeStampsMixin = true;

}