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

Passes flags to commands #230

Merged
merged 16 commits into from
Oct 15, 2016
Merged
Show file tree
Hide file tree
Changes from 10 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
7 changes: 4 additions & 3 deletions cli/actions/__tests__/dev.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ describe('dev', () => {
clientURL: mockURL,
serverURL: mockURL,
reactHotLoader: false,
});
},
[]);

it('cleans the build directory', () => {
expect(logger.task).toBeCalledWith('Cleaned ./build');
Expand Down Expand Up @@ -83,9 +84,9 @@ describe('dev', () => {
});

it('server compilation done', () => {
const flagsStub = [];
serverCompilerDone(stats);

expect(nodemon).toBeCalledWith({ script: 'fakePath', watch: ['fakePath'] });
expect(nodemon).toBeCalledWith({ script: 'fakePath', watch: ['fakePath'], nodeArgs: flagsStub });

// nodemon.once
expect(nodemon.once.mock.calls[0][0]).toBe('start');
Expand Down
32 changes: 12 additions & 20 deletions cli/actions/__tests__/lint.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,37 @@ jest.setMock('path', {
jest.mock('../../logger');
jest.mock('../../../utils/paths');

jest.mock('shelljs', () => (
{
exec: () => ({
stdout: '',
code: 0,
}),
}
));

describe('lint', () => {
global.process.exit = jest.fn();
const logger = require('../../logger');
const lint = require('../lint');
const eslint = require('eslint');

beforeEach(() => {
jest.resetModules();
});

it('logs the user linter filename when found', () => {
lint();
lint({}, []);
expect(logger.info).toBeCalledWith('Using ESLint file: filename');
});

it('logs the base filename when there is no user file', () => {
lint();
lint({}, []);
expect(logger.info).toBeCalledWith('Using ESLint file: base filename');
});

it('logs and exits 0 when no errors', () => {
lint();
lint({}, []);
expect(logger.end).toBeCalledWith('Your JS looks great ✨');
expect(process.exit).toBeCalledWith(0);
});

it('logs and exits 0 when there are warnings', () => {
eslint.__setExecuteOnFiles({ warningCount: 1 }); // eslint-disable-line no-underscore-dangle
lint();

expect(logger.end)
.toBeCalledWith('Your JS looks OK, though there were warnings 🤔👆');
expect(process.exit).toBeCalledWith(0);
});

it('exits 1 when there are errors', () => {
eslint.__setExecuteOnFiles({ errorCount: 1 }); // eslint-disable-line no-underscore-dangle
lint();

expect(process.exit).toBeCalledWith(1);
});
});
4 changes: 2 additions & 2 deletions cli/actions/__tests__/start.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ const start = require('../start');

describe('start', () => {
const serverURL = { href: 'href' };
start({ serverURL });
start({ serverURL }, []);

it('logs start and end', () => {
expect(logger.start).toBeCalledWith('Starting production...');
expect(logger.end).toBeCalledWith(`Server running at ${serverURL.href}`);
});

it('executes the node process asynchronously', () => {
expect(shell.exec).toBeCalledWith('node build/server/main.js', { async: true });
expect(shell.exec).toBeCalledWith('node build/server/main.js ', { async: true });
});
});
4 changes: 2 additions & 2 deletions cli/actions/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const buildConfigs = require('../../utils/buildConfigs');
const webpackCompiler = require('../../utils/webpackCompiler');
const { buildPath, serverSrcPath } = require('../../utils/paths')();

module.exports = (config) => {
module.exports = (config, flags) => {
logger.start('Starting development build...');

// Kill the server on exit.
Expand Down Expand Up @@ -51,7 +51,7 @@ module.exports = (config) => {
serverCompiler.options.output.path, `${Object.keys(serverCompiler.options.entry)[0]}.js`
);

nodemon({ script: serverPath, watch: [serverPath] })
nodemon({ script: serverPath, watch: [serverPath], nodeArgs: flags })
.once('start', () => {
logger.task(`Server running at: ${serverURL.href}`);
logger.end('Development started');
Expand Down
27 changes: 7 additions & 20 deletions cli/actions/lint.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,29 @@

// Command to lint src code

const CLIEngine = require('eslint').CLIEngine;
const shell = require('shelljs');
const path = require('path');
const logger = require('./../logger');
const glob = require('glob');
const { userRootPath } = require('../../utils/paths')();

module.exports = () => {
module.exports = (config, flags) => {
const eslintrc = glob.sync(`${userRootPath}/.*eslintrc*`);
const configFile = eslintrc.length
? eslintrc[0]
: path.join(__dirname, '../../config/.eslintrc.base.json');

logger.info(`Using ESLint file: ${configFile}`);

// http://eslint.org/docs/developer-guide/nodejs-api
const eslintCLI = {
envs: ['browser'],
extensions: ['.js'],
useEslintrc: true,
configFile,
};

// Get the default dir or the dir specified by the user/-d.
const lint = () => {
const files = ['src/'];
const cli = new CLIEngine(eslintCLI);
const report = cli.executeOnFiles(files);
const formatter = cli.getFormatter();
logger.log(`${formatter(report.results)}\n`);

if (report.errorCount === 0) {
logger.end(`Your JS looks ${report.warningCount === 0 ? 'great ✨' :
const cmd = `eslint src/ -c ${configFile} --color --env browser ${flags.join(' ')}`;
Copy link
Contributor

@tizmagik tizmagik Oct 12, 2016

Choose a reason for hiding this comment

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

Could this just be eslint src/ -c ${configFile} ${flags.join(' ')}? E.g. drop the --color and --env flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to specify color and browser since we're running it with shelljs now

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to put env: { browser: true} in the base eslint config instead, this way it's overridable at the command-line by the user.

I believe color is enabled by default anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved env but color doesn't work without the flag.

const output = shell.exec(cmd);
if (output.code === 0) {
logger.end(`Your JS looks ${output.stdout === '' ? 'great ✨' :
'OK, though there were warnings 🤔👆'}`);
}

process.exit(report.errorCount > 0 ? 1 : 0);
process.exit(output.code > 0 ? 1 : 0);
};

lint();
Expand Down
3 changes: 1 addition & 2 deletions cli/actions/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ const {
// eslint-disable-next-line import/no-dynamic-require
const kytPkg = require(path.join(__dirname, '../../package.json'));

module.exports = (config, program) => {
const args = program.args[0];
module.exports = (config, flags, args) => {
const date = Date.now();
const tmpDir = path.resolve(userRootPath, '\.kyt-tmp'); // eslint-disable-line no-useless-escape
const repoURL = args.repository || 'git@github.com:NYTimes/kyt-starter.git';
Expand Down
5 changes: 3 additions & 2 deletions cli/actions/start.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
const shell = require('shelljs');
const logger = require('./../logger');

module.exports = (config) => {
module.exports = (config, flags) => {
logger.start('Starting production...');
shell.exec('node build/server/main.js', { async: true });
const cmd = `node build/server/main.js ${flags.join(' ')}`;
shell.exec(cmd, { async: true });
logger.end(`Server running at ${config.serverURL.href}`);
};
7 changes: 2 additions & 5 deletions cli/actions/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,10 @@ const { srcPath } = require('../../utils/paths')();
const buildConfigs = require('../../utils/buildConfigs');


module.exports = (config, program) => {
module.exports = (config, flags) => {
// Comment the following to see verbose shell ouput.
shell.config.silent = false;

// Grab args to pass along (e.g. kyt test -- --watch)
const args = program.args.filter(a => typeof a === 'string');

// set BABEL_ENV to test if undefined
process.env.BABEL_ENV = process.env.BABEL_ENV || 'test';

Expand All @@ -27,5 +24,5 @@ module.exports = (config, program) => {
jestConfig = config.modifyJestConfig(clone(jestConfig));

// Run Jest
jest.run(['--config', JSON.stringify(jestConfig), ...args]);
jest.run(['--config', JSON.stringify(jestConfig), ...flags]);
};
10 changes: 7 additions & 3 deletions cli/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ if (!shell.test('-f', userPackageJSONPath)) {
}

const loadConfigAndDo = (action, optionalConfig) => {
const args = program.args.filter(item => typeof item === 'object');
const flags = program.args.filter(item => typeof item === 'string');
const config = kytConfigFn(optionalConfig);
action(config, program);
action(config, flags, args);
};

program
Expand All @@ -36,7 +38,8 @@ program
.option('-C, --config <path>', 'config path')
.description('Start an express server for development')
.action(() => {
const config = program.args[0].config ? program.args[0].config : null;
const args = program.args.filter(item => typeof item === 'object');
const config = args[0].config ? args[0].config : null;
loadConfigAndDo(devAction, config);
});

Expand All @@ -45,7 +48,8 @@ program
.option('-C, --config <path>', 'config path')
.description('Create a production build')
.action(() => {
const config = program.args[0].config ? program.args[0].config : null;
const args = program.args.filter(item => typeof item === 'object');
const config = args[0].config ? args[0].config : null;
loadConfigAndDo(buildAction, config);
});

Expand Down
12 changes: 12 additions & 0 deletions docs/commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ node_modules/.bin/kyt setup -r git@github.com:delambo/kyt-starter-universal-angu
The `dev` command takes the entry index.js in `src/client/` and `src/server/`, compiles them, and starts client and backend servers. The dev environment includes hot reloading to allow for fast development.
Optionally, you can configure urls for the development servers in the [kyt config](/docs/kytConfig.md).

You can pass flags to the node server through `kyt dev`.
For example:
```
kyt dev -- --inspect
```
will run the [node debugging for Chrome DevTools](https://medium.com/@paul_irish/debugging-node-js-nightlies-with-chrome-devtools-7c4a1b95ae27#.mpuwgy17v)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also mention for the test and lint commands? Or, mention that flags can now be passed generically in almost all cases (except for lint-style) so it doesn't read as necessarily specific to these two commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added separate notes. good catch!

## build

The `build` command takes the entry index.js in `src/client/` and `src/server/`, compiles them, and saves them to a build folder. This is an optimized production build.
Expand All @@ -91,6 +98,11 @@ The `start` command takes the compiled code from the production build and runs a

Optionally, you can configure the server url in your [kyt.config.js](/docs/kytConfig.md).

You can also pass flags to node through `kyt start`:
```
kyt start -- --no-warnings
```

## test

The `test` command takes test files in your `src/` directory and runs them using [Jest](http://facebook.github.io/jest/).
Expand Down