Nx: Run-Commands: Arguments are incorrectly appended when using multiple commands

[germanp173]

Current Behavior

When attempting to run multiple commands with arguments as shown in the docs, there are two scenarios:

1. Commands that use argument(s):
   
   
   
   • These run fine and are parsed correctly assuming you use {args.some_arg}
   
   
2. Commands that do not use argument(s):
   
   
   
   • These commands always get the raw arguments appended to it
     
     
     
     
     
     • Ex: Passing the argument --name=testing, the command mkdir scripts would actually run mkdir scripts --name=testing
     
     
     
     • This can cause commands to fail
     
     
     
   
   

• Note: This became a problem for us after upgrading Nx from 9.3.0 to 9.5.1.

Expected Behavior

Commands that do not use arguments should not have raw arguments appended to them as it can cause them to fail.

Steps to Reproduce

https://github.com/nrwl/nx-examples/pull/109

Failure Logs

Reference repro PR.

Environment

@nrwl/angular : Not Found
@nrwl/cli : 9.5.1
@nrwl/cypress : 9.5.1
@nrwl/eslint-plugin-nx : 9.5.1
@nrwl/express : Not Found
@nrwl/jest : 9.5.1
@nrwl/linter : 9.5.1
@nrwl/nest : 9.5.1
@nrwl/next : 9.5.1
@nrwl/node : 9.5.1
@nrwl/react : 9.5.1
@nrwl/schematics : Not Found
@nrwl/tao : 9.5.1
@nrwl/web : 9.5.1
@nrwl/workspace : 9.5.1
typescript : 3.8.3

[germanp173]

Looks like this is the current expected (undocumented) behavior as shown by the following source code:

packages/workspace/src/builders/run-commands/run-commands.impl.spec.ts:77:

  it('should add all args to the command if no interpolation in the command', async () => {
    const exec = spyOn(require('child_process'), 'execSync').and.callThrough();

    const scheduleRun = await architect.scheduleBuilder(
      '@nrwl/workspace:run-commands',
      {
        command: `echo`,
        a: 123,
        b: 456,
      }
    );

    //wait a tick for the serial runner to schedule the first task
    await Promise.resolve();
    const run = await scheduleRun;
    const result = await run.result;

    expect(exec).toHaveBeenCalledWith(`echo --a=123 --b=456`, {
      stdio: [0, 1, 2],
      cwd: undefined,
      env: process.env,
    });
  });

packages/workspace/src/builders/run-commands/run-commands.impl.ts:198:

function transformCommand(command: string, args: { [key: string]: string }) {
  if (command.indexOf('{args.') > -1) {
    const regex = /{args\.([^}]+)}/g;
    return command.replace(regex, (_, group: string) => args[group]);
  } else if (Object.keys(args).length > 0) {
    const stringifiedArgs = Object.keys(args)
      .map((a) => `--${a}=${args[a]}`)
      .join(' ');
    return `${command} ${stringifiedArgs}`;
  } else {
    return command;
  }
}

This looks to have been added as the default behavior after commands began supporting an array of strings as well as an array of objects.

Example:

// OLD docs:
"create-script": {
    "builder": "@nrwl/workspace:run-commands",
    "options": {
        "commands": [
            {
            "command": "mkdir -p scripts"
            },
            {
            "command": "touch scripts/{args.name}.sh"
            },
            {
            "command": "chmod +x scripts/{args.name}.sh"
            }
        ],
        "cwd": "apps/frontend",
        "parallel": false
    }
}

// NEW docs:
"create-script": {
    "builder": "@nrwl/workspace:run-commands",
    "options": {
        "commands": [
          "mkdir -p scripts",
          "touch scripts/{args.name}.sh",
          "chmod +x scripts/{args.name}.sh"
        ],
        "cwd": "apps/frontend",
        "parallel": false
    }
}

It still doesn't make any sense to me though why you would want to force the arguments into a command if it doesn't explicitly use interpolation.

Users should be expected to use interpolation since the documentation explicitly states to use interpolation (i.e. {args.name}) if you want to pass argument(s) to a command.

Forcing arguments regardless of the use of interpolated values breaks lots of commands.

Happy to submit a PR if my logic is correct 👌 .

[vsavkin]

There are two separate scenarios run-commands is used for:

1. Orchestrating multiple command invocation. Here interpolation makes total sense.
2. Wrapping a tool (e.g., webpack). Here you want all parameters to be passed through. Your intend here is to use the tool, but you want to do it via Nx so caching works etc..

I see the following options:

1. Keep the current behavior and add an option to not-forward:

{
    command: 'some command',
    forwardAllArgs: false
} 

1. Default arg forwarding to false and have an option to forward:

{
    command: 'some command',
    forwardAllArgs: true
} 

I think when using command (singular), forwarding by default is what you want.

       "build": {
          "builder": "@nrwl/workspace:run-commands",
          "options": {
            "command": "webpack"
          }
        }

There is not reason not to forward. You are wrapping another tool here. For multiple commands, not forwarding is probably more reasonable--cause args won't align.

Maybe we can introduce forwardAllArgs option, default it to false when using "commands" (plural).

What do you think?

[germanp173]

@vsavkin thanks for your response.
I would agree that when using command (singular) there is no reason not to forward so forwarding by default sounds good.

I would also agree that it's not reasonable to forward when using commands (plural) so introducing a forwardAllArgs option and defaulting it to false sounds good to me 👍 .

And just to be clear, this wouldn't affect interpolation correct? Meaning forwardAllArgs would default to false but you could still use args interpolation (i.e. {args.name}) in commands.

[vsavkin]

Sorry for the late reply.

Yup. It won't affect interpolation. It will only affect default forwarding.

@germanp173 would you like to send a PR making the change? I can help you out. If not, I'll make the change myself.

[germanp173]

@vsavkin no worries and I got you ... I should be able to send a PR early next week 👍

[vsavkin]

Thank you!

[cliffmeyers]

I would love to see this backported to 9.x. As far as I can tell it's not supported in 9.7.0:

An unhandled exception occurred: Schema validation failed with the following errors:
  Data path ".commands[0]" should NOT have additional properties(forwardAllArgs).