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

karlhorky · 2017-01-11T18:09:43Z

If there is an error in your routes.js file (for instance, referencing an undeclared variable), react-server-cli will fail with a confusing error message:

TypeError: Cannot read property 'routes' of undefined
    at exports.default (~/projects/rs/node_modules/react-server-cli/target/buildWebpackConfigs.js:112:77)
    at buildWebpack (~/projects/rs/node_modules/react-server-cli/target/commands/start.js:302:54)
    at start (~/projects/rs/node_modules/react-server-cli/target/commands/start.js:109:20)
    at run (~/projects/rs/node_modules/react-server-cli/target/run.js:61:74)
    at process._tickCallback (internal/process/next_tick.js:103:7)
    at Module.runMain (module.js:609:11)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:535:3

I saw that @gigabo added this try / catch in #490 so that the errors can be handled in the commands but I cannot see how this case is handled.

Tests

CLAassistant · 2017-01-11T18:09:44Z

All committers have signed the CLA.

doug-wade · 2017-01-11T21:39:53Z

Hi @karlhorky @kaytcat! Thanks for pitching in!

I definitely agree that's a really unhelpful error, and that we need to improve the user experience. Unfortunately, since our test coverage for the cli is pretty low, I can't quite be certain whether this change is good. In particular, we added that when we made react-server-cli globally installable, and I suspect that this change will introduce a regression for when you're using react-server-cli outside of an existing React Server project. What happens when you're bootstrapping a new project with react-server init? Won't the require throw an equally-confusing error?

I think we'll need to keep the try-catch, and inspect the error. I think something like this should work

try {
  options.routes = require(options.routesPath);
} catch (e) {
  if (isParseError(e)) {
    throw e;
  }
  if (isNonDefaultRoutes(options.routes) && isFileNotFoundError(e)) {
    throw new Error("You've specified a custom routes file that does not exist.  Please add it");
  }
  // Pass. Commands need to check for routes themselves.
}

Tests would also be wonderful.

karlhorky · 2017-01-12T16:30:56Z

Ah right, good catch. Looking through the commands, they all need the options.routes except for init. I've wrapped the require in a conditional to test for that. The error message now complains about a missing routes.json, which would lead to a clearer debugging experience:

➜  src git:(add-error-output) ✗ node cli.js start
{ Error: Cannot find module '~/projects/rs/packages/react-server-cli/src/routes.json'
    at Function.Module._resolveFilename (module.js:472:15)
    at Function.Module._load (module.js:420:25)
    at Module.require (module.js:500:17)
    at require (internal/module.js:20:19)
    at run (~/projects/rs/packages/react-server-cli/src/run.js:29:20)
    at process._tickCallback (internal/process/next_tick.js:103:7)
    at Module.runMain (module.js:609:11)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:535:3 code: 'MODULE_NOT_FOUND' }

I'm open to improving this though, if we think this error message is still too confusing.

karlhorky · 2017-01-12T16:37:25Z

As for tests, I will take a look at how other popular command line libraries like babel test their CLI tools and do something similar in packages/react-server-cli/test.

doug-wade · 2017-01-12T19:47:41Z

Error: Cannot find module '~/projects/rs/packages/react-server-cli/src/routes.json'

That seems reasonable to me

drewpc

I agree with this logic and really appreciate the contribution. Question, though: why "fail hard" instead of output an appropriate, specific error and then end the process? Also...can you please add the tests so we can incorporate this?

karlhorky · 2017-01-16T16:12:55Z

why "fail hard" instead of output an appropriate, specific error and then end the process

Ah maybe that's just bad wording on my part, sorry. I do mean that what you're describing, I think. I'll change the title of the PR.

karlhorky · 2017-01-16T16:13:14Z

Tests are coming along! It took a bit of work but I have AVA now executing the start command and checking some output.

➜  react-server-cli git:(add-cli-tests) ✗ npm run ava

> react-server-cli@0.5.1 ava ~/projects/rs/packages/react-server-cli
> ava

TAP version 13
2017-01-16T16:10:54.061Z - warning: [react-server-cli.src.logProductionWarnings] PRODUCTION WARNING: the following current settings are discouraged in production environments. (If you are developing, carry on!):
2017-01-16T16:10:54.064Z - warning: [react-server-cli.src.logProductionWarnings] -- Hot reload is enabled. To disable, set hot to false (--hot=false at the command-line) or set NODE_ENV=production.
2017-01-16T16:10:54.065Z - warning: [react-server-cli.src.logProductionWarnings] -- Minification is disabled. To enable, set minify to true (--minify at the command-line) or set NODE_ENV=production.
2017-01-16T16:10:54.066Z - warning: [react-server-cli.src.logProductionWarnings] -- Long-term caching is disabled. To enable, set longTermCaching to true (--long-term-caching at the command-line) or set NODE_ENV=production to turn on.
2017-01-16T16:10:54.066Z - info: [react-server-cli.src.logProductionWarnings] NODE_ENV is set to undefined
2017-01-16T16:10:54.172Z - notice: [react-server-cli.src.commands.start] Starting servers...
2017-01-16T16:10:54.172Z - info: [react-server-cli.src.commands.start] Starting hot reload JavaScript server...
2017-01-16T16:10:54.285Z - info: [react-server-cli.src.commands.start] Starting HTML server...
2017-01-16T16:10:54.400Z - info: [react-server-cli.src.commands.start] Started hot reload JavaScript server over HTTP on 0.0.0.0:3001
2017-01-16T16:10:54.401Z - info: [react-server-cli.src.commands.start] Started HTML server over HTTP on 0.0.0.0:3000
2017-01-16T16:10:54.401Z - notice: [react-server-cli.src.commands.start] Ready for requests on 0.0.0.0:3000.
# test
ok 1 - test

1..1

Doing this work over here: kaytcat#1

Going to add a few test cases including a routes.js file with an error in it.

drewpc · 2017-01-16T16:16:10Z

@karlhorky Thanks for continuing to work on this. Often the smallest changes take the longest time to write tests! I wasn't so much as concerned with the wording as the question of: why output an error + stack trace instead of something like "The routesFile specified doesn't exist. Please create a proper routesFile and re-run this command again." followed by process.exit(1)

karlhorky · 2017-01-18T07:21:18Z

why output an error + stack trace instead of something like "The routesFile specified doesn't exist. ... "

Good point, in the case that it doesn't find the file we may want to improve the error message if the error I posted above (#828 (comment)) isn't a good enough Developer Experience. I'm good with changing it if that's what we want to do!

In the case that there is really an error in routes.js, because it could be anything we probably shouldn't try to be smart about that one - just let it crash with the real error message.

drewpc · 2017-01-18T16:06:36Z

@karlhorky Agreed. So maybe a happy medium? Something like this:

try {
    if (options.command !== 'init') {
        options.routes = require(options.routesPath);
    }
} catch (e) {
    console.error(chalk.red(`The routes file '${options.routesPath}' failed to load properly`));
    throw e;
}

* Start setup of react-server-cli integration tests * Run tests in temp directories * Ignore temp directories generated by tests

karlhorky · 2017-01-26T18:12:40Z

Ok, tests are done (inspired by Babel's testing system from babel/babel#1978), I'll now improve the messaging guided by @drewpc 's suggestion and then we can get this one in!

karlhorky · 2017-01-27T10:41:44Z

I've improved the error message. Summoning @drewpc and @doug-wade for review!

gigabo · 2017-01-27T20:19:21Z

Nice improvement @karlhorky. Love the tests! 🤘

drewpc

Minor change to the packages.json file to match the rest of the project. I tried to make these changes myself, but we weren't given edit rights to the code in this PR. Thanks @karlhorky!

drewpc · 2017-01-27T20:30:40Z

packages/react-server-cli/package.json

    "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",


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

drewpc · 2017-01-27T20:31:11Z

packages/react-server-cli/package.json

    "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",
+    "react-dom": "^15.4.2",


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

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

Would peer dependencies help?

doug-wade

This is awesome and I love it. Thanks for sticking with it @karlhorky

karlhorky · 2017-01-28T09:45:38Z

@drewpc Fixed the versions, please re-review

kaytcat added 2 commits January 11, 2017 13:37

Output errors from the routes

1d16729

Remove try catch completely

544ca6b

karlhorky added 2 commits January 12, 2017 17:26

Disable routes require when command is 'init'

d9141e0

Conform to code style, add comment

dec327b

karlhorky mentioned this pull request Jan 13, 2017

Add CLI argument for configuration path #804

Closed

3 tasks

drewpc reviewed Jan 16, 2017

View reviewed changes

karlhorky changed the title ~~Fail hard on error when requiring routes~~ Improve error messaging when requiring routes Jan 16, 2017

Add integration tests for react-server-cli (#1)

c44107c

* Start setup of react-server-cli integration tests * Run tests in temp directories * Ignore temp directories generated by tests

karlhorky changed the title ~~Improve error messaging when requiring routes~~ Improve error messaging when requiring routes, add CLI tests Jan 26, 2017

karlhorky added 2 commits January 27, 2017 11:40

Improve error message

f1bf63b

Add comment

74513c1

karlhorky added 2 commits January 27, 2017 15:53

Lock dependency

8e73edc

Fix linting errors

4758224

gigabo approved these changes Jan 27, 2017

View reviewed changes

drewpc requested changes Jan 27, 2017

View reviewed changes

drewpc modified the milestone: 0.6.0 Jan 27, 2017

doug-wade self-requested a review January 28, 2017 00:21

doug-wade approved these changes Jan 28, 2017

View reviewed changes

drewpc modified the milestones: 0.6.1, 0.6.0 Jan 28, 2017

Fix react and react-dom versions

aeef9cb

Merge branch 'master' into add-error-output

9d596cd

drewpc approved these changes Jan 28, 2017

View reviewed changes

drewpc merged commit f57a62d into redfin:master Jan 28, 2017

drewpc added the cleanup label Jan 28, 2017

karlhorky deleted the add-error-output branch January 28, 2017 14:42

karlhorky restored the add-error-output branch January 28, 2017 14:42

karlhorky deleted the add-error-output branch January 28, 2017 14:42

karlhorky mentioned this pull request Feb 7, 2017

Get dynamic config from process.env.REACT_SERVER_CONFIGS #871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

karlhorky commented Jan 11, 2017 •

edited

Loading

CLAassistant commented Jan 11, 2017 •

edited

Loading

doug-wade commented Jan 11, 2017

karlhorky commented Jan 12, 2017 •

edited

Loading

karlhorky commented Jan 12, 2017

doug-wade commented Jan 12, 2017

drewpc left a comment •

edited

Loading

karlhorky commented Jan 16, 2017 •

edited

Loading

karlhorky commented Jan 16, 2017 •

edited

Loading

drewpc commented Jan 16, 2017

karlhorky commented Jan 18, 2017

drewpc commented Jan 18, 2017

karlhorky commented Jan 26, 2017

karlhorky commented Jan 27, 2017

gigabo commented Jan 27, 2017

drewpc left a comment

drewpc Jan 27, 2017

drewpc Jan 27, 2017

gigabo Jan 28, 2017

karlhorky Jan 28, 2017

doug-wade left a comment

karlhorky commented Jan 28, 2017

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

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

Conversation

karlhorky commented Jan 11, 2017 • edited Loading

CLAassistant commented Jan 11, 2017 • edited Loading

doug-wade commented Jan 11, 2017

karlhorky commented Jan 12, 2017 • edited Loading

karlhorky commented Jan 12, 2017

doug-wade commented Jan 12, 2017

drewpc left a comment • edited Loading

Choose a reason for hiding this comment

karlhorky commented Jan 16, 2017 • edited Loading

karlhorky commented Jan 16, 2017 • edited Loading

drewpc commented Jan 16, 2017

karlhorky commented Jan 18, 2017

drewpc commented Jan 18, 2017

karlhorky commented Jan 26, 2017

karlhorky commented Jan 27, 2017

gigabo commented Jan 27, 2017

drewpc left a comment

Choose a reason for hiding this comment

drewpc Jan 27, 2017

Choose a reason for hiding this comment

drewpc Jan 27, 2017

Choose a reason for hiding this comment

gigabo Jan 28, 2017

Choose a reason for hiding this comment

karlhorky Jan 28, 2017

Choose a reason for hiding this comment

doug-wade left a comment

Choose a reason for hiding this comment

karlhorky commented Jan 28, 2017

karlhorky commented Jan 11, 2017 •

edited

Loading

CLAassistant commented Jan 11, 2017 •

edited

Loading

karlhorky commented Jan 12, 2017 •

edited

Loading

drewpc left a comment •

edited

Loading

karlhorky commented Jan 16, 2017 •

edited

Loading

karlhorky commented Jan 16, 2017 •

edited

Loading