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

Conversation

matteofigus
Copy link
Member

@matteofigus matteofigus changed the title [WIP][DX-185] install compiler inside each components' dir [DX-185] install compiler inside each components' dir Oct 25, 2017
@matteofigus matteofigus removed the wip label Oct 25, 2017
Copy link
Contributor

@nickbalestra nickbalestra left a comment

Choose a reason for hiding this comment

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

Nothing blocking.
I just left some comments/notes.

I think we should merge this. Great job on this!

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 ?

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

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?

'use strict';

const tryRequire = require('try-require');
const _ = require('lodash');
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess no need for lodash here?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@nickbalestra nickbalestra merged commit e79db46 into master Oct 31, 2017
@nickbalestra nickbalestra deleted the install-compiler branch October 31, 2017 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to package oc-template-react component
2 participants