-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Hyper-cli] embed hpm #2375
[Hyper-cli] embed hpm #2375
Conversation
* add_cli_cmd: Fix path for linux
* add_cli_cmd: Add Windows compatibility
I've submitted a PR for |
I can't add reviewers but I nominate @albinekb :) |
Instead of going to the path in the first argument, can it be a command? hyper Alternatively, use a flag argument. EX: `hyper --run "ls ~/". It might be out of scope with the current pull request though, but it's something to think about. |
cli/index.js
Outdated
const args = require('args'); | ||
const chalk = require('chalk'); | ||
const opn = require('opn'); | ||
// const columnify = require('columnify'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used by lsRemote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return true; | ||
} | ||
let msg = chalk.red(`Error! Config file not found: ${api.configPath}\n`); | ||
msg += 'Please launch Hyper and retry.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const message = `
${chalk.red(`Error! Config file not found: ${api.configPath}`)}
Please launch Hyper and retry.
`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an issue to make the cli ask to create the file if it does not exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Config creation should remain an Hyper
task. And yes, we should improve this automatic creation. But this is not a blocker. Most of case, config file will be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
app/utils/cli-install.js
Outdated
reject(err); | ||
return; | ||
} | ||
// C:\Users\<user>\AppData\Local\hyper\app-2.0.4\resources\bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// C:\Users\<user>\AppData\Local\hyper\app-<version>\resources\bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
cli/api.js
Outdated
@@ -0,0 +1,112 @@ | |||
const fs = require('fs'); | |||
const os = require('os'); | |||
const npmName = () => Promise.resolve(); //require('npm-name'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hoped that this PR would have been merged sooner: dominictarr/rc#94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
cli/index.js
Outdated
process.exit(1); | ||
}); | ||
/* | ||
const lsRemote = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be reactivated when NPM API request (URL and criteria) will be chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@htkoca thank you for your feedback. You're right. Passing command could be very usefull. Imo, I prefer command as an option like your |
For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work @chabou 🙏
vercel#2375 adds a wrapper CLI tool that parses the ~/.hyper.js configuration file. However, the static analysis logic can't possibly handle every possible configuration file without executing it. In my case, [my configuration file][0] defines a constant at the top, which means that the `module.exports` definition isn't the first statement. This breaks the parsing logic. However, this static analysis is only necessary if one wishes to use the CLI subcommands. They're not needed if one just wants to launch hyper. Unfortunately, the existing implementation tries to read from the AST immediately when `cli/api.js` is imported. This changes it to be lazy, and uses memoization retain the same reference, allowing the AST to still be mutated in-place. [0]: https://github.com/bgw/dotfiles/blob/665e83fd7a13be675ad0ae984478d5b0de502ee1/hyper.js#L4
build/win/hyper
Outdated
@@ -5,6 +5,9 @@ NAME="Hyper" | |||
HYPER_PATH="$(dirname "$(dirname "$(dirname "$(realpath "$0")")")")" | |||
ELECTRON="$HYPER_PATH/$NAME.exe" | |||
if grep -q Microsoft /proc/version; then | |||
echo "Warning! Due to WSL limitations, you can use CLI commands here. Please use Hyper CLI on cmd, PowerShell or GitBash/CygWin." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? Do you mean "you cannot use Hyper CLI here"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just missing the 't
@chabou I left a comment about the language of the warning message in As an aside, I wonder why my comment was on an outdated commit. |
Yes this is a typo. |
This PR has PR #2342 already merged into it.
Next steps:
Hyper CLI
to create a new instance ofHyper
if it is already launched. It should act the same way that launching Hyper twice by icon.Hyper
and not install/uninstall/list plugins due to Env var problem: see Allow environment to be set when invoking Win32 process from WSL microsoft/WSL#1494Bonus:
hyper <path>
should open Hyper in this pathScreencast: