Support node 7 and directories with spaces #762
Conversation
…oupled with recent versions of nodejs (like v7.1.0). Update Travis to use stable versions of node.
… single quotes, etc) in them while executing the generator-react-server tests to install dependencies with npm. We get lazy with Mac OS X and have poorly named folder structures that don't work well when executing shell scripts.
|
|
| @@ -8,7 +8,6 @@ | |||
| "prepublish": "gulp build", | |||
| "start": "react-server start", | |||
| "test": "gulp test", | |||
| "debug": "node-debug --debug-brk=0 -p 9000 react-server start", | |||
There was a problem hiding this comment.
I'm pretty sure we want to keep the debug target for test pages -- it's purpose is a place to repro and debug issues.
There was a problem hiding this comment.
I actually suggested on slack that we just drop it for now and re-add with node --inspect once someone figures out how to do that.
Didn't want to put that on @drewpc for this PR, since getting node 7 working is already tangential to what he's actually trying to do. 😆
| @@ -37,7 +37,8 @@ | |||
| "yeoman-test": "^1.0.0", | |||
| "babel-eslint": "^7.0.0", | |||
| "eslint": "^3.8.1", | |||
| "eslint-plugin-react": "^6.4.1" | |||
| "eslint-plugin-react": "^6.4.1", | |||
| "shell-escape": "^0.2.0" | |||
There was a problem hiding this comment.
Looks like the indentation is off here
| @@ -129,7 +130,7 @@ function installDeps() { | |||
| } else { | |||
| const localDeps = ['react-server-cli', 'react-server'] | |||
| .map(dep => path.resolve(path.join(__dirname, '../..', dep))); | |||
| cp.exec('npm install ' + localDeps.join(' '), (error) => { | |||
| cp.exec(shellescape(['npm', 'install'].concat(localDeps)), (error) => { | |||
There was a problem hiding this comment.
Out of curiosity, is there a deps that needs escaping, or is this defensive in case we add one? I think the change is good either way
There was a problem hiding this comment.
when the current path has spaces in it, it screws up the cp.exec() call. Example:
cp.exec('npm install /my path with spaces/react-server-cli')
What should happen instead is something like this:
cp.exec('npm install /my\ path\ with\ spaces/react-server-cli')
Same thing would happen if there were a ' in the path...it would mess up the JavaScript string.
|
Looks like the indentation is cleaned up (thanks for catching that @doug-wade). I've created #765 to capture the need to re-introduce a "debug" script to test pages. Gonna get this merged to unblock folks on node 7. Thanks @drewpc! |
gigabo
dismissed
doug-wade’s stale review
Next steps for the "debug" script in #765. Thanks!
gigabo
changed the title
Fixes bootstrap process identified in #761. Also fixes a minor issue when running tests in a folder structure with invalid shell characters in the folder names (spaces, single quotes, etc).