moleculerjs / moleculer-repl

REPL module for Moleculer framework
http://moleculer.services/docs/moleculer-repl.html
MIT License
27 stars 25 forks source link

Dropping Vorpal and Migrating to Shargs #48

Closed AndreMaz closed 2 years ago

AndreMaz commented 4 years ago

@Yord yesterday I've started to play with shargs-repl. I've managed to implement some basic commands (e.g., cls, exit, etc.) but I need some guidance on the call command. If possible, I want args to have the same structure as vorpal's args

Here's how my implementation looks like:

const subCommandOpt = subcommand([
    stringPos('actionName', { desc: "Action name (e.g., greeter.hello)", descArg: 'actionName', required: true} ),
    stringPos('jsonParams', { desc: `JSON Parameters (e.g. '{"a": 5}' )`, descArg: 'jsonParams'} ),
    stringPos('meta', { desc: "Metadata to pass to the service action. Must start with '#' (e.g., --#auth 123)", descArg: 'meta'} ),
    string("load", ["--load"], { desc: "Load params from file.", descArg: 'filename' }),
    string("stream", ["--stream"], { desc: "Save response to file.", descArg: 'filename' }),
    string("save", ["--save"], { desc: "Save response to file.", descArg: 'filename' }),
]);

and this is vopral implementation:

vorpal
    .removeIfExist("call")
    .command("call <actionName> [jsonParams] [meta]", "Call an action")
    .autocomplete({
        data() {
            return _.uniq(_.compact(broker.registry.getActionList({}).map(item => item && item.action ? item.action.name: null)));
        }
    })
    .option("--load [filename]", "Load params from file")
    .option("--stream [filename]", "Send a file as stream")
    .option("--save [filename]", "Save response to file")
    .allowUnknownOptions()
    .action((args, done) => call(broker, args, done));

and below are some examples of how vorpal and shargs parse the input data:


  1. Call with --load option.

Command

call greeter.data --load ./package.json --save p.json

vorpal args

{
  options: { load: './package.json', save: 'p.json' },
  actionName: 'greeter.data',
  rawCommand: 'call greeter.data --load ./package.json --save p.json'
}

shargs args

{
  _: [],
  actionName: 'greeter.data',
  load: './package.json',
  save: 'p.json'
}

  1. call with JSON string

Command

call greeter.data '{"a": 5, "b": "Bob", "c": true, "d": false, "e": { "f": "hello" } }'

vorpal args

{
  options: {},
  actionName: 'greeter.data',
  jsonParams: '{"a": 5, "b": "Bob", "c": true, "d": false, "e": { "f": "hello" } }',
  rawCommand: `call greeter.data '{"a": 5, "b": "Bob", "c": true, "d": false, "e": { "f": "hello" } }'`
}

shargs args

{
  _: [],
  actionName: 'greeter.data',
  jsonParams: '{"a": 5, "b": "Bob", "c": true, "d": false, "e": { "f": "hello" } }'
}

  1. Call with random params

Command

call greeter.data --a 5 --b Bob --c --no-d --e.f "hello"

vorpal args

{
  options: { a: 5, b: 'Bob', c: true, d: false, e: { f: 'hello' } },
  actionName: 'greeter.data',
  rawCommand: 'call greeter.data --a 5 --b Bob --c --no-d --e.f "hello"'
}

shargs args

{
  _: [ '--b', 'Bob', '--c', '--no-d', '--e.f', 'hello' ],
  actionName: 'greeter.data',
  jsonParams: '--a',
  meta: '5'
}

With a little work 1. and 2. can be transformed into the desired shape but the 3. is completely wrong. What's is the correct way of parsing unknown params? Vorpal has a function called .allowUnknownOptions() that parses unknown options that start with -- (e.g. --something 1) into

{
  options: { something: 1 },
  actionName: 'greeter.data',
  rawCommand: 'call greeter.data --something 1'
}

How can I achieve the same behavior with shargs?

Yord commented 4 years ago

@AndreMaz oh my! Must be a problem with the lexer. Good catch! I will look into it tomorrow.

Yord commented 4 years ago

I have published shargs-repl 0.7.1 that resolves the autocomplete issue.

AndreMaz commented 4 years ago

@Yord thanks for the quick fix. @icebob try again please

icebob commented 4 years ago

Thanks @AndreMaz & @Yord

My new observations, sorry :)

  1. Something strange with long help lines: image
  2. Please put an extra trailing space after mol $
  3. If autocomplete put action name and press TAB again, it lists all actions & commands: TaMaS04Sup
  4. Action autocomplete doesn't work with dcall command
  5. Are there any chance to support autocomplete for nodeID-s in dcall command? I mean autocomplete lists nodeIDs for first parameter and actions for second parameter :) Just if possible.
Yord commented 3 years ago

The first problem can be fixed by adjusting the style width in help.js:

For example by using 25 (instead of 20) for the first column and 51 (instead of 56) for the second:

const style = {
  line: [{ padStart: 2, width: 78 }],
  cols: [{ padStart: 4, width: 25 }, { width: 51 }]
}

Shargs has no function to automatically adjust these values, however, it could be written quite easily by looking at the opts and computing the max length of an option description string.

Edited to clarify: The idea would be to autogenerate a style based on the input to the help function, which is opts (the description of the command-line options). The pros are, it would adjust automatically if new actions are added. The cons are it would add additional complexity, where a fixed width may suffice.

Yord commented 3 years ago

The third problem appears to be in the way Shargs resolves positional arguments in the autocomplete function. Nice catch! I will look into it.

Yord commented 3 years ago

Are there any chance to support autocomplete for nodeID-s in dcall command? I mean autocomplete lists nodeIDs for first parameter and actions for second parameter :) Just if possible.

This one I don't understand :). Can you please elaborate @icebob?

Yord commented 3 years ago

I have had some time to look at the help lines and wrote a function that determines the column width dynamically:

This function ensures the displayed help always has enough space, and option descriptions are never cut off. It should also work for actions that will be implemented in the future. Note, though, that the function has no upper bounds for the width, so if arguments are very long, the left options column may be very long. In such a case, choosing a shorter argument name or using descArg to shorten the option description is a way out.

icebob commented 3 years ago

Good job Yord!

This one I don't understand :). Can you please elaborate @icebob?

The dcall command has two main arguments, the nodeID and the action name. Like dcall node-1 greeter.hello --name Bob I'm wondering we can add autocomplete for both nodeID and action name as well.

AndreMaz commented 3 years ago

Sorry for the delay. I've merged @Yord's commit that solves the issues with the "help" function. I've also added a trailing space after mol $.

Regarding the dcall I've added the following:

stringPos('nodeID', {
    desc: "ID of the node",
    descArg: 'nodeID',
    required: true,
    only: broker.registry.nodes.list({ onlyAvailable: false, withServices : false }).map(node => node.id)
} ),

so now it will autocomplete the nodeID. However, there seems to be an issue with 2 autocompletes in the same command because the following

stringPos('actionName', {
    desc: "Action name (e.g., greeter.hello)",
    required: true,
    only: _.uniq(_.compact(broker.registry.getActionList({}).map(item => item && item.action ? item.action.name: null)))
}),

doesn't have any effect, i.e., it doesn't do the autocomplete of actions.

Yord commented 3 years ago

I am currently working on autocomplete for the node id. It is a little more involved than I originally thought but is definitely doable. I will have more time on the WE to tackle the problem :).

AndreMaz commented 3 years ago

Hey @Yord did you have a chance to work on this?

Yord commented 3 years ago

@AndreMaz Yeah I am halfway there and hope to finish soon (maybe even tomorrow). Sorry for being a little intransparent :sweat_smile:.

Yord commented 3 years ago

Ok so I decided to publish my first fix now (for 5), while I continue working on a possible improvement that surfaced while I was analyzing 5.

  1. shargs-repl 0.7.2:\ Swap the position of only and descArg fields during autocomplete: Now, if both fields are present, only fields are preferred.
  2. molecular-repl da747bd:\ Bump shargs-repl version to 0.7.2 so only fields autocomplete earlier than descArg fields.
Yord commented 3 years ago

And in the same spirit, here is the fix for 3:

  1. shargs-repl 0.7.3:\ Fix a bug where autocomplete would make too many suggestions if the last argument was a positional argument.
  2. moldecular-repl ebb2f24:\ Bump shargs-repl to 0.7.3 to fix an issue where autocomplete would show too many suggestions.
AndreMaz commented 3 years ago

Thanks @Yord 👍 @icebob test again please

Yord commented 3 years ago

While working on the fixes I have identified a small problem with corner cases in the shargs-repl autocomplete function:

If a command had more than two positional arguments with only values and the first argument already has values supplied, autocomplete would nonetheless still suggest values for the first positional argument.

The problem here is, that I initially designed autocomplete to only look at the last supplied argument. Thus, autocomplete cannot know which options have already been provided. I am currently redesigning this by also looking at all options that have already been provided.

The problem surfaces in molecular-repl with the dcall command, which has the nodeID and the actionName positional arguments that (now) both have only values. However, I am confident to solve the issue soon and the only thing to change in molecular-repl is to bump the patch version of shargs-repl.

AndreMaz commented 3 years ago

