fix(react): use and detect local npx to create-react-app #85
Conversation
|
Pull Request Report PR Title ✅ Title follows the conventional commit spec. |
| return false; | ||
| } | ||
|
|
||
| if (stdio.stderr) { |
There was a problem hiding this comment.
npx depends on update-notifier.
What would happen if the user has an older version of npx and gets the update warning message? Will it be redirected to stderr?
There was a problem hiding this comment.
I've tested this and it seems everything will be okay/will still function properly with an older version of npm/node/npx 🤞, as long as it's higher than Node 10.16, and npx is globally available (no matter the version).
| this.warn('Please visit https://nodejs.org/ for installation or upgrade.'); | ||
| this.warn( | ||
| 'Or use (strongly recommended) https://github.com/nvm-sh/nvm and https://github.com/coreybutler/nvm-windows to manage multiple version of Node.js' | ||
| ); |
There was a problem hiding this comment.
Use a template string instead of two warn
| this.warn('Please visit https://nodejs.org/ for installation or upgrade.'); | |
| this.warn( | |
| 'Or use (strongly recommended) https://github.com/nvm-sh/nvm and https://github.com/coreybutler/nvm-windows to manage multiple version of Node.js' | |
| ); | |
| this.warn( | |
| `Please visit https://nodejs.org/ for installation or upgrade. | |
| Or use (strongly recommended) https://github.com/nvm-sh/nvm and https://github.com/coreybutler/nvm-windows to manage multiple version of Node.js` | |
| ); |
| import {Storage} from '../../../lib/oauth/storage'; | ||
| import AuthenticationRequired from '../../../lib/decorators/authenticationRequired'; | ||
| import {AuthenticatedClient} from '../../../lib/platform/authenticatedClient'; | ||
|
|
||
| import {lt} from 'semver'; |
There was a problem hiding this comment.
that's a really cryptic name, can you use an as to rename it to something more meaningful?
There was a problem hiding this comment.
lt gt for lessThan, greaterThan. That's the same in HTML ;)
I can alias, sure !
| private async checkIfUserHasNpx() { | ||
| const stdio = await spawnProcessStdio('npx', ['--version']); | ||
|
|
||
| if (stdio.stderr && stdio.stderr.match(/ENOENT/i)) { |
There was a problem hiding this comment.
I would assert on the actual exit code (int). I do a few suggestions, starting here:
| if (stdio.stderr && stdio.stderr.match(/ENOENT/i)) { | |
| if (stdio.exitcode === constants.errno.ENOENT) { |
| import {Storage} from '../../../lib/oauth/storage'; | ||
| import AuthenticationRequired from '../../../lib/decorators/authenticationRequired'; | ||
| import {AuthenticatedClient} from '../../../lib/platform/authenticatedClient'; | ||
|
|
||
| import {lt} from 'semver'; |
There was a problem hiding this comment.
Adding constants from os for the kill code assertion
| import {lt} from 'semver'; | |
| import {lt} from 'semver'; | |
| import {constants} from 'os'; |
| options: SpawnOptions = {} | ||
| ): Promise<{stdout: string; stderr: string}> { | ||
| return new Promise((resolve) => { | ||
| const stdio = {stdout: '', stderr: ''}; |
There was a problem hiding this comment.
Add exitCode to the stdio (prolly should be renamed to something else like processOutputs)
| const stdio = {stdout: '', stderr: ''}; | |
| const stdio = {stdout: '', stderr: '', exitCode: null}; |
| child.on('close', () => { | ||
| resolve(stdio); | ||
| }); |
There was a problem hiding this comment.
| child.on('close', () => { | |
| resolve(stdio); | |
| }); | |
| child.on('close', (code) => { | |
| stdio.exitCode = code; | |
| resolve(stdio); | |
| }); |
| import {Storage} from '../../../lib/oauth/storage'; | ||
| import AuthenticationRequired from '../../../lib/decorators/authenticationRequired'; | ||
| import {AuthenticatedClient} from '../../../lib/platform/authenticatedClient'; | ||
|
|
||
| import {lt} from 'semver'; |
There was a problem hiding this comment.
lt gt for lessThan, greaterThan. That's the same in HTML ;)
I can alias, sure !
| return false; | ||
| } | ||
|
|
||
| if (stdio.stderr) { |
There was a problem hiding this comment.
I've tested this and it seems everything will be okay/will still function properly with an older version of npm/node/npx 🤞, as long as it's higher than Node 10.16, and npx is globally available (no matter the version).
| private async checkIfUserHasNodeGreaterThan10() { | ||
| const output = await spawnProcessOutput('node', ['--version']); | ||
| if (this.isMissingExecutable(output)) { | ||
| this.warn(` |
There was a problem hiding this comment.
You will see I've split up all the warn message with an empty line at the start and end of the message.
It help readability in the CLI at the end, with a nice "paragraph" separation kinda thing.
eg:
› Warning:
› ui:create:react requires a valid Node.js installation to run.
› An unknown error happened while trying to determine your node version with node --version
› spawn foo ENOENT
Try to detect valid
nodeversion 10.16.0 ( https://github.com/facebook/create-react-app#creating-an-app ) + npx.Otherwise, warn and exit, with some (hopefully) helpful message about how to install node with nvm.
https://coveord.atlassian.net/browse/CDX-102