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

[Hyper-cli] embed hpm #2375

Merged
merged 39 commits into from
Jan 9, 2018
Merged

[Hyper-cli] embed hpm #2375

merged 39 commits into from
Jan 9, 2018

Conversation

chabou
Copy link
Contributor

@chabou chabou commented Oct 18, 2017

This PR has PR #2342 already merged into it.

Next steps:

  • Prevent Hyper CLI to create a new instance of Hyper if it is already launched. It should act the same way that launching Hyper twice by icon.
    • macOS (new tab)
    • Windows (new window)
    • Linux (new window)
  • Test it on Linux
  • Test it on Windows

Bonus:

  • hyper <path> should open Hyper in this path
    • macOS
    • Windows
    • Linux
    • Check if path exists

Screencast:
hyper-cli

@chabou chabou added the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Oct 18, 2017
@chabou chabou added this to the 2.0.0 milestone Oct 18, 2017
@chabou chabou mentioned this pull request Nov 2, 2017
@chabou chabou removed the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Nov 4, 2017
@chabou
Copy link
Contributor Author

chabou commented Nov 4, 2017

One cool side effect is that Hyper CLI is our open <path> in new tab feature (on macOS):
hyper-cli-openinnewtab

But the more important thing is: THIS PR IS READY TO MERGE 🎉 🎉 🎉

@peterfraedrich
Copy link

I've submitted a PR for hpm that deprecates the use of npm-name and adds authenticated proxy support.
Link To PR

@Stanzilla
Copy link
Contributor

I can't add reviewers but I nominate @albinekb :)

@tkodev
Copy link

tkodev commented Nov 26, 2017

Instead of going to the path in the first argument, can it be a command? hyper cd /path rather than just /path . This will allow cd to handle path existence (which it is designed to do), and also allow other applications to run things in hyper (such as an SFTP program opening a SSH connection in 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');
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

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

Copy link
Contributor Author

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.';
Copy link
Contributor

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.
`

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

reject(err);
return;
}
// C:\Users\<user>\AppData\Local\hyper\app-2.0.4\resources\bin
Copy link
Contributor

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

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'll fix 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.

fixed

cli/api.js Outdated
@@ -0,0 +1,112 @@
const fs = require('fs');
const os = require('os');
const npmName = () => Promise.resolve(); //require('npm-name');
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

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 hoped that this PR would have been merged sooner: dominictarr/rc#94

Copy link
Contributor Author

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 = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@chabou
Copy link
Contributor Author

chabou commented Nov 27, 2017

@htkoca thank you for your feedback. You're right. Passing command could be very usefull.

Imo, I prefer command as an option like your --run suggestion. Most of time, users will only want to open Hyper on a (default) location. I don't want to oblige them to add cd to their path.

@albinekb @rauchg @leo Thoughts ?

@chabou chabou mentioned this pull request Jan 4, 2018
@chabou
Copy link
Contributor Author

chabou commented Jan 9, 2018

For install, unsinstall, docs commands, hyper- prefix is added if not present.
For search and ls-remote commands, results are filtered to show only hyper- prefixed packages.

Copy link
Contributor

@albinekb albinekb left a 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 🙏

@chabou chabou merged commit 5700690 into vercel:canary Jan 9, 2018
bgw added a commit to bgw/hyper that referenced this pull request Jan 12, 2018
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."

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"?

Copy link
Contributor

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

@amckinlay
Copy link

@chabou I left a comment about the language of the warning message in build/win/hyper.

As an aside, I wonder why my comment was on an outdated commit.

@chabou chabou deleted the embed_hpm branch April 27, 2018 07:06
@chabou
Copy link
Contributor Author

chabou commented Apr 27, 2018

Yes this is a typo.
Can you send a PR or open an issue if you can't?
Thank you 🙏

@Stanzilla Stanzilla mentioned this pull request Apr 27, 2018
@Stanzilla Stanzilla mentioned this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants