Review of Client Commands merge

[timkinnane]

@mikaelmello - Starting this issue just to collect and resolve any questions about merging your features into the master branch. Currently your PR merged with master is available on feature/client-commands branch.

waitForClientCommands

I think this needs more comments. I don't really understand what it does in the case where it's false, it still sends respondToCommands but catches error silently. What would decide if you need to enable/disable that? Also, never a good idea to catch errors without logging or re-throwing. If the .catch(() => {}) block ends up actually catching an error, it will be near impossible to find out what’s happening and you’ll just have a silent fail state to debug without knowing where to start.

With the env settings, I think we'll change it to something like ROCKETCHAT_AWAIT_COMMANDS because we are trying to keep the prefix consistent for the RC/SDK specific settings vs those from bot frameworks.

Coding Style

I made a few changes for style consistency and lint (line length, indentation of then/catch etc). But I'm also thinking we should maybe refactor all the client commands code into it's own module, using driver but not in driver.ts. It might make it easier to merge the other branch that was done in parallel but also we need to think about users with earlier versions of Rocket.Chat where client commands was not a thing, to support some easy switch to just disable all the the command features. Maybe you've thought of that already, I just can't see where it happens.

Also, most your logs started with [ClientCommand], which should be more specific to the current function or process, so it's easy to quickly read the log headers and locate an operation.

I think the commandHandler function is too long, does too many things in one method making it hard to read and test. I think it needs a simple structure, perhaps calling methods from an object by the command.cmd.key. Then each command key callback can be tested to see it returns the result object required.

intercept-stdout

This wasn't in the package.json. I've added it, but it looks like it's not maintained. Is there a simpler alternative to using this? We always try to reduce dependencies. We also have to be careful with deps now to ensure they work client side, for the bundled version of the SDK used in React apps.

[mikaelmello]

@timkinnane first of all, what do you think about renaming from ClientCommands to ClientRequests? It was something I thought of when talking to Arthur yesterday and I don't know if it is less ambiguous. Now that I think more of it ClientRequests can be interpreted as requests coming from the client, so I'd like a second opinion on that.

waitForClientCommands

Yeah, the reason behind this was to have a way to force-wait ClientCommands (mainly to be able to test it) without stopping the bot from working in case it was an older version of RC that didn't support CCs.

Coding Style

Agree with everything, I wanted to put everything into a new module but wasn't really able to because I needed to access some things from the driver while the driver needed to also import the module.

About commandHandler, yeah I agree too. At first I only had like 3 commands to answer without the error-handling we have now so it was easier and I didn't see it growing.

intercept-stdout

It should be pretty easy to write our own intercepter. See

[timkinnane]

Sounds great. Agree that Client Requests sounds better - request is a more common term for this kind of communication between apps. It also avoids confusion in bots that use "commands" to mean user input.

You should act quickly on that if you decide to change it, to and update the core PR quickly before it's merged.

The driver/clientRequests circular dependency is tricky but maybe there's a way to refactor to work it. Could we start with the other changes and try and keep all the client request code at the bottom of the file. I'll try some things then.