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.

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

[GPT-565] Dynamic support to oc-templates in oc dev #426

Merged
merged 10 commits into from
Apr 5, 2017
8 changes: 7 additions & 1 deletion client/src/get-compiled-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ module.exports = function(cache){
if (type === 'jade') { type = 'oc-template-jade'; }
if (type === 'handlebars') { type = 'oc-template-handlebars'; }

var ocTemplate = requireTemplate(type);
var ocTemplate;
try {
ocTemplate = requireTemplate(type);
} catch (err) {
return callback(err);
}

cb(null, ocTemplate.getCompiledTemplate(templateText, template.key));
});
};
Expand Down
3 changes: 2 additions & 1 deletion client/src/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ module.exports = {
registriesEmpty: 'registries must contain at least one endpoint',
registriesIsNotObject: 'registries must be an object',
serverSideRenderingFail: 'Server-side rendering failed: {0}',
warmupFailed: 'Error warming up oc-client: request {0} failed ({1})'
warmupFailed: 'Error warming up oc-client: request {0} failed ({1})',
gettingTemplateFailed: 'Error requiring template {0}'
};
8 changes: 7 additions & 1 deletion client/src/template-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@ module.exports = function(){
if (type === 'jade') { type = 'oc-template-jade'; }
if (type === 'handlebars') { type = 'oc-template-handlebars'; }

var ocTemplate = requireTemplate(type);
var ocTemplate;
try {
ocTemplate = requireTemplate(type);
} catch (err) {
return callback(err);
}

ocTemplate.render(
{
template,
Expand Down
56 changes: 50 additions & 6 deletions client/src/utils/require-template.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,56 @@
'use strict';

var format = require('stringformat');
var templateNotSupported = 'Error loading component: template "{0}" not supported';
var path = require('path');
var _ = require('underscore');
Copy link
Member

Choose a reason for hiding this comment

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

we don't have underscore in the client! Made the same mistake when reviewing the previous PR with @mattiaerre and then realised - are you able to go vanilla with client's code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, actually I like it even more without it.


module.exports = function(type) {
var templateNotFound = 'Error requiring oc-template: "{0}" not found';
var templateNotValid = 'Error requiring oc-template: "{0}" is not a valid oc-template';

function isValidTemplate(template){
if (typeof template !== 'object') {
return false;
}

var apiMethods = _.keys(template);
if (apiMethods.length !== 4) {
return false;
}

return _.contains(
apiMethods,
'getInfo',
'getCompiledTemplate',
'compile',
'render'
);
}


module.exports = function(template) {
var ocTemplate;
var localTemplate = path.join(__dirname, '../../', 'node_modules', template);
var relativeTemplate = path.resolve('.', 'node_modules', template);

try {
return require(type);
} catch (err) {
throw format(templateNotSupported, type);
if (require.cache && !!require.cache[localTemplate]) {
delete require.cache[localTemplate];
}
ocTemplate = require(localTemplate);
} catch(err) {
try {
if (require.cache && !!require.cache[relativeTemplate]) {
delete require.cache[relativeTemplate];
}
ocTemplate = require(relativeTemplate);
} catch (err) {
throw format(templateNotFound, template);
}
}
};

if (!isValidTemplate(ocTemplate)) {
throw format(templateNotValid, template);
}

return ocTemplate;
};
42 changes: 30 additions & 12 deletions src/cli/domain/get-components-deps.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,48 @@
var fs = require('fs-extra');
var path = require('path');
var _ = require('underscore');
var format = require('stringformat');
var settings = require('../../resources');

module.exports = function(components){
var deps = { modules: {}, withVersions: {}, templates: {} };

var deps = { modules: [], withVersions: [] };

_.forEach(components, function(c){
var legacyTemplates = {
'jade': true,
'handlebars': true
};

components.forEach(function(c){
var pkg = fs.readJsonSync(path.join(c, 'package.json'));
var type = pkg.oc.files.template.type;
var dependencies = pkg.dependencies;

_.forEach(_.keys(pkg.dependencies), function(d){
if (!deps.templates[type] && !legacyTemplates[type]) {
Copy link
Member

Choose a reason for hiding this comment

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

I think

var legacyTemplates = ['jade', 'handlebars']
...
if (!deps.templates[type] && !_.includes(legacyTemplates, type)) {

would be a bit more readable?

Copy link
Contributor Author

@nickbalestra nickbalestra Mar 31, 2017

Choose a reason for hiding this comment

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

very nit and very arguable weather an array of 2 elements is more readable then an object with 2 keys.. I've removed the usage of _.include when reformatting this, we were doing way too many useless array loops, so, my rationale is: quicklookups ftw

Copy link
Member

Choose a reason for hiding this comment

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

That's why I put it as question. I don't have strong opinions on this. Let's leave it as is if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add check that if templatetype is not legacy must be declared as dependency

if (!dependencies[type]) {
throw new Error(format(settings.errors.cli.TEMPLATE_DEP_MISSING, type));
}
deps.templates[type] = true;
}

var version = pkg.dependencies[d],
hasVersion = !_.isEmpty(version),
depToInstall = hasVersion ? (d + '@' + version) : d;
_.keys(dependencies).forEach(function(name){
var version = dependencies[name];
var depToInstall = version.length > 0
? (name + '@' + version)
: name;

if(!_.contains(deps.withVersions, depToInstall)){
deps.withVersions.push(depToInstall);
if (!deps.withVersions[depToInstall]) {
deps.withVersions[depToInstall] = true;
}

if(!_.contains(deps.modules, d)){
deps.modules.push(d);
if (!deps.modules[name]) {
deps.modules[name] = true;
}
});
});

return deps;
return {
modules: _.keys(deps.modules),
withVersions: _.keys(deps.withVersions),
templates: _.keys(deps.templates)
};
};
9 changes: 8 additions & 1 deletion src/cli/domain/package-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,18 @@ var javaScriptizeTemplate = function(functionName, data){

var compileView = function(viewPath, type, cb) {
var template = fs.readFileSync(viewPath).toString();
var ocTemplate;

if (type === 'jade') { type = 'oc-template-jade'; }
if (type === 'handlebars') { type = 'oc-template-handlebars'; }


try {
ocTemplate = requireTemplate(type);
} catch (err) {
return cb(err);
}

var ocTemplate = requireTemplate(type);
ocTemplate.compile({ template, viewPath }, function(err, compiledView){
if (err) { return cb(err);}

Expand Down
12 changes: 6 additions & 6 deletions src/cli/facade/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,13 @@ module.exports = function(dependencies){

var loadDependencies = function(components, cb){
log.warn(strings.messages.cli.CHECKING_DEPENDENCIES, true);

var dependencies = getComponentsDependencies(components),
missing = getMissingDeps(dependencies.withVersions, components);

if(_.isEmpty(missing)){
log.ok('OK');
return cb(dependencies.modules);
return cb(dependencies);
}

log.err('FAIL');
Expand Down Expand Up @@ -152,11 +152,11 @@ module.exports = function(dependencies){
log.ok('OK');
_.forEach(components, function(component){
logger.log(colors.green('├── ') + component);
});
});

loadDependencies(components, function(dependencies){
packageComponents(components, function(){

var registry = new oc.Registry({
local: true,
hotReloading: hotReloading,
Expand All @@ -167,8 +167,8 @@ module.exports = function(dependencies){
port: port,
baseUrl: baseUrl,
env: { name: 'local' },
dependencies: dependencies,
templates: []
dependencies: dependencies.modules,
templates: dependencies.templates
});

registerPlugins(registry);
Expand Down
21 changes: 13 additions & 8 deletions src/registry/domain/repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ var settings = require('../../resources/settings');
var strings = require('../../resources');
var validator = require('./validators');
var versionHandler = require('./version-handler');
var requireTemplate = require('../../utils/require-template');

module.exports = function(conf){

Expand All @@ -26,17 +27,21 @@ module.exports = function(conf){
var coreTemplates = ['oc-template-jade', 'oc-template-handlebars'];
var templates = _.union(coreTemplates, conf.templates)
.map(function(template){
var info;
try {
info = require(template).getInfo();
var ocTemplate = requireTemplate(template);
var info = ocTemplate.getInfo();
return {
type: info.type,
version: info.version,
externals: info.externals
};
} catch (err) {
throw new Error(format(strings.errors.registry.TEMPLATE_NOT_FOUND, template));
throw err;
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's re-add a try catch here for a template that, in case is not valid, may throw as getInfo doesn't exist or is not a function. Error wouldn't be "template not found" anymore but more of a "template not valid" kind?

return {
type: info.type,
version: info.version,
externals: info.externals
};




});

var local = {
Expand Down
9 changes: 7 additions & 2 deletions src/registry/routes/helpers/get-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,17 @@ module.exports = function(conf, repository){
returnResult(cached);
} else {
repository.getCompiledView(component.name, component.version, function(err, templateText){

var ocTemplate;
var type = component.oc.files.template.type;
if (type === 'jade') { type = 'oc-template-jade'; }
if (type === 'handlebars') { type = 'oc-template-handlebars'; }

var ocTemplate = requireTemplate(type);
try {
ocTemplate = requireTemplate(type);
} catch (err) {
throw err;
}

var template = ocTemplate.getCompiledTemplate(templateText, key);
cache.set('file-contents', cacheKey, template);
returnResult(template);
Expand Down
6 changes: 4 additions & 2 deletions src/resources/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ module.exports = {
PLUGIN_NOT_IMPLEMENTED: 'registry does not implement plugins: {0}',
PLUGIN_NOT_VALID: 'Plugin {0} is not valid',
RESOLVING_ERROR: 'component resolving error',
TEMPLATE_NOT_FOUND: 'Template {0} not found'
TEMPLATE_NOT_FOUND: 'Template {0} not found',
TEMPLATE_NOT_VALID: '{0} is not a valid oc-template'
},
cli: {
COMPONENT_HREF_NOT_FOUND: 'The specified path is not a valid component\'s url',
Expand All @@ -83,7 +84,8 @@ module.exports = {
REGISTRY_NOT_FOUND: 'oc registries not found. Run "oc registry add <registry href>"',
SERVERJS_DEPENDENCY_NOT_DECLARED: 'Missing dependencies from package.json => {0}',
TEMPLATE_NOT_FOUND: 'file {0} not found',
TEMPLATE_TYPE_NOT_VALID: 'the template is not valid. Allowed values are handlebars and jade'
TEMPLATE_TYPE_NOT_VALID: 'the template is not valid. Allowed values are handlebars and jade',
TEMPLATE_DEP_MISSING: 'Template dependency missing. Run "$npm install --save {0}" to fix it.'
},
generic: 'An error occurred: {0}',
s3: {
Expand Down
56 changes: 50 additions & 6 deletions src/utils/require-template.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,56 @@
'use strict';

var format = require('stringformat');
var templateNotSupported = 'Error requiring the template "{0}": oc-template not found';
var path = require('path');
var _ = require('underscore');

module.exports = function(type) {
var templateNotFound = 'Error requiring oc-template: "{0}" not found';
var templateNotValid = 'Error requiring oc-template: "{0}" is not a valid oc-template';

function isValidTemplate(template){
if (typeof template !== 'object') {
return false;
}

var apiMethods = _.keys(template);
if (apiMethods.length !== 4) {
return false;
}

return _.contains(
apiMethods,
'getInfo',
'getCompiledTemplate',
'compile',
'render'
);
}

module.exports = function(template) {
var ocTemplate;
var localTemplate = path.join(__dirname, '../../', 'node_modules', template);
var relativeTemplate = path.resolve('.', 'node_modules', template);


try {
return require(type);
} catch (err) {
throw format(templateNotSupported, type);
if (require.cache && !!require.cache[localTemplate]) {
delete require.cache[localTemplate];
}
ocTemplate = require(localTemplate);
} catch(err) {
try {
if (require.cache && !!require.cache[relativeTemplate]) {
delete require.cache[relativeTemplate];
}
ocTemplate = require(relativeTemplate);
} catch (err) {
throw format(templateNotFound, template);
}
}

if (!isValidTemplate(ocTemplate)) {
throw format(templateNotValid, template);
}
};

return ocTemplate;
};
2 changes: 1 addition & 1 deletion test/unit/cli-domain-package-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('cli : domain : package-template', function(){
});

it('should show error', function(){
expect(error).to.equal('template.wha compilation failed - Error requiring the template "whazaaa": oc-template not found');
expect(error).to.equal('template.wha compilation failed - Error requiring oc-template: "whazaaa" not found');
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/registry-domain-repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('registry : domain : repository', function(){
try {
var repository = new Repository(conf);
} catch (err) {
expect(err).to.deep.equal(new Error('Template oc-template-react not found'));
expect(err).to.equal('Error requiring oc-template: "oc-template-react" not found');
}
});
});
Expand Down
Loading