Generating error messages for failed commands errors out

[colin-grant-work]

Bug Description:

If a command fails to run, and one of the arguments to the handler cannot be stringified, the console shows an error about a failure of JSON.stringify, rather than a failure of the command.

Reproduction

1. Create a command with no handler
2. Make that command take a circular object (e.g. a Widget, which has a .parent field) as an argument.
3. Invoke the command
4. Check the console, see that you get an error about JSON.stringify rather than a message about the command that failed.

Additional Information

theia/packages/core/src/common/command.ts

Lines 290 to 301 in 47be972

	async executeCommand<T>(commandId: string, ...args: any[]): Promise<T | undefined> {
	const handler = this.getActiveHandler(commandId, ...args);
	if (handler) {
	await this.fireWillExecuteCommand(commandId, args);
	const result = await handler.execute(...args);
	this.onDidExecuteCommandEmitter.fire({ commandId, args });
	return result;
	}
	const argsMessage = args && args.length > 0 ? ` (args: ${JSON.stringify(args)})` : '';
	// eslint-disable-next-line max-len
	throw Object.assign(new Error(`The command '${commandId}' cannot be executed. There are no active handlers available for the command.${argsMessage}`), { code: 'NO_ACTIVE_HANDLER' });
	}

The problem is the invocation of JSON.stringify on args that can be any, including something not stringifiable.

• Operating System: RHEL
• Theia Version: 0.9.0 (master)

[kittaakos]

Related: #8035 (comment)

[colin-grant-work]

@kittaakos, thanks for linking some background. It looks like there's some debate about whether it's even worth showing the args at all.

I'd propose something like this:

    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    protected getUnhandledCommandMessage(args: any[], commandId: string): string {
        let argsMessage = '';
        if (args.length) {
            try {
                argsMessage = ` (args: ${JSON.stringify(args)})`;
            } catch {
                argsMessage = ` (args: ${JSON.stringify(args.map(arg => typeof arg?.constructor?.name === 'string'
                    ? arg.constructor.name
                    : typeof arg
                ))})`;
            }
        }
        const errorMessage = `The command '${commandId}' cannot be executed. There are no active handlers available for the command.${argsMessage}`;
        return errorMessage;
    }

It's fair number of lines just to get the message, but it should be safe and reasonably informative.

[kittaakos]

I'd propose something like this:

Feel free to drop args from the log and we are done.