Skip to content
Open
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
43 changes: 38 additions & 5 deletions src/helpers.js → src/helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const os = require('os');
const prettier = require('prettier');
const chalk = require('chalk');

const { requireOptional } = require('./utils');
const { requireOptional } = require('../utils');


// Get the configuration for this component.
Expand All @@ -30,6 +30,7 @@ module.exports.getConfig = () => {

const defaults = {
type: 'class',
styling: 'none',
dir: 'src/components',
extension: 'js'
};
Expand All @@ -46,6 +47,24 @@ module.exports.buildPrettifier = prettierConfig => text => (
prettier.format(text, prettierConfig)
);

const componentTypes = {
FUNCTIONAL: 'functional',
CLASS: 'class',
PURE_CLASS: 'pure-class',
};
module.exports.componentTypes = componentTypes;

const stylingTypes = {
STYLED: 'styled-components',
CSS_MODULES: 'css-modules',
APHRODITE: 'aphrodite',
};
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should consider none a valid styling type.

My reasoning is that the default styling is no styling. Someone may install this project, skim the README too quickly, and not even realize that it's capable of doing styling!

By showing that "None" is the styling option they've selected, we also show the options they haven't selected. They might say "oh cool this does styled-components! I need that".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK! When you said you want to keep the project as simple as possible, I thought showing the styling: none styled css-modules aphrodite in console every time would confuse beginner users.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, my fault for not specifying! I can see the argument of wanting to reduce the noise as much as possible, but I think that ultimately it's worth the added line of info :)

module.exports.stylingTypes = stylingTypes;
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: I think we can just do module.exports.componentTypes = { ... }. Same for stylingTypes.

My reasoning is that I'm so used to ES6 exports... the fact that it's const stylingTypes and not export const stylingTypes made me think it wasn't exported... so having it connected directly to module.exports = would make it easier to tell at a glance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reasoning is that I'm so used to ES6 exports
Me too.

They were like what you said, but later I discovered we'll need these variables here for example. Can we reference these by modules.exports.componentTypes?

Copy link
Owner

@joshwcomeau joshwcomeau Oct 11, 2017

Choose a reason for hiding this comment

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

Ohh, I see. Didn't realize that. Yeah, I'd leave this as-is then :)

EDIT: For what it's worth, you can reference them that way. but it feels a little funny to me.


module.exports.hasCssFile = styling => {
return styling === module.exports.stylingTypes.CSS_MODULES;
}

// Emit a message confirming the creation of the component
const colors = {
red: [216, 16, 16],
Expand All @@ -57,7 +76,17 @@ const colors = {
};

const logComponentType = (selected) => (
['class', 'pure-class', 'functional']
Object.values(componentTypes)
.sort((a, b) => a === selected ? -1 : 1)
.map(option => (
option === selected
? `${chalk.bold.rgb(...colors.blue)(option)}`
: `${chalk.rgb(...colors.darkGray)(option)}`
)).join(' ')
);

const logStylingType = (selected) => (
Object.values(stylingTypes)
.sort((a, b) => a === selected ? -1 : 1)
.map(option => (
option === selected
Expand All @@ -66,17 +95,21 @@ const logComponentType = (selected) => (
)).join(' ')
);
Copy link
Owner

Choose a reason for hiding this comment

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

I think there's enough duplication that an abstraction could come in handy here :)

const logSelected = (values, selected) => (
  Object.values(values)
    .sort((a, b) => a === selected ? -1 : 1)
    .map(option => (
      option === selected
        ? `${chalk.bold.rgb(...colors.blue)(option)}`
        : `${chalk.rgb(...colors.darkGray)(option)}`
    )).join('  ')
);

Then you could just use it:

const componentTypeString = logSelected(componentTypes, type);
const stylingTypeString = logSelected(stylingTypes, styling);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good refactoring tip. I'll do better with refactoring later on.


module.exports.logIntro = ({ name, dir, type }) => {
module.exports.logIntro = ({ name, dir, type, styling }) => {
console.info('\n');
console.info(`✨ Creating the ${chalk.bold.rgb(...colors.gold)(name)} component ✨`);
console.info('\n');


const pathString = chalk.bold.rgb(...colors.blue)(dir);
const typeString = logComponentType(type);
const componentTypeString = logComponentType(type);
const stylingTypeString = logStylingType(styling);

console.info(`Directory: ${pathString}`);
console.info(`Type: ${typeString}`);
console.info(`Type: ${componentTypeString}`);
if (styling && styling !== 'none') {
Copy link
Owner

Choose a reason for hiding this comment

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

I talked more about this above, but yeah I think we should log the styling even when "none" is selected :)

console.info(`Styling: ${stylingTypeString}`);
}
console.info(chalk.rgb(...colors.darkGray)('========================================='));

console.info('\n');
Expand Down
23 changes: 23 additions & 0 deletions src/helpers/templateConstructor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const reactImportTemplates = require('../templates/react-imports');
const styleImportTemplates = require('../templates/style-imports');
const renderContentTemplates = require('../templates/render-contents');
const componentDefinitionTemplates = require('../templates/component-definition');
const belowComponentTemplates = require('../templates/below-component');
const exportsTemplates = require('../templates/exports');

module.exports = (componentName, componentType, style) => {
const renderContents = renderContentTemplates(style);
const template = `
${reactImportTemplates(componentType)}
${styleImportTemplates(style, componentName)}

${componentDefinitionTemplates(componentType, componentName, renderContents)}

${belowComponentTemplates(style)}

${exportsTemplates(style, componentName)}
`;
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: So with the templates (even the ones I wrote), we don't indent the template string contents:

const template = `
stuff
`;

Initially this was because I didn't want the exported file having a bunch of leading whitespace... but now that I think about it, I think prettier would trim all that, so it wouldn't be a problem.

In this case though, because it's already indented by the module.exports function, I think it's fine to indent:

module.exports = (componentName, componentType, style) => {
  const template = `
    stuff
  `;
}

No change necessary for the other template strings though (unless you think it'd be better; I'm happy either way with the stuff in templates/component-definition.js)


return template;
}

57 changes: 37 additions & 20 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ const {
logItemCompletion,
logConclusion,
logError,
hasCssFile,
} = require('./helpers');
const {
requireOptional,
mkDirPromise,
readFilePromiseRelative,
writeFilePromise,
} = require('./utils');

const generateTemplate = require('./helpers/templateConstructor');
const cssTemplate = require('./templates/css');

// Load our package.json, so that we can pass the version onto `commander`.
const { version } = require('../package.json');
Expand All @@ -30,6 +32,9 @@ const config = getConfig();
// Convenience wrapper around Prettier, so that config doesn't have to be
// passed every time.
const prettify = buildPrettifier(config.prettierConfig);
const prettifyCss = buildPrettifier(Object.assign({}, config.prettierConfig, {
parser: 'postcss'
}));
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome! Thanks so much for prettifying the CSS, had no idea Prettier could do that 🔥 🔥 🔥



program
Expand All @@ -40,6 +45,11 @@ program
'Type of React component to generate (default: "class")',
/^(class|pure-class|functional)$/i,
config.type
).option(
'-s, --styling <stylingSolution>',
'Which styling solution to generate for (default: "none")',
Copy link
Owner

Choose a reason for hiding this comment

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

Copy nit: Can we make this Styling tool to use for the component (default: "none")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! You know English better than me for SURE :)

/^(none|styled-components|css-modules|aphrodite)$/i,
config.styling
).option(
'-d, --dir <pathToDirectory>',
'Path to the "components" directory (default: "src/components")',
Expand All @@ -52,20 +62,23 @@ program

const [componentName] = program.args;

// Find the path to the selected template file.
const templatePath = `./templates/${program.type}.js`;

// Get all of our file paths worked out, for the user's project.
const componentDir = `${program.dir}/${componentName}`;
const filePath = `${componentDir}/${componentName}.${program.extension}`;
const componentFilePath = `${componentDir}/${componentName}.${program.extension}`;
const cssFilePath = `${componentDir}/${componentName}.${'css'}`;
const indexPath = `${componentDir}/index.js`;

// Our index template is super straightforward, so we'll just inline it for now.
const indexTemplate = prettify(`\
const indexTemplate = `\
export { default } from './${componentName}';
`);
`;

logIntro({ name: componentName, dir: componentDir, type: program.type });
logIntro({
name: componentName,
dir: componentDir,
type: program.type,
styling: program.styling,
});


// Check if componentName is provided
Expand All @@ -91,33 +104,37 @@ if (fs.existsSync(fullPathToComponentDir)) {
// Start by creating the directory that our component lives in.
mkDirPromise(componentDir)
.then(() => (
readFilePromiseRelative(templatePath)
generateTemplate(componentName, program.type, program.styling)
))
.then(template => {
logItemCompletion('Directory created.');
return template;
})
.then(template => (
// Replace our placeholders with real data (so far, just the component name)
template.replace(/COMPONENT_NAME/g, componentName)
))
.then(template => (
// Format it using prettier, to ensure style consistency, and write to file.
writeFilePromise(filePath, prettify(template))
writeFilePromise(componentFilePath, prettify(template))
))
.then(template => {
.then(() => {
// Check whether we should make a CSS file too
if (hasCssFile(program.styling)) {
// Format it using prettier, to ensure style consistency, and write to file.
return writeFilePromise(cssFilePath, prettifyCss(cssTemplate))
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: I think it'd be better to move the line that defines cssFilePath (line 68) to within this if statement.

Just because it isn't used most of the time, so it feels a little cleaner to not have this unused constant defined for every execution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it should be like what you said! But I thought maybe you'll like to put it beside other paths variables :) Sorry!

.then(() => {
logItemCompletion('CSS file built and saved to disk.');
});
}
})
.then(() => {
logItemCompletion('Component built and saved to disk.');
return template;
})
.then(template => (
.then(() => (
// We also need the `index.js` file, which allows easy importing.
writeFilePromise(indexPath, prettify(indexTemplate))
))
.then(template => {
.then(() => {
logItemCompletion('Index file built and saved to disk.');
return template;
})
.then(template => {
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch! Didn't realize I was pointlessly passing the template down :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was confused for a few moments why we're doing this :))

.then(() => {
logConclusion();
})
.catch(err => {
Expand Down
26 changes: 26 additions & 0 deletions src/templates/below-component.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const { stylingTypes } = require('../helpers');

const styled = `\
const Wrapper = styled.div\`
\`;
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: can you add a blank line inside the template string?

Will Prettier strip it out, if you do? I like the idea of having a space for someone to add their styles, but it's not a big deal :)

Copy link
Collaborator Author

@morajabi morajabi Sep 23, 2017

Choose a reason for hiding this comment

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

Will Prettier strip it out, if you do?

Yeah, Prettier will make it like so:

const Wrapper = styled.div``

Copy link
Owner

Choose a reason for hiding this comment

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

ok! No problem then :)

`;

const aphrodite = `\
const styles = StyleSheet.create({
wrapper: {
},
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Same note as above; if possible with Prettier, can we add a blank line between open and close curlies?

Copy link
Collaborator Author

@morajabi morajabi Sep 23, 2017

Choose a reason for hiding this comment

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

I think as Prettier introduces itself as an opinionated code formatter, it shouldn't be possible :) (Also I couldn't find an option to avoid it here)

But maybe we can write a placeholder style for wrapper as create-react-app does that when you first init a project. I mean something like this:

{
  wrapper: {
    height: 'auto',
    width: 'auto',
  }
}

What do you think? Happy to discuss!

Copy link
Owner

Choose a reason for hiding this comment

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

Its ok :) no change necessary, I'd rather not apply any styles, and it's super easy for a developer to just add that new line themselves when they want to add styles.

});
`;

module.exports = (style, componentType) => {
switch (style) {
case stylingTypes.STYLED:
return styled;

case stylingTypes.APHRODITE:
return aphrodite;

default:
return '';
}
}
10 changes: 0 additions & 10 deletions src/templates/class.js

This file was deleted.

36 changes: 36 additions & 0 deletions src/templates/component-definition.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
const { componentTypes } = require('../helpers');

const functional = (ComponentName, renderContents) => (`
const ${ComponentName} = () => {
return ${renderContents};
};
`)

const classComp = (ComponentName, renderContents) => (`
class ${ComponentName} extends Component {
render() {
return ${renderContents};
}
}
`)

const pureClass = (ComponentName, renderContents) => (`
class ${ComponentName} extends PureComponent {
render() {
return ${renderContents};
}
}
`)

module.exports = (componentType, ...args) => {
switch (componentType) {
case componentTypes.FUNCTIONAL:
return functional(...args);

case componentTypes.CLASS:
return classComp(...args);

case componentTypes.PURE_CLASS:
return pureClass(...args);
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Great method! Good use of rest/spread :)

4 changes: 4 additions & 0 deletions src/templates/css.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module.exports = `
.wrapper {
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: same note as above, add a blank line if possible

`;
8 changes: 8 additions & 0 deletions src/templates/exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const { stylingTypes } = require('../helpers');

module.exports = (style, componentName) => {
switch (style) {
default:
return `export default ${componentName};`;
}
}
8 changes: 0 additions & 8 deletions src/templates/functional.js

This file was deleted.

10 changes: 0 additions & 10 deletions src/templates/pure-class.js

This file was deleted.

14 changes: 14 additions & 0 deletions src/templates/react-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const { componentTypes } = require('../helpers');

module.exports = (componentType) => {
switch (componentType) {
case componentTypes.FUNCTIONAL:
return "import React from 'react';";

case componentTypes.CLASS:
return "import React, { Component } from 'react';";

case componentTypes.PURE_CLASS:
return "import React, { PureComponent } from 'react';";
}
}
17 changes: 17 additions & 0 deletions src/templates/render-contents.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const { stylingTypes } = require('../helpers');

module.exports = (style) => {
switch (style) {
case stylingTypes.STYLED:
return '<Wrapper />';

case stylingTypes.CSS_MODULES:
return '<div className={styles.wrapper} />';

case stylingTypes.APHRODITE:
return '<div className={css(styles.wrapper)} />';

default:
return '<div />';
}
}
17 changes: 17 additions & 0 deletions src/templates/style-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const { stylingTypes } = require('../helpers');

module.exports = (style, componentName) => {
switch (style) {
case stylingTypes.STYLED:
return "import styled from 'styled-components';";

case stylingTypes.CSS_MODULES:
return `import styles from './${componentName}.css';`;

case stylingTypes.APHRODITE:
return "import { StyleSheet, css } from 'aphrodite';";

default:
return '';
}
Copy link
Owner

Choose a reason for hiding this comment

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

If we add none as a valid styling type, I would just do this:

case stylingTypes.NONE:
default:
  return '';

By using the fallthrough we can make none do the default thing, but we're being explicit about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. This (the fallthrough) was the reason I suggested using switch statements, neat!

}