[WIP] Hyper CLI#2337
Conversation
For now, only macOS has been tested and it works great:
|
albinekb
left a comment
albinekb
left a comment
There was a problem hiding this comment.
Looks 👍 to me, just a few comments. Awesome job, and nice that we're looking at what VS Code does as their setup seems to work great.
| const target = process.platform === 'darwin' ? '/usr/local/bin/hyper' : '/usr/bin/hyper'; | ||
| const source = cliScriptPath; | ||
|
|
||
| const checkInstall = () => { |
There was a problem hiding this comment.
I would write all these promise-chain-functions without the return statement (implicit return) saves a few lines and also makes the code more readable, but if it's like this in the readme, keep it. 👍 (very very minor thing 😂 )
There was a problem hiding this comment.
I really prefer a first explicit return in these functions
| console.log('running in prod mode'); | ||
| if (process.platform === 'win32') { | ||
| //eslint-disable-next-line no-console | ||
| addBinToUserPath().catch(err => console.error('Failed to add Hyper CLI path to user PATH', err)); |
There was a problem hiding this comment.
Should this be shown to the user (notification) or not?
There was a problem hiding this comment.
Not enough place to print error in notification.
On macOS, error could be read in Console.app. But I don't know for other platform
There was a problem hiding this comment.
Let's keep it as it is and see if it's needed later on 👍 Let's not over engineer it
| addBinToUserPath().catch(err => console.error('Failed to add Hyper CLI path to user PATH', err)); | ||
| } else { | ||
| //eslint-disable-next-line no-console | ||
| addSymlink().catch(err => console.error('Failed to symlink Hyper CLI', err)); |
There was a problem hiding this comment.
Same with this one, shown to the user or just a log?
| "queue": "4.4.0", | ||
| "semver": "5.4.1", | ||
| "shell-env": "0.3.0", | ||
| "util.promisify": "false1.0.0", |
There was a problem hiding this comment.
this is built in: https://nodejs.org/api/util.html#util_util_promisify_original
There was a problem hiding this comment.
I forgot to remove it from this file.
I replaced util.promisify by pify.
Promisify built-in is only available from node v8.x and we are using v7.9.0 (with Electron v1.7.8).
albinekb
left a comment
albinekb
left a comment
There was a problem hiding this comment.
Remove line 30 of package.json
"util.promisify": "false1.0.0",
albinekb
left a comment
albinekb
left a comment
There was a problem hiding this comment.
A W E S O M E 💯 I like this PR, it's not too huge and does only one thing, very :nice:
|
I'm closing this PR. |
2 participants
Will fix #2288