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

[DX-185] install compiler inside each components' dir #710

Merged
merged 23 commits into from
Oct 31, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6690987
Rework init template part
matteofigus Oct 11, 2017
2bf6a21
Fix red test on windows CI
matteofigus Oct 11, 2017
e564ef5
First go on a clean rewrite of the dependency handler
matteofigus Oct 12, 2017
7ebdcc7
Moved things around, renamed things, cleanup
matteofigus Oct 12, 2017
6ccdb6e
Moved things around a bit more
matteofigus Oct 12, 2017
eb3e0e0
Add support for legacy templates
matteofigus Oct 13, 2017
5af1975
Moved things around again and again
matteofigus Oct 13, 2017
033f792
Moveed things aroound
matteofigus Oct 13, 2017
2261d07
Fix publish, moved things around and removed old files
matteofigus Oct 13, 2017
9a094bf
Merge branch 'master' of github.com:opentable/oc into install-compiler
matteofigus Oct 13, 2017
8de124b
Merge branch 'master' of github.com:opentable/oc into install-compiler
matteofigus Oct 18, 2017
a0f560a
require-template tests and cleanup
matteofigus Oct 18, 2017
00211e4
More cleanup, more tests
matteofigus Oct 18, 2017
4a8465d
More tests
matteofigus Oct 23, 2017
1f2bc22
Install compiler tests
matteofigus Oct 23, 2017
2d5b184
Install missing deps cleanup, fixes, and tests
matteofigus Oct 24, 2017
a23e539
More tests
matteofigus Oct 25, 2017
ac45083
Cleanup
matteofigus Oct 25, 2017
a059b76
Merge branch 'master' of github.com:opentable/oc into install-compiler
matteofigus Oct 25, 2017
0747c8d
Merge branch 'master' of github.com:opentable/oc into install-compiler
matteofigus Oct 25, 2017
5e0279a
Don't mess with package-lock.json
matteofigus Oct 25, 2017
41d8bcd
Merge branch 'master' of github.com:opentable/oc into install-compiler
matteofigus Oct 31, 2017
4ac814a
Cleanup
matteofigus Oct 31, 2017
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
55 changes: 0 additions & 55 deletions src/cli/domain/get-components-deps.js

This file was deleted.

22 changes: 0 additions & 22 deletions src/cli/domain/get-local-npm-modules.js

This file was deleted.

31 changes: 0 additions & 31 deletions src/cli/domain/get-missing-deps.js

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

const format = require('stringformat');

const strings = require('../../../resources');

module.exports = (options, cb) => {
const { componentPath, pkg, template } = options;
const compilerDep = `${template}-compiler`;
const isOk = pkg.devDependencies[compilerDep];

const err = isOk
? null
: format(strings.errors.cli.TEMPLATE_DEP_MISSING, template, componentPath);

cb(err, compilerDep);
};
31 changes: 31 additions & 0 deletions src/cli/domain/handle-dependencies/get-compiler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const path = require('path');

const cleanRequire = require('../../../utils/clean-require');
const installCompiler = require('./install-compiler');

