Fixes #90 by passing flags down as args for run commands

[lukebatchelor]

Thanks heaps for the patience everyone!

The solution I ended up going with is basically what was outlined here

With each of these ways to run a script

bolt foo
bolt run foo
bolt p run foo
bolt w @dovetail/bar run foo
bolt ws --only="@dovetail/bar" run foo
bolt ws exec --only="@dovetail/bar" -- yarn foo

The way you would now pass flags is exactly as you'd expect

bolt foo --flag1 --flag2=5
bolt run foo --flag1 --flag2=5
bolt p run foo --flag1 --flag2=5
bolt w @dovetail/bar run foo --flag1 --flag2=5
bolt ws exec --only="@dovetail/bar" -- yarn foo  --flag1 --flag2=5

The only different one is the ws run command

bolt ws --only="@dovetail/bar" run foo -- --flag1 --flag2=5

The things we are trading off here are

• Complexity of code (simpler to do it this way)
• Complexity for the consumer (slightly more complicated this way)
• Ambiguity of commands (this ensures there can be no ambiguity)

The implementation

This wasn't as clean as I'd hoped. You'll see the comments I added, but basically, because meow has already parsed all our flags, there's no perfect way to pass them down accurately without reaching out and using process.argv directly.

This also makes writing tests for this a much bigger PITA. I'm willing to come back to those at some point after I do some investigations today.

Leaving this PR up now to get some feedback.

CC: @jamiebuilds @marcins @d4rkr00t @bradleyayers @drew-walker

[ajaymathur]

To me the change looks good. just one thing that the ws run command is a little different from other but you have already pointed it out. and the exec command already does that for ws.

[jamiebuilds]

This should probably be inside a util

[lukebatchelor]

As discussed, might just try removing the scriptArgs completely and removing the extra indexOf here.
At that point, its quite a simple function, still want it in a util?

[willmendesneto]

👏

[lukebatchelor]

@jamiebuilds changed those scripts to just use argv now, much cleaner. Going to merge.

Edit: Except it breaks the tests. Dammit.

So, the tests break because of how they pass flags in. Looking at it now though.... I'm wondering if that was ever quite right.... (this is workspace run command)

    await workspaceRun(
      toWorkspaceRunOptions(['foo', 'test', '--first-arg', '--second-arg'], {
        cwd: projectDir
      })
    );

The way we use toWorkspaceRunOptions looks slightly different to how it would be called in production...

else if (commandMap.WORKSPACE[command]) {
    let [workspaceCommand, ...rest] = subCommandArgs;
    let workspaceArgs = [subCommand, ...rest];
   // ... ...
  else if (commandMap.WORKSPACE_RUN[workspaceCommand]) {
      return commands.workspaceRun(
        commands.toWorkspaceRunOptions(workspaceArgs, flags)
      );
    } 

In the test we are passing in the flags as args, not flags.

i.e running something like

bolt w @atlaskit/avatar run foo --arg1 --bar --zee=1

would normally call toWorkspaceRunOptions with:

commands.toWorkspaceRunOptions([ '@atlaskit/avatar', 'foo' ], { arg1: true, bar: true, zee: 1, '--': [] })

So, we're in an interesting place.... Getting this test to pass is gonna be tricky, I'd have to mock process.argv, which seems a little dangerous... Other than that, I'd kind of want to look at rewriting this flag passing logic to maybe just give us access to the raw flags but still be able to keep typing information for flags that are recognized...

I can confirm that this change works though... ...

Any thoughts @jamiebuilds ?

[lukebatchelor]

Hey @jamiebuilds I've skipped the tests for now and I'm leaning towards just changing how we pass args. I'll leave this up for a couple more days in case you've got thoughts 👌