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

Ensure generated .reactserverrc option keys match cli option keys #625

Merged
merged 5 commits into from
Aug 26, 2016
Merged

Ensure generated .reactserverrc option keys match cli option keys #625

merged 5 commits into from
Aug 26, 2016

Conversation

jbenesch
Copy link
Contributor

@jbenesch jbenesch commented Aug 25, 2016

Looks like the cli started using camelCasing for configuration options, whereas the generator generates a config file that uses dashes to separate words.

So when following the getting started instructions with the generator, and trying to change the js-port option, results in the port remaining 3001. Updating the option to jsPort, corrects the issue.

Added a unit test to future proof changes to the defaultOptions in the cli package and the generator.

}
}
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa. This is sweet!

@gigabo gigabo added the bug An issue with the system label Aug 26, 2016
.toPromise();

// Reading files here instead of requiring because we don't understand export default here.
const defaultOptions = await readFile('defaultOptions.js', path.join(__dirname, '../../react-server-cli/src'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems easier to just require(path.resolve(...)) here... what's the benefit of reading and parsing like this?

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 completely agree but when I try and require it I get this error:

# generator-react-server:app .reactserverrc matches react-server-cli default option keys
not ok 1 - failed with "Unexpected token export"
  ---
    operator: undefined
    expected: undefined
    actual: undefined
    at: SyntaxError: Unexpected token export
  ...

Thoughts? There's gotta be a babel plugin different between react-server-cli and the generator but I couldn't quite figure it out and didn't really want to add a package for a unit 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 get this error when I try and import the file at the top:

import defaultOptions from '../../react-server-cli/src/defaultOptions.js';

TAP version 13
/Users/jason/Projects/react-server/packages/react-server-cli/src/defaultOptions.js:14
export default {
^^^^^^
SyntaxError: Unexpected token export
    at Object.exports.runInThisContext (vm.js:76:16)
    at Module._compile (module.js:513:28)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Yeah, that's the source file, which hasn't gone through babel yet.

I think the nicest here would be to export defaultOptions from react-server-cli?

Then you could just do:

import {defaultOptions} from "react-server-cli";

If you're up for that, the CLI exports are set up here. Because the CLI has a build step that does babel transpilation you'll need to cd in there and npm run prepublish for the changes to become available.

@doug-wade
Copy link
Collaborator

doug-wade commented Aug 26, 2016

omg this is the best and you are the best and I ❤️ everything about this forever ✨ 🚀 🚢 :shipit:

@jbenesch jbenesch changed the title Ensure generated .reactserverrc option keys match cli option keys Generator-react-server: Ensure generated .reactserverrc option keys match cli option keys Aug 26, 2016
@jbenesch jbenesch changed the title Generator-react-server: Ensure generated .reactserverrc option keys match cli option keys Ensure generated .reactserverrc option keys match cli option keys Aug 26, 2016
@doug-wade doug-wade merged commit 0b64f43 into redfin:master Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants