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

Cli commands #490

Merged
merged 16 commits into from
Aug 3, 2016
Merged

Cli commands #490

merged 16 commits into from
Aug 3, 2016

Conversation

gigabo
Copy link
Contributor

@gigabo gigabo commented Aug 2, 2016

Now that the CLI can be installed globally it can be used to bootstrap a React Server site.

This is totally new functionality that's only tangentially related to the previous "start server" behavior. So... I've added "commands".

$ react-server
Usage: react-server <command> [options]

Commands:
  start     Start the server
  compile   Compile static assets
  init      Initialize a React Server site
  add-page  Add a page to an existing site

Options:
  --routes-file        The routes file to load. Default is 'routes.js'.
  -p, --port           Port to start listening for react-server. Default is 3000.
...

The new quick start in the CLI doc looks like:

$ npm install -g react-server-cli
$ react-server init
$ react-server add-page '/' Homepage
$ react-server start

This is a pretty big change to the interface of react-server-cli.

@aickin - How does this look to you?

@gigabo gigabo added the enhancement New functionality. label Aug 2, 2016
@@ -4,7 +4,7 @@ simultaneously, so port conflicts will fail the tests. Make sure
to update this manifest when adding a test that starts a server.

## generator-react-server
3010, 3011
3000, 3001
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't 3001 conflict with react-server-integration-test?

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 don't think so. I think react-server-integration-test uses a dummy static asset server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, you're right. 😞
image

//
// Gotta do this the old-fashioned way. :p
//
export default function ConfigurationError(message) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to consider using VError for our custom error classes

import setupLogging from "../setupLogging";
import logProductionWarnings from "../logProductionWarnings";

const logger = require("react-server").logging.getLogger(__LOGGER__);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we were trying to set the pattern of import {logging} from 'react-server'; const logger = loggin.getLogger(__LOGGER__);? Or is that a non-goal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

@@ -0,0 +1,10 @@
import fs from "fs";

export default function fileExists(fn) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ That still throws. This is just a helper that returns boolean.

@@ -119,6 +136,14 @@ const sslize = argv => {
}
}

if (argv.https && (argv.httpsKey || argv.httpsCert || argv.httpsCa || argv.httpsPfx || argv.httpsPassphrase)) {
throw new Error("If you set https to true, you must not set https-key, https-cert, https-ca, https-pfx, or https-passphrase.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we decide to go all-in with good error handling (probably out-of-scope), maybe a ParseError?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For ordinary errors where stack trace is actually desirable I don't see any need to subclass.

applies here too

@doug-wade
Copy link
Collaborator

I had a lot to say about this pr, but its super awesome and I like the new interface way better and I whole-heartedly endorse this plan. Thanks for pulling this together @gigabo

@gigabo
Copy link
Contributor Author

gigabo commented Aug 3, 2016

Gonna merge this because I want to build on it.

@aickin - If you get a chance please take a look!

@gigabo gigabo merged commit c3533db into redfin:master Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants