add min max required commands for generate and db

[thedavidprice]

When using rw db and rw generate, we do not validate the input mix/max commands. This means no error is thrown if too many commands are incorrectly used -- and in some situations the command runs with unexpected behavior.

tl;dr How to Fix

We need to add Yargs min/max demandCommand() validation:
https://github.com/yargs/yargs/blob/master/docs/api.md#demandcommandmin1-max-minmsg-maxmsg

Example: User adds space to directory name

If a user adds a directory name incorrectly, the generator runs with unexpected behavior.

$ yarn rw g cell blog posts
yarn run v1.22.0
$ /Users/price/Repos/redwoodblog/node_modules/.bin/rw g cell blog posts
  ✔ Generating cell files...
    ✔ Writing `./web/src/components/BlogCell/BlogCell.js`...
✨  Done in 1.26s.

We can set a min/max requirement by adding yargs…demandCommand(2, 2, …). This returns an error with help text and explanation:

$ yarn rw generate cell oh yeah baby
yarn run v1.22.0
$ /Users/price/Repos/redwoodblog/node_modules/.bin/rw generate cell oh yeah baby
rw <command>

Commands:
  rw generate cell <name>         Generate a cell component.
  ...
  rw generate service <model>     Generate a service object.

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]

You entered too many commands. Maybe you used a space in your directory name?

Special Handling for rw db save and rw generate page

The current PR changes work for all cases except:

1. rw db save, which should be set to demandCommand(1, 2, …)
2. rw generate page, which should be set to demandCommand(2, 3, …)

I’m not familiar enough with Yargs to know where to override the config in generate.js and db.js within page.js and save.js respectively.

TO DO:

• handle max commands set to 2 for db save
• handle max commands set to 3 for generate page

[peterp]

I don't think we have a problem with yarn rw db save since the name argument is optional and it accepts spaces:

yarn rw db save i added a yoyo field
yarn run v1.21.1
$ /Users/peterp/Personal/redwoodjs/example-todo/node_modules/.bin/rw db save i added a yoyo field
Creating database migration... [started]
📼  migrate save --name i,added,a,yoyo,field

Local datamodel Changes:

// Define your own models here and run yarn db:save to create
// migrations for them.

model Todo {
  id Int @id @default(autoincrement())
  body String @unique
  status String @default("off")
  yoyo String
}

Prisma Migrate just created your migration 20200223093555-i-added-a-yoyo-field in

migrations/
  └─ 20200223093555-i-added-a-yoyo-field/
    └─ steps.json
    └─ schema.prisma
    └─ README.md

Run prisma2 migrate up --experimental to apply the migration

Creating database migration... [completed]
✨  Done in 0.91s.

Prisma converts the spaces into dashes.

I did see that whilst we've marked the name argument as optional we don't filter it out from the command that we send to Prisma, so the name becomes undefined, maybe we should do something like this:

- args: ['migrate save', `--name ${name}`, '--experimental'],
+ args: ['migrate save', name && `--name ${name}`, '--experimental'].filter(Boolean)

[peterp]

I think setting the min/max values for all the child commands might be a bit too restrictive; perhaps we can look at adding something like coerce to the actual command in the builder:

redwood/packages/cli/src/commands/generate/commands/page.js

Lines 35 to 36 in 02dce70

	export const builder = { force: { type: 'boolean', default: false } }

[thedavidprice]

Roger that re: “coerce”. Also, realized I have a good friend who’s core contrib on Yargs (small world). Going to ask him in general what some best practices are here — primarily for my learning. But will report back with applicable updates.

[bcoe]

I think setting the min/max values for all the child commands might be a bit too restrictive; perhaps we can look at adding something like coerce to the actual command in the builder:

Because demandCommand() is at the top level, it will enforce a positional argument count for every command, so I agree that this might be a bit too restrictive.

I might be tempted to use .check for the builder of specific commands, it would look something like this:

require('./')
  .command('cmd <args>', 'example command', (yargs) => {
    yargs.check((argv) => {
      if (argv._.length > 1) {
        return Error('too many argument provided after args')
      } else {
        return true;
      }
    })
  }, (argv) => {
    console.info('succes')
  }).argv

The nice thing about this is that you can get very specific about requirements for a given command.

[thedavidprice]

Thanks @bcoe 🚀

I’ll take a look at implementing once the dust settles from current push.

[thedavidprice]

Further reminder+note to my future self. We need better error feedback/handling. This is on me, but it took me way too long to figure out what happened cause visually just looks like it ran when I’m quickly scanning the terminal:

What happened? Nothing.

[peterp]

@thedavidprice Hey! I pushed this forward a bit, I kept getting annoyed by the fact that I would enter the wrong command yarn rw gg page MyPage and I wouldn't get any output, now you get an error:

rw <command>

Commands:
  rw build [app..]    Build for production.
  rw db <command>     Database tools.                        [aliases: database]
  rw dev [app..]      Run development servers.
  rw generate <type>  Save time by generating boilerplate code.     [aliases: g]
  rw info             Prints your system environment information
  rw lint             Lint your files.
  rw open             Open your project in your browser.
  rw test [app..]     Run Jest tests for api and web.

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]

Examples:
  yarn rw g page home /  "Create a page component named 'Home' at path '/'"

Unknown arguments: gg, page, rabbit
error Command failed with exit code 1.

[thedavidprice]

cc @jamesgeorge007

[thedavidprice]

Status Update: testing recent revision with .strict() breaks commands like yarn rw g … with multiple options.

[thedavidprice]

@bcoe We are running into problems with .strict() conflicting with commands that use Positional Arguments. It’s most likely our implementation that’s causing the issue (and not your code 😄 ) But if you have some time, we would greatly appreciate some guidance from you about how to move this forward.

Goal

Our Yargs commands should give an error:

1. if user inputs an unknown command or unknown options
2. if user does not input required args for a given command or if user inputs too many commands

Implementation

We added .strict() method to our top level composition in index.js here.

We also added .strict() method to the top level of a command directory for ‘generate’.

But ’.strict()` results in errors for commands with positional Args

Here are our redwood generate commands (alias rw g):

Commands:
  rw g cell <name>                Generate a cell component.
  rw g component <name>           Generate a component component.
  rw g layout <name>              Generate a layout component.
  rw g page <name> [router-path]  Generates a page component.
  rw g scaffold <model>           Generate pages, SDL, and a services object.
  rw g sdl <model>                Generate a GraphQL schema and service object.
  rw g service <name>             Generate a service component.

With .scrict(), running any generate command results in an error:

 $ yarn rw g page FooBar  
...
Unknown argument: FooBar
error Command failed with exit code 1.

A potential clue

It’s possible there is something wrong with our directory structure for ‘generate’ (and also for ‘db’). The top level file ‘generate.js’ is here. And the directory is here.

We are unable to print ‘—help’ output more granular than the ‘generate’ command itself (see example output above). For example, ‘yarn redwood generate —help’ produces the same output as ‘yarn redwood generate page —help’. The page argument has several options including ‘url’ (the router-path) and ‘—force’, for which we’d like to display help information.

Attempted Fix for yarn redwood generate page command

For the generate command arg page (see page.js), here’s the current builder:

export const builder = {
  'router-path': {
    type: 'string',
    required: false,
    description:
      'The path used to reference this page in the router: `/about-us`, `/home`, etc...',
  },
  force: { type: 'boolean', default: false },
}

I attempted to update this using both .positional() and ‘.option(), which unfortunately did not resolve our issues with .strict()` errors for this command:

export const builder = (yargs) => {
  yargs
    .positional('name', {
      desscription: 'Name of the page to generate',
      type: 'string',
    })
    .positional('router-path', {
      describe:
        'The path used to reference this page in the router: `/about-us`, `/home`, etc...',
      type: 'string',
    })
    .option('f', {
      alias: 'force',
      type: 'boolean',
      default: false,
      description: '--force to overwrite existing page and route',
    })
}

Help with next steps

Is is possible we did not implement our Command Directory structure correctly?

Or perhaps .strict() cannot be used for our case?

Thanks again for any/all help, Ben. Yargs has been a fantastic resource 🚀

[bcoe]

@thedavidprice I introduced strictCommand recently, which is meant to strictly enforce that you used a known command, I wonder if this is closer to what you want?

This feature we could flesh out further too, and perhaps make sure it does what you're hoping for.

If you can find any isolated reproductions for some of this stuff, and send a failing test to yargs, I'm happy to prioritize work for redwoodjs.

[thedavidprice]

Thanks for taking a look @bcoe 🏆

I'll definitely re-try strictCommand and let you know if I have any further specific questions that aren't answered in your docs.

At this time, I think any issues are likely in our implementation. But of course I'll send anything your way, including reproductions, if they arise.

Lastly, grateful in advance for your offer to prioritize work for Redwood if/when it's needed. Truly.

[thedavidprice]

.strictCommands() still results in error. I attempted using at two levels: “index.js” and “generate.js” (top level of generate directory).

yarn rw g page Foo returns error Unknown command: Foo

yarn rw g page returns error Not enough non-option arguments: got 0, need at least 1

[thedavidprice]

Possibly related. Not a fix, I don't think, but could help diagnose:
yargs/yargs#1625
PR here --> yargs/yargs#1626

[cannikin]

@thedavidprice This one hasn't seen any movement in two weeks, is it still valid?

[thedavidprice]

Yes, it is. Peter and I have worked on it behind the scenes. It's needed but will likely take re-structuring the Yargs builder and/or directory structure. I'm thinking on it and have some ideas. But gonna take some time + focus...

[thedavidprice]

@bcoe Non-urgent update here. Our issue with having multiple command directories work with strict() has been fixed here #555

Turns out it was a problem caused by using .argv at multiple levels (i.e. not solely at our top-level index.js). We have two separate sub-command options, db.js and generate.js, each having an associated directory of options.

Effectively, the Yargs example instruction for cli.js in the Advanced.md commandDir() section only works if cli.js is the top level command.

Is this something in the documentation we just missed? And/or would it help if I added a note/warning/example about how to implement for a case like ours?

tl;dr;

The fix was simply removing .args from the builder of db.js and generate.js:

// db.js
- yargs.commandDir('./dbCommands').demandCommand().argv
+ yargs.commandDir('./dbCommands').demandCommand()

// generate.js
- yargs.commandDir('./generate', { recurse: true }).demandCommand().argv
+ yargs.commandDir('./generate', { recurse: true }).demandCommand()