moleculerjs / moleculer-repl

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

Adds `--local` opt to `call` cmd #66

Closed ccampanale closed 1 year ago

ccampanale commented 1 year ago

Addresses #65

Creating this PR as a draft because during implementation it occurred to me that any options added to call/dcall commands fundamentally prevent use of those options for simple action parameter passing; i.e. call v1.test.actionWithLocalParam --local would both assume the caller wanted to call the action v1.test.actionWithLocalParam on the local service broker node event though they may have intended to call the action on a remote node but with a payload of { "local": true }.

I have mixed feelings about this; on one hand this may rarely be an issue for anyone but on the other hand this could be a very inconvenient opinionation. It's somewhat similar to the con I described in the issue for detecting "local" as a valid node name as this could in rare circumstances be interesting if someone has a node in the system actually called "local".

Let me know what you think and I can make the necessary changes (or close the PR pending a better option). Thanks!

AndreMaz commented 1 year ago

Hi @ccampanale I think that it would be safer to use $ to pass opts to the call command. So instead of --local it would be --$local ( More context here: https://github.com/moleculerjs/moleculer-repl/issues/55 )

This way we avoid any potential conflicts that the --local flag may cause

ccampanale commented 1 year ago

@AndreMaz

Oh, good idea! That's just as convenient but avoids an issue with action params. I'll push up a commit when I get a chance. Thanks!

ccampanale commented 1 year ago

Again, thanks for the suggestion! I've pushed a commit to adjust the option as discussed along with an updated unit test. I feel much better about this now given it should not overlap with action parameter payloads as easily. 👏🏻

The tests are passing so I'm going to mark the PR ready for review. Please let me know if there are any questions or concerns.

AndreMaz commented 1 year ago

Nice job. Can you do a small improvement and replace in handler() the local with nodeID?

At the moment it will print that the action was called with Call <other stuff> with options: { local: true } which doesn't exist and it might confuse the users. So the in case local is passed to the handler it should be replaced with nodeID: broker.nodeID.

https://github.com/moleculerjs/moleculer-repl/blob/7ef4e61bbafc4feaf41e0322a167803106fe8e66/src/commands/call.js#L126-L128

ccampanale commented 1 year ago

@icebob Thanks!

@AndreMaz Good catch! I've added a little bit of logic to handle this and any potential future cases if they arise (please let me know if this is overkill). I pushed a second commit because I noticed that the current logic appeared to be attempting to prevent printing ... with options: { ... } if callOpts was null | undefined however, the object is always initialized as an empty object and would therefore always be printed. I tested this and confirmed that was the case so I patched it to only print this information if there are any keys on callOpts.

All tests continue to pass and the local testing I did with and without --$local appear to work wonderfully and with clean/improved output for the call command.

Please let me know if you see anything else that could/should be changed.

icebob commented 1 year ago

Released.