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

Improve error messaging when requiring routes, add CLI tests #828

Merged
merged 11 commits into from
Jan 28, 2017
1 change: 1 addition & 0 deletions packages/react-server-cli/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
node_modules
target
test/**/tmp
17 changes: 14 additions & 3 deletions packages/react-server-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"prepublish": "gulp",
"lint": "gulp eslint",
"test": "npm run ava && gulp test && nsp check",
"ava": "ava test",
"ava": "ava",
"ava-watch": "ava --watch",
"clean": "rimraf target npm-debug.log*"
},
"author": "Sasha Aickin",
Expand Down Expand Up @@ -56,18 +57,28 @@
"babel-plugin-add-module-exports": "^0.2.1",
"babel-preset-react-server": "^0.4.10",
"eslint-plugin-react": "^6.4.1",
"fs-readdir-recursive": "^1.0.0",
"gulp": "^3.9.1",
"gulp-babel": "^6.1.2",
"gulp-eslint": "^3.0.1",
"nsp": "^2.6.2",
"output-file-sync": "^1.1.2",
"react": "^15.4.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to match the rest of the project: "react": "~0.14.2 || ^15.1.0"

"react-dom": "^15.4.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to match the rest of the project: "react-dom": "~0.14.2 || ^15.1.0"

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch @drewpc. Wonder how we could automate this check... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would peer dependencies help?

"react-hot-loader": "^1.3.1",
"react-server": "^0.5.1",
"react-server-gulp-module-tagger": "^0.4.10",
"rimraf": "^2.5.4"
"rimraf": "^2.5.4",
"superagent": "1.8.4"
},
"ava": {
"require": [
"babel-core/register"
],
"tap": true
"tap": true,
"files": [
"test/**/*.js",
"!test/fixtures/*.js"
]
}
}
15 changes: 11 additions & 4 deletions packages/react-server-cli/src/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@ export default function run(options = {}) {
options.routesPath = path.resolve(process.cwd(), routesFile);
options.routesDir = path.dirname(options.routesPath);

try {
options.routes = require(options.routesPath);
} catch (e) {
// Pass. Commands need to check for routes themselves.
// No routes file available when performing init command
if (options.command !== 'init') {
try {
options.routes = require(options.routesPath);
} catch (e) {
if (e.code === 'MODULE_NOT_FOUND') {
console.error(chalk.red(`Failed to load routes file at ${options.routesPath}`));
}

throw e;
}
}

options.outputUrl = jsUrl || `${httpsOptions ? "https" : "http"}://${host}:${jsPort}/`;
Expand Down
98 changes: 98 additions & 0 deletions packages/react-server-cli/test/commands.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import path from 'path';
import fs from 'fs';
import readdirSyncRecursive from 'fs-readdir-recursive';
import outputFileSync from 'output-file-sync';
import child_process from 'child_process';
import test from 'ava';
import rimraf from 'rimraf';

const fixturesPath = path.join(__dirname, 'fixtures', 'commands');

fs.readdirSync(fixturesPath).forEach(testName => {
if (testName[0] === '.') return;

const [, command, testType] = testName.match(/([^-]+)-(.+)/);

test(`${command} command: ${testType}`, async t => {
const testPath = path.join(fixturesPath, testName);
const tmpPath = path.join(testPath, 'tmp');
createAndChangeToTempDir(tmpPath);

// Write files to temporary location
Object.entries(readDir(path.join(testPath, 'in-files')))
.forEach(([filename, content]) =>
outputFileSync(filename, content)
);

const {
args,
stdoutIncludes,
stderrIncludes,
} = JSON.parse(fs.readFileSync(path.join(testPath, 'options.json')));

const server = child_process.spawn(
process.execPath,
[
path.join(__dirname, '..', 'bin', 'react-server-cli'),
...args,
]
);

let stdout = '';
let stderr = '';

server.stdout.on('data', chunk => stdout += chunk);
server.stderr.on('data', chunk => stderr += chunk);

const frequency = 100;
let elapsed = 0;

// Wait for the expected output or the timeout
await new Promise(resolve => {
const checkForExpectedOutput = setInterval(() => {
// Increment the elapsed time if neither stdout nor stderr includes the expected content and the time limit hasn't been reached.
if (
(
(stdoutIncludes && !stdout.includes(stdoutIncludes)) ||
(stderrIncludes && !stderr.includes(stderrIncludes))
) &&
elapsed < 5000
) {
elapsed += frequency;
return;
}

clearInterval(checkForExpectedOutput);
resolve();
}, frequency);
});

if (stdoutIncludes) t.true(stdout.includes(stdoutIncludes), 'stdout includes expected output');
if (stderrIncludes) t.true(stderr.includes(stderrIncludes), 'stderr includes expected output');

server.kill();

// Remove temporary directory after the test
rimraf.sync(tmpPath);
});
});

function createAndChangeToTempDir(tmpPath){
if (fs.existsSync(tmpPath)) rimraf.sync(tmpPath);
fs.mkdirSync(tmpPath);
process.chdir(tmpPath);
}

function noDotDirectory(x){
return x !== '.';
}

function readDir(dirPath){
const files = {};
if (fs.existsSync(dirPath)) {
readdirSyncRecursive(dirPath, noDotDirectory).forEach(filename => {
files[filename] = fs.readFileSync(path.join(dirPath, filename));
});
}
return files;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"middleware": [],
"routes": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"args": ["start"],
"stdoutIncludes": "Started HTML server over HTTP on"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"args": ["start"],
"stderrIncludes": "Failed to load routes file at"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"routesFile": "customRoutes.js",
"port": 4000,
"jsPort": 4001
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
routes: {}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"args": ["start"],
"stdoutIncludes": "Started HTML server over HTTP on"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
undeclaredVariable();
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"args": ["start", "--routes-file", "routes.js"],
"stderrIncludes": "ReferenceError: undeclaredVariable is not defined"
}