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

Make the default app generator prompt user for choice #67

Merged
merged 3 commits into from Sep 16, 2019

Conversation

@zone117x
Copy link
Member

commented Sep 10, 2019

Closes issue:


The default app generator now prompts user for choice:

npm -g yo generator-blockstack
yo blockstack

Results in:
image

Additionally, the yeoman-environment is now integrated into the package.json bin script. This now allows non-global usage and a single command.

Examples:

npx generator-blockstack
# ? Select Blockstack app output type (Use arrow keys)
# ❯ React 
#   Plain JavaScript 
#   Vue 
npx generator-blockstack --help
# Options:
#  -h,   --help           # Print the generator's options and usage
#        --skip-cache     # Do not remember prompt answers                     Default: false
#        --skip-install   # Do not automatically install dependencies          Default: false
#        --force-install  # Fail on install dependencies error                 Default: false
#        --react          # Generate a React app without prompting
#        --plain          # Generate a plain JavaScript app without prompting
#        --vue            # Generate a Vue app without prompting
npx generator-blockstack --react

# The above command is equivalent to running:
npm -g yo generator-blockstack
yo blockstack:react

Testing

The above commands referencing generator-blockstack won't work until the package is published to npm. This github branch must be fully specified. Example:

npx blockstack/blockstack-app-generator#feature/change-defaults
# or
npm install -g npx blockstack/blockstack-app-generator#feature/change-defaults
…ther app generators.

* Support for direct usage via npx.
@zone117x zone117x requested review from moxiegirl and yknl Sep 10, 2019
@zone117x zone117x self-assigned this Sep 10, 2019
@zone117x zone117x added this to the DX Q3 Sprint 2 milestone Sep 10, 2019
@moxiegirl

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

What is the benefit of showing warnings when generating? We get quite a few which is not confidence-inspiring. For example:

5 warnings generated.
  CC(target) Release/obj.target/secp256k1/native/secp256k1/src/secp256k1.o
In file included from ../native/secp256k1/src/secp256k1.c:13:
../native/secp256k1/src/group_impl.h:686:12: warning: unused function 'secp256k1_gej_has_quad_y_var' [-Wunused-function]
static int secp256k1_gej_has_quad_y_var(const secp256k1_gej *a) {
           ^
../native/secp256k1/src/group_impl.h:113:13: warning: unused function 'secp256k1_ge_set_gej_var' [-Wunused-function]
static void secp256k1_ge_set_gej_var(secp256k1_ge *r, secp256k1_gej *a) {
            ^
../native/secp256k1/src/group_impl.h:267:12: warning: unused function 'secp256k1_gej_is_valid_var' [-Wunused-function]
static int secp256k1_gej_is_valid_var(const secp256k1_gej *a) {
           ^
In file included from ../native/secp256k1/src/secp256k1.c:15:
../native/secp256k1/src/ecmult_const_impl.h:123:13: warning: unused function 'secp256k1_ecmult_const' [-Wunused-function]
static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *scalar) {
            ^
4 warnings generated.
  SOLINK_MODULE(target) Release/secp256k1.node

Could we suppress the warnings instead for better user experience? Many of our developers are not super experienced and those warnings can be intimidating.

app/index.js Outdated Show resolved Hide resolved
app/index.js Outdated Show resolved Hide resolved
app/index.js Show resolved Hide resolved
type: 'list',
name: 'generator_type',
message: 'Select Blockstack app output type',
choices: [

This comment has been minimized.

Copy link
@moxiegirl

moxiegirl Sep 11, 2019

Contributor

The (Use arrow keys) disappears after the one use. Is there any reason to make them disappear?

(Use arrow keys to select and press ENTER)

Many of our app mining users are beginner programmers from other countries. It helps to use complete sentences and instructions and to leave the instructions in place.

This comment has been minimized.

Copy link
@zone117x

zone117x Sep 11, 2019

Author Member

That is behavior and messaging hardcoded by what yo prompt uses: https://github.com/SBoudrias/Inquirer.js/blob/d3d632a7d94b3d719152c109f1865a8a9a2a2432/packages/inquirer/lib/prompts/list.js#L84-L86

One work around is to use raw index lists which let us customize that message, but then forces a number-based UI (arrow keys do still work):
image

Those appear to be our 2 options, I'm fine with either -- what do you think?

This comment has been minimized.

Copy link
@moxiegirl

moxiegirl Sep 11, 2019

Contributor

Yeah, I suspected this might be the case. Um, I think the number based thing is a lot more obvious than the keyboard input. Let's go with that if you can do it easy. Thanks for adjusting this.

This comment has been minimized.

Copy link
@zone117x

zone117x Sep 11, 2019

Author Member

Updated with that change ^

@zone117x

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@moxiegirl

What is the benefit of showing warnings when generating? We get quite a few which is not confidence-inspiring. For example:

5 warnings generated.
  CC(target) Release/obj.target/secp256k1/native/secp256k1/src/secp256k1.o
In file included from ../native/secp256k1/src/secp256k1.c:13:
../native/secp256k1/src/group_impl.h:686:12: warning: unused function 'secp256k1_gej_has_quad_y_var' [-Wunused-function]
static int secp256k1_gej_has_quad_y_var(const secp256k1_gej *a) {
           ^
../native/secp256k1/src/group_impl.h:113:13: warning: unused function 'secp256k1_ge_set_gej_var' [-Wunused-function]
static void secp256k1_ge_set_gej_var(secp256k1_ge *r, secp256k1_gej *a) {
            ^
../native/secp256k1/src/group_impl.h:267:12: warning: unused function 'secp256k1_gej_is_valid_var' [-Wunused-function]
static int secp256k1_gej_is_valid_var(const secp256k1_gej *a) {
           ^
In file included from ../native/secp256k1/src/secp256k1.c:15:
../native/secp256k1/src/ecmult_const_impl.h:123:13: warning: unused function 'secp256k1_ecmult_const' [-Wunused-function]
static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *scalar) {
            ^
4 warnings generated.
  SOLINK_MODULE(target) Release/secp256k1.node

Could we suppress the warnings instead for better user experience? Many of our developers are not super experienced and those warnings can be intimidating.

Yeah I hate these warnings, they are in almost all of our projects as a result of bitcoinjs-lib dependencies. bitcoinjs-lib strictly follows a Don't trust. Verify. model, and refuse to bundle pre-compiled native binaries. I think they should be able to tweak their node-gyp build config so it doesn't output useless compiler warnings... I might make a PR to their stuff that attempts that.

Could we suppress the warnings instead for better user experience? Many of our developers are not super experienced and those warnings can be intimidating.

Hmm, possibly. However, yo already does a lot of stdin/stdout redirection and manipulation to provide the terminal UI, so if it is possible to hide these I think it would be pretty brittle and hacky. For example hiding all stderr output and causing legitimate errors to be then hidden. I think we should leave it be for this PR.

On a positive note -- the plans I have so far for secp256k1 related issues may do away with this dependency all together in our repos (progress on that will be captured in blockstack/blockstack.js#703).

app/index.js Show resolved Hide resolved
@moxiegirl

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@zone117x The prompting system should be tested on Windows and Ubuntu. I attempted some tests in Windows VM but hard to tell if the issues I encountered were related to Windows or the env I was running in. Specifically, arrow keys did not work to navigate the prompts. Though, arrow keys did work in general throughout my VM.

Aborting the interactive prompts resulted in this:

image

I did use ENTER to take the default React generation in my next test. The npm succeeded with odd errors:

image

image

image

Then, after the npm start to start the hello-blockstack app failed.

image

We should be testing this in Windows/Ubuntu as well as Mac OS, this generator should work in all three and produce a generated app the compiles in all three.

It could be the env in one or the other needs to be prettty specific to actually succeeded. I had limited time to do a full test run in Windows and none at all in Ubuntu.

cc: @timstackblock

@zone117x

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Looks like a problem with your environment. Installation and running works just fine for me with mingw, aside from the interactive rendering bugs. Those happen all the time with 3rd party bash TTY emulators for Windows. All yo generators with interactive prompts have the same rendering bugs. Just tested with the most popular and maintained yo package generator-jhipster and it has the same behavior.

Windows support is widely considered to be cmd.exe and Powershell, both of which work. Also tested with Ubuntu 18.04.

@moxiegirl

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Thanks for testing. My env was definitely suspect, I was just using Git Bash.

@yknl
yknl approved these changes Sep 13, 2019
Copy link
Contributor

left a comment

Works great for me 👍

@yknl yknl modified the milestones: DX Q3 Sprint 2, DX Q3 Sprint 3 Sep 16, 2019
@zone117x zone117x merged commit 2ca4cea into master Sep 16, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
moxiegirl added a commit to moxiegirl/docs.blockstack that referenced this pull request Sep 26, 2019
Signed-off-by: Mary Anthony <mary@blockstack.com>
moxiegirl added a commit to blockstack/docs.blockstack that referenced this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.