The problem here is, that I initially designed autocomplete to only look at the last supplied argument. Thus, autocomplete cannot know which options have already been provided. I am currently redesigning this by also looking at all options that have already been provided.

Hey @Yord did you have a chance to work on this?

Yord commented 3 years ago

Hi @AndreMaz, unfortunately, other things came up and I could not have a closer look at this issue. But it is definitely on the list and I will approach it soonish ;). From the tests I have already conducted, I can say, that this issue will most probably be just a bug fix and should not break any function signatures.

That said, your question suggests this issue is important to you, so I will try and prioritize it :).

AndreMaz commented 3 years ago

Hey @Yord no rush, do your things first 😉 I was just looking at my ToDo list and remembered about this issue.

Yord commented 3 years ago

Happy new year guys! I solved the issue where the autocomplete function returned too many suggestions:

  1. shargs-repl 0.7.4:\ Fix a case where the autocompletion suggested all positional arguments instead of only the next unused one.

The only thing you need to do is to bump shargs-repl to 0.7.4.

Yord commented 3 years ago

Btw, some things have happened in shargs as I am approaching version 1.0.0, most notably in version 0.26.0. While the API has remained stable and the code should continue working as is, the underlying structure of the shargs module has changed:

shargs now depends on shargs-usage, shargs-opts and shargs-parser. This means the latter dependencies may be removed from moleculer-repl and all require('shargs-opts') should be replaced with require('shargs/opts') (the same for usage and parser).

This is a small and straight forward migration that, although not necessary for shargs to continue working, should reduce the bundle size a bit. I have written a more detailed migration guide in 0.26.0.

icebob commented 3 years ago

Awesome @Yord, Happy New Year! @AndreMaz when you have time, could you please upgrade your fork?

AndreMaz commented 3 years ago

Hey @Yord happy new year and thanks for your work 😄

UPDATE I've looked at the shargs-example-repl where you've replaced lexerSync with parserSync so I've done the same. It seems to be working.

Hoever, now there's another issue, now when I'm pressing Tab I'm getting the following error:

mol $ readline.js:1143
            throw err;
            ^

TypeError: Cannot read property 'reduce' of undefined
    at flatMap (C:\Dev\Projects\Mol\moleculer-repl\node_modules\shargs-repl\src\repl\completer\flatMap.js:1:29)
    at justValues (C:\Dev\Projects\Mol\moleculer-repl\node_modules\shargs-repl\src\repl\completer\getMatches.js:255:10)
    at getMatches (C:\Dev\Projects\Mol\moleculer-repl\node_modules\shargs-repl\src\repl\completer\getMatches.js:4:18)
    at C:\Dev\Projects\Mol\moleculer-repl\node_modules\shargs-repl\src\repl\completer\index.js:10:12
    at REPLServer.completerWrapper [as completer] (readline.js:173:18)
    at REPLServer.Interface._tabComplete (readline.js:502:8)
    at REPLServer.Interface._ttyWrite (readline.js:1048:16)
    at REPLServer.self._ttyWrite (repl.js:851:9)
    at ReadStream.onkeypress (readline.js:205:10)
    at ReadStream.emit (events.js:315:20)

OUTDATED I've updated the packages but there's a slight issue with lexerSync function.

It's being imported in /parser/index.js

const {fromArgs: fromArgsDefault, lexerSync, parserSync} = require('shargs')

const lexer = lexerSync({
  toArgv
})

const parser = cmd => rawCommand => parserSync({
  toArgv,
  argv: [equalsSignAsSpace],
  opts: [requireOpts, bestGuess, suggestOpts, groupOptions, arrayOnRepeat, adjustErrMessages],
  args: [flagsAsBools, bestGuessCast, nestKeys],
  fromArgs: fromArgs(rawCommand)
})(cmd)(rawCommand)

module.exports = {
  lexer,
  parser
}

and then used in shargs-repl.js

repl(lexer, parser, commands, {only: true, defaultAction, prompt: opts.delimiter})

However, It seems that you've removed it from the API in this commit

Yord commented 3 years ago

I've looked at the shargs-example-repl where you've replaced lexerSync with parserSync so I've done the same. It seems to be working.

Oh, I forgot to mention that 😅. But you were spot on!

Hoever, now there's another issue, now when I'm pressing Tab I'm getting the following error:

This is rather odd, it worked on my moleculer-repl copy when I tried it. But I am confident the error can be resolved. I will have a look at it tomorrow after work!

UPDATE

I may have found the error. lexer needs more arguments:

const lexer = parserSync({
  toArgv,
  toArgs:   ({errs, opts}) => ({errs, args: opts}),
  fromArgs: ({errs, args}) => ({errs, opts: args})
})

This should do the trick!