Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support 'mixinsources' option #131

Merged
merged 1 commit into from
May 7, 2015
Merged

support 'mixinsources' option #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
76 changes: 69 additions & 7 deletions lib/compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ module.exports = function compile(options) {
modelsRootDir, modelsConfig, modelSources);

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

// When executor passes the instruction to loopback methods,
// loopback modifies the data. Since we are loading the data using `require`,
Expand Down Expand Up @@ -606,21 +607,61 @@ function resolveAppScriptPath(rootDir, relativePath, resolveOptions) {
return (fixedFile === undefined ? resolvedPath : fixedFile);
}

function buildAllMixinInstructions(appRootDir, mixinDirs, options) {
function buildAllMixinInstructions(appRootDir, mixinDirs, mixinSources, options,
modelInstructions) {
var extensions = _.without(_.keys(require.extensions),
_.keys(getExcludedExtensions()));
var files = options.mixins || [];

mixinDirs.forEach(function(dir) {
// load mixins from `options.mixins`
var sourceFiles = options.mixins || [];
var instructionsFromMixins = loadMixins(sourceFiles, options);

// load mixins from `options.mixinDirs`
sourceFiles = findMixinDefinitions(appRootDir, mixinDirs, extensions);
if (sourceFiles === undefined) return;
var instructionsFromMixinDirs = loadMixins(sourceFiles, options);

/* If `mixinDirs` and `mixinSources` have any directories in common,
* then remove the common directories from `mixinSources` */
mixinSources = _.difference(mixinSources, mixinDirs);

// load mixins from `options.mixinSources`
sourceFiles = findMixinDefinitions(appRootDir, mixinSources, extensions);
if (sourceFiles === undefined) return;
var instructionsFromMixinSources = loadMixins(sourceFiles, options);

// Fetch unique list of mixin names, used in models
var modelMixins = fetchMixinNamesUsedInModelInstructions(modelInstructions);
modelMixins = _.unique(modelMixins);

// Filter-in only mixins, that are used in models
instructionsFromMixinSources = filterMixinInstructionsUsingWhitelist(
instructionsFromMixinSources, modelMixins);

var mixins = _.assign(
instructionsFromMixins,
instructionsFromMixinDirs,
instructionsFromMixinSources);

return _.values(mixins);
}

function findMixinDefinitions(appRootDir, sourceDirs, extensions) {
var files = [];
sourceDirs.forEach(function(dir) {
dir = tryResolveAppPath(appRootDir, dir);
if (!dir) {
debug('Skipping unknown module source dir %j', dir);
return;
}
files = files.concat(findScripts(dir, extensions));
});
return files;
}

var mixins = files.map(function(filepath) {
function loadMixins(sourceFiles, options) {
var mixinInstructions = {};
sourceFiles.forEach(function(filepath) {
var dir = path.dirname(filepath);
var ext = path.extname(filepath);
var name = path.basename(filepath, ext);
Expand All @@ -634,10 +675,31 @@ function buildAllMixinInstructions(appRootDir, mixinDirs, options) {
_.extend(meta, require(metafile));
}
meta.sourceFile = filepath;
return meta;
mixinInstructions[meta.name] = meta;
});

return mixins;
return mixinInstructions;
}

function fetchMixinNamesUsedInModelInstructions(modelInstructions) {
return _.flatten(modelInstructions
.map(function(model) {
return model.definition && model.definition.mixins ?
Object.keys(model.definition.mixins) : [];
}));
}

function filterMixinInstructionsUsingWhitelist(instructions, includeMixins) {
var instructionKeys = Object.keys(instructions);
includeMixins = _.intersection(instructionKeys, includeMixins);

var filteredInstructions = {};
instructionKeys.forEach(function(mixinName) {
if (includeMixins.indexOf(mixinName) !== -1) {
filteredInstructions[mixinName] = instructions[mixinName];
}
});
return filteredInstructions;
}

function normalizeMixinName(str, options) {
Expand Down
3 changes: 1 addition & 2 deletions test/browser.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ describe('browser support', function() {
it('loads mixins', function(done) {
var appDir = path.resolve(__dirname, './fixtures/browser-app');
var options = {
appRootDir: appDir,
mixinDirs: ['./mixins']
appRootDir: appDir
};

browserifyTestApp(options, function(err, bundlePath) {
Expand Down
187 changes: 160 additions & 27 deletions test/compiler.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,41 +1016,175 @@ describe('compiler', function() {
});

describe('for mixins', function() {
function verifyMixinIsFoundViaMixinDirs(sourceFile, mixinDirs) {
var appJS = appdir.writeFileSync(sourceFile, '');
describe(' - mixinDirs', function() {
function verifyMixinIsFoundViaMixinDirs(sourceFile, mixinDirs) {
var appJS = appdir.writeFileSync(sourceFile, '');

var instructions = boot.compile({
appRootDir: appdir.PATH,
mixinDirs: mixinDirs
var instructions = boot.compile({
appRootDir: appdir.PATH,
mixinDirs: mixinDirs
});

expect(instructions.mixins[0].sourceFile).to.eql(appJS);
}

it('supports `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('custom-mixins/other.js',
['./custom-mixins']);
});

Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason why vehicle.js was renamed to other.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not fitting the 80 character limit. So, instead of moving custom-mixins/vehicle.js to new line, I renamed vehicle.js to other.js

Copy link
Member

Choose a reason for hiding this comment

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

Haha, fair enough :)

expect(instructions.mixins[0].sourceFile).to.eql(appJS);
}
it('resolves relative path in `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('custom-mixins/other.js',
['./custom-mixins']);
});

it('supports `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('mixins/other.js', ['./mixins']);
it('resolves module relative path in `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('node_modules/custom-mixins/other.js',
['custom-mixins']);
});
});

it('resolves relative path in `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('custom-mixins/vehicle.js',
['./custom-mixins']);
});
describe(' - mixinSources', function() {
beforeEach(function() {
appdir.createConfigFilesSync({}, {}, {
Car: { dataSource: 'db' }
});
appdir.writeConfigFileSync('models/car.json', {
name: 'Car',
mixins: {'TimeStamps': {} }
});
});

function verifyMixinIsFoundViaMixinSources(sourceFile, mixinSources) {
var appJS = appdir.writeFileSync(sourceFile, '');

var instructions = boot.compile({
appRootDir: appdir.PATH,
mixinSources: mixinSources
});

expect(instructions.mixins[0].sourceFile).to.eql(appJS);
Copy link
Member

Choose a reason for hiding this comment

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

If my understanding is correct, then this function should always fail if the compiler implementation works correctly. The mixin instruction should be included only when there is a model using the mixin. Since there are no models defined here, the compiler should not load any mixins from mixinSources.

Am I missing something?

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, you are right!
My code to filter mixinSources is currently

if (includeMixins.length > 0) {
   ...
}

The condition is not needed. I'll fix this!

}

it('supports `mixinSources` option', function() {
verifyMixinIsFoundViaMixinSources('mixins/time-stamps.js',
['./mixins']);
});

it('resolves relative path in `mixinSources` option', function() {
verifyMixinIsFoundViaMixinSources('custom-mixins/time-stamps.js',
['./custom-mixins']);
});

it('resolves module relative path in `mixinSources` option',
function() {
verifyMixinIsFoundViaMixinSources(
'node_modules/custom-mixins/time-stamps.js',
['custom-mixins']);
});

it('supports `mixins` option in `model-config.json`', function() {
appdir.createConfigFilesSync({}, {}, {
_meta: {
mixins: ['./custom-mixins']
},
Car: {
dataSource: 'db'
}
});

var appJS = appdir.writeFileSync('custom-mixins/time-stamps.js', '');
var instructions = boot.compile(appdir.PATH);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('resolves module relative path in `mixinDirs` option', function() {
verifyMixinIsFoundViaMixinDirs('node_modules/custom-mixins/vehicle.js',
['custom-mixins']);
it('sets by default `mixinSources` to `mixins` directory', function() {
var appJS = appdir.writeFileSync('mixins/time-stamps.js', '');
var instructions = boot.compile(appdir.PATH);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('loads only mixins used by models', function() {
var appJS = appdir.writeFileSync('mixins/time-stamps.js', '');
appdir.writeFileSync('mixins/foo.js', '');

var instructions = boot.compile(appdir.PATH);
expect(instructions.mixins).to.have.length(1);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('loads mixins from model using mixin name in JSON file', function() {
var appJS = appdir.writeFileSync('mixins/time-stamps.js', '');
appdir.writeConfigFileSync('mixins/time-stamps.json', {
name: 'Timestamping'
});

appdir.writeConfigFileSync('models/car.json', {
name: 'Car',
mixins: {'Timestamping': {} }
});

var instructions = boot.compile(appdir.PATH);
expect(instructions.mixins).to.have.length(1);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('loads mixin only once for dirs common to mixinDirs & mixinSources',
function() {
var appJS = appdir.writeFileSync('custom-mixins/time-stamps.js', '');

var options = {
appRootDir: appdir.PATH,
mixinDirs: ['./custom-mixins'],
mixinSources: ['./custom-mixins']
};

var instructions = boot.compile(options);
expect(instructions.mixins).to.have.length(1);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('loads mixin from mixinSources, when it is also found in mixinDirs',
function() {
appdir.writeFileSync('mixinDir/time-stamps.js', '');
var appJS = appdir.writeFileSync('mixinSource/time-stamps.js', '');

var options = {
appRootDir: appdir.PATH,
mixinDirs: ['./mixinDir'],
mixinSources: ['./mixinSource']
};

var instructions = boot.compile(options);
expect(instructions.mixins).to.have.length(1);
expect(instructions.mixins[0].sourceFile).to.eql(appJS);
});

it('loads mixin from the most recent mixin definition', function() {
appdir.writeFileSync('mixins1/time-stamps.js', '');
var mixins2 = appdir.writeFileSync('mixins2/time-stamps.js', '');

var options = {
appRootDir: appdir.PATH,
mixinSources: ['./mixins1', './mixins2']
};

var instructions = boot.compile(options);
expect(instructions.mixins).to.have.length(1);
expect(instructions.mixins[0].sourceFile).to.eql(mixins2);
});
});

describe('name normalization', function() {
var options;
beforeEach(function() {
options = { appRootDir: appdir.PATH, mixinDirs: ['./mixins'] };
options = { appRootDir: appdir.PATH, mixinDirs: ['./custom-mixins'] };

appdir.writeFileSync('mixins/foo.js', '');
appdir.writeFileSync('mixins/time-stamps.js', '');
appdir.writeFileSync('mixins/camelCase.js', '');
appdir.writeFileSync('mixins/PascalCase.js', '');
appdir.writeFileSync('mixins/space name.js', '');
appdir.writeFileSync('custom-mixins/foo.js', '');
appdir.writeFileSync('custom-mixins/time-stamps.js', '');
appdir.writeFileSync('custom-mixins/camelCase.js', '');
appdir.writeFileSync('custom-mixins/PascalCase.js', '');
appdir.writeFileSync('custom-mixins/space name.js', '');
});

it('supports classify', function() {
Expand Down Expand Up @@ -1146,15 +1280,15 @@ describe('compiler', function() {
});

it('extends definition from JSON with same file name', function() {
var appJS = appdir.writeFileSync('mixins/foo-bar.js', '');
var appJS = appdir.writeFileSync('custom-mixins/foo-bar.js', '');

appdir.writeConfigFileSync('mixins/foo-bar.json', {
appdir.writeConfigFileSync('custom-mixins/foo-bar.json', {
description: 'JSON file name same as JS file name' });
appdir.writeConfigFileSync('mixins/FooBar.json', {
appdir.writeConfigFileSync('custom-mixins/FooBar.json', {
description: 'JSON file name same as normalized name of mixin' });

var options = { appRootDir: appdir.PATH,
mixinDirs: ['./mixins'],
mixinDirs: ['./custom-mixins'],
normalization: 'classify' };
var instructions = boot.compile(options);

Expand All @@ -1166,7 +1300,6 @@ describe('compiler', function() {
}
]);
});

});
});

Expand Down
25 changes: 18 additions & 7 deletions test/executor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,28 +293,39 @@ describe('executor', function() {
});

describe ('for mixins', function() {
it('defines mixins from instructions', function() {
appdir.writeFileSync('mixins/example.js',
var options;
beforeEach(function() {
appdir.writeFileSync('custom-mixins/example.js',
'module.exports = ' +
'function(Model, options) {}');

appdir.writeFileSync('mixins/time-stamps.js',
appdir.writeFileSync('custom-mixins/time-stamps.js',
'module.exports = ' +
'function(Model, options) {}');

appdir.writeConfigFileSync('mixins/time-stamps.json', {
appdir.writeConfigFileSync('custom-mixins/time-stamps.json', {
name: 'Timestamping'
});

var options = {
appRootDir: appdir.PATH,
mixinDirs: ['./mixins']
options = {
appRootDir: appdir.PATH
};
});

it('defines mixins from instructions - using `mixinDirs`', function() {
options.mixinDirs = ['./custom-mixins'];
boot(app, options);

var modelBuilder = app.registry.modelBuilder;
var registry = modelBuilder.mixins.mixins;
expect(Object.keys(registry)).to.eql(['Example', 'Timestamping']);
});

it('defines mixins from instructions - using `mixinSources`', function() {
options.mixinSources = ['./custom-mixins'];
boot(app, options);

var modelBuilder = app.registry.modelBuilder;
var registry = modelBuilder.mixins.mixins;
expect(Object.keys(registry)).to.eql(['Example', 'Timestamping']);
});
Expand Down