module.exports = (options, cb) => {
const { compilerDep, componentPath, logger, pkg } = options;
const compilerPath = path.join(componentPath, 'node_modules', compilerDep);
const compiler = cleanRequire(compilerPath, { justTry: true });

if (compiler) {
return cb(null, compiler);
}

let dependency = compilerDep;
if (pkg.devDependencies[compilerDep]) {
dependency += `@${pkg.devDependencies[compilerDep]}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen in the case of someone having something like * or a relative path ?


const installOptions = {
compilerPath,
componentName: pkg.name,
componentPath,
dependency,
logger
};

installCompiler(installOptions, cb);
};
17 changes: 17 additions & 0 deletions src/cli/domain/handle-dependencies/get-missing-dependencies.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

const path = require('path');
const _ = require('lodash');

const cleanRequire = require('../../../utils/clean-require');

module.exports = dependencies => {
const missing = [];
_.each(dependencies, (version, dependency) => {
const pathToModule = path.resolve('node_modules/', dependency);
if (!cleanRequire(pathToModule, { justTry: true })) {
missing.push(`${dependency}@${version || 'latest'}`);
}
});
return missing;
};
80 changes: 80 additions & 0 deletions src/cli/domain/handle-dependencies/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
'use strict';

const async = require('async');
const coreModules = require('builtin-modules');
const fs = require('fs-extra');
const path = require('path');
const _ = require('lodash');

const ensureCompilerIsDeclaredAsDevDependency = require('./ensure-compiler-is-declared-as-devDependency');
const getCompiler = require('./get-compiler');
const installMissingDependencies = require('./install-missing-dependencies');
const isTemplateLegacy = require('./is-template-legacy');
const strings = require('../../../resources');

const getComponentPackageJson = (componentPath, cb) =>
fs.readJson(path.join(componentPath, 'package.json'), cb);

module.exports = (options, callback) => {
const { components, logger } = options;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the future: I guess we could start to use destructuring in the signature leve for cases like this.
ie: module.exports = ({ components, logger }, callback) => { ... as i see we do this quite often


const dependencies = {};
const addDependencies = componentDependencies =>
_.each(componentDependencies || {}, (version, dependency) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lodash each already support passing in undefined for the collection item

dependencies[dependency] = version;
});

const templates = {};
const addTemplate = (templateName, template) => {
templates[templateName] = template;
};

const setupComponentDependencies = (componentPath, done) =>
async.waterfall(
[
cb => getComponentPackageJson(componentPath, cb),
(pkg, cb) => {
addDependencies(pkg.dependencies);

const template = pkg.oc.files.template.type;
if (isTemplateLegacy(template)) {
return done();
}

cb(null, { componentPath, logger, pkg, template });
},

(options, cb) =>
ensureCompilerIsDeclaredAsDevDependency(options, (err, compilerDep) =>
cb(err, _.extend(options, { compilerDep }))
),

(options, cb) =>
getCompiler(options, (err, compiler) =>
cb(err, _.extend(options, { compiler }))
),

(options, cb) => {
const { compiler, template } = options;
addTemplate(template, compiler);
cb();
}
],
done
);

logger.warn(strings.messages.cli.CHECKING_DEPENDENCIES);
async.eachSeries(components, setupComponentDependencies, err => {
if (err) {
return callback(err);
}

const result = {
modules: _.union(coreModules, _.keys(dependencies)).sort(),
templates: _.values(templates)
};

const installOptions = { dependencies, logger };
installMissingDependencies(installOptions, err => callback(err, result));
});
};
35 changes: 35 additions & 0 deletions src/cli/domain/handle-dependencies/install-compiler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

const format = require('stringformat');

const cleanRequire = require('../../../utils/clean-require');
const isTemplateValid = require('../../../utils/is-template-valid');
const npm = require('../../../utils/npm-utils');
const strings = require('../../../resources/index');

module.exports = (options, cb) => {
const {
compilerPath,
componentName,
componentPath,
dependency,
logger
} = options;

logger.warn(format(strings.messages.cli.INSTALLING_DEPS, dependency), true);

const npmOptions = {
dependency,
installPath: componentPath,
save: false,
silent: true
};

npm.installDependency(npmOptions, err => {
err ? logger.err('FAIL') : logger.ok('OK');
const compiler = cleanRequire(compilerPath, { justTry: true });
const isOk = isTemplateValid(compiler);
const errorMsg = 'There was a problem while installing the compiler';
cb(!err && isOk ? null : errorMsg, compiler);
});
};
40 changes: 40 additions & 0 deletions src/cli/domain/handle-dependencies/install-missing-dependencies.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use strict';

const format = require('stringformat');
const path = require('path');
const _ = require('lodash');

const getMissingDependencies = require('./get-missing-dependencies');
const npm = require('../../../utils/npm-utils');
const strings = require('../../../resources/index');

module.exports = (options, callback) => {
const { dependencies, logger } = options;

const missing = getMissingDependencies(dependencies);

if (_.isEmpty(missing)) {
return callback(null);
}

logger.warn(
format(strings.messages.cli.INSTALLING_DEPS, missing.join(', ')),
true
);

const npmOptions = {
dependencies: missing,
installPath: path.resolve('.'),
save: false,
silent: true
};

npm.installDependencies(npmOptions, err => {
if (err || !_.isEmpty(getMissingDependencies(dependencies))) {
logger.fail('FAIL');
return callback(strings.errors.cli.DEPENDENCIES_INSTALL_FAIL);
}
logger.ok('OK');
callback(null);
});
};
3 changes: 3 additions & 0 deletions src/cli/domain/handle-dependencies/is-template-legacy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';

module.exports = template => !!{ jade: true, handlebars: true }[template];
51 changes: 51 additions & 0 deletions src/cli/domain/handle-dependencies/require-template.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

const format = require('stringformat');
const path = require('path');

const cleanRequire = require('../../../utils/clean-require');
const isTemplateLegacy = require('./is-template-legacy');
const isTemplateValid = require('../../../utils/is-template-valid');
const strings = require('../../../resources');

module.exports = function(template, options) {
const requireOptions = options || {};
let ocTemplate;

if (isTemplateLegacy(template)) {
template = `oc-template-${template}`;
}

if (requireOptions.compiler) {
template = `${template}-compiler`;
}

const localTemplate = path.join(__dirname, '../../node_modules', template);
const relativeTemplate = path.resolve('.', 'node_modules', template);
const componentRelativePath = path.join(
requireOptions.componentPath,
'node_modules',
template
);

[
componentRelativePath,
template,
localTemplate,
relativeTemplate
].forEach(pathToTry => {
ocTemplate = ocTemplate || cleanRequire(pathToTry, { justTry: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

why not using some instead of forEach so that you breakout of the loop as soon it get required? Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know about some. Perhaps let's do it in another PR as optimisation?

});

if (!ocTemplate) {
throw new Error(format(strings.errors.cli.TEMPLATE_NOT_FOUND, template));
}

if (!isTemplateValid(ocTemplate, requireOptions)) {
throw new Error(
format(strings.errors.cli.TEMPLATE_TYPE_NOT_VALID, template)
);
}

return ocTemplate;
};
Loading