Cli-microsoft365: Simplify building commands by wrapping common functionality in a base class

[waldekmastykarz]

Currently, when building commands, each command has to include pieces such as calling telemetry, checking if user is connected to a service, retrieving request digest, validating URLs, etc. We could simplify building commands, by wrapping all this into a base class.

Following this suggestion from @erwinvanhunen, I put together a proof of concept showing how this would look in practice.
SpoCommand is the base class for all SharePoint Online commands. Notice the implementation of common pieces such as requiresTenantAdmin, logging telemetry or checking connection to SharePoint. Additionally, two methods: getRequestDigest and isValidSharePointUrl are added. With this in place, a command can focus on its specific logic without implementing common pieces.

While the SpoCommand class currently defines the verbose getter, that could go to the Command class so that it's available to all commands in the CLI. For the PoC I didn't restructure everything as I just wanted to see how it would work on a few commands.

What do you think: would the gained simplification be worth the effort to refactor the existing commands? @estruyf, @wictorwilen, @andrewconnell

[erwinvanhunen]

Absolutely valuable. Having this code in all cmdlets makes it complex to change if requirements change later, also it's pretty error prone (copy and paste errors). I would absolutely move -verbose to the base class, Also think of including parameters (alike PowerShell) for error handling (failsilently, ignore, etc.)

[waldekmastykarz]

Handling errors using failsilently or ignore is something we should definitely have a look at, when implementing the non-immersive mode when multiple commands will be run as a part of a bigger script. At this moment any errors (unless exceptions) are mainly informative and doesn't prevent you from calling additional commands.

[waldekmastykarz]

I started working on this in the fork I mentioned above. Feel free to have a look if you're curious about the progress and new setup (be sure to switch to the spocommand branch rather than looking at the specific commit I linked earlier).

[andrewconnell]

In spirit, me like. However, consider the following (maybe splitting hairs)...

The base class approach works, but that's following more of the OOP model... is that the best approach. I for one would rather attempt to have listeners or have commands publish events that the rest of the CLI is listening to handle avoiding a nested inheritance mess (_ala: not following how SPFx was built_).

[waldekmastykarz]

Looking at TypeScript I thought that classes and base classes would feel natural, but maybe that's just my OOP mind. From the PoC I've made, it seemed to have sense, but it could grow significantly in scale. Theoretically, we could introduce a base class for everything which eventually would complicate the whole structure. With one base class or one per service (ie. all SPO commands one, all MSGraph commands another) we could get the balance of convenience vs. structure and not repeating common things like telemetry, auth, etc.

The event-driven model you mentioned sounds interesting. I wonder how that would fit into Vorpal commands (or just a CLI in general). Do you know of a CLI that uses that? I know that Angular CLI uses a similar thing with rx but I've seen it there only for Schematics and file system events. Also, would this support two-way communication, like the listener checking if the user is connected to SharePoint and cancelling the command if he's not? This could absolutely work for uni-directional things like telemetry or logging, where the event is emitted and the command doesn't really care how its handled (if at all).

[andrewconnell]

Sorry but I don't have any CLI examples or have much insight into your questions.

[waldekmastykarz]

Changed with #40