fix: Added `shell: true` option to `spawnSync`

[arthur-cheung]

…rectly.

[arthur-cheung]

It pretty much mirrors this: https://github.com/nodejs/help/issues/728

Basically, when I ran severless syncToS3 there was nothing returned from spawnSync so at this point here (https://github.com/serverless/examples/blob/master/aws-node-single-page-app-via-cloudfront/serverless-single-page-app-plugin/index.js#L48) it falls over with TypeError: Cannot read property 'toString' of null as result is null.

I changed spawnSync to execSync to see what it was actually trying to do. And I get the following error from severless:

  Error: Command failed: aws --region ap-southeast-2

  usage: aws [options] <command> <subcommand> [<subcommand> ...] [parameters]
  To see help text, you can run:

    aws help
    aws <command> help
    aws <command> <subcommand> help

  aws: error: the following arguments are required: command

So it looks like it's failed to parse the commands, but I'm not 100% sure.

Following issue I linked up top, I tried adding { shell: true } and it then worked.

I'm on Mac with zsh

ProductName:    macOS
ProductVersion: 11.4
BuildVersion:   20F71

zsh 5.8 (x86_64-apple-darwin20.0)

Any thoughts or insight?

[pgrzesik]

Thanks for sharing more context - I think the problem is with this logic instead of the need to add shell: true there. I think the logic that calls toString on stdout should first check if stdout is defined, same with stderr

[arthur-cheung]

I think that is true it should be null protected, but I think it should always return a result? I actually tried just adding protection in initially, but it it wasn't running the aws-cli command correctly.

EDIT: stderr actually didn't have a value either in that instance. So both stdout and stderr had nothing.

[pgrzesik]

That's interesting why both stdout and stderr had nothing, but now I'm looking into the code, I beleive that whole logic of executing command is flaved. If command is aws only without actual command, wouldn't it always fail? I think accepting the change you're proposing doesn't really solve the underlying issue, correct?

Could you clarify what do you mean by

Following issue I linked up top, I tried adding { shell: true } and it then worked.

what exactly worked?

[arthur-cheung]

Running severless syncToS3 worked. It updated my changes to S3 if I pass { shell: true } as a third argument to spawnSync. Without it, it falls over without stdout or stderr in the result. result did have a property error though, which you can see in my screen capture attached.

[arthur-cheung]

Also, s3 is added as an argument to spawnSync. You can see it in the function syncDirectory:

// syncs the `app` directory to the provided bucket
  syncDirectory() {
    const s3Bucket = this.serverless.variables.service.custom.s3Bucket;
    const args = [
      's3',
      'sync',
      'app/',
      `s3://${s3Bucket}/`,
      '--delete',
    ];
    const { sterr } = this.runAwsCommand(args);
    if (!sterr) {
      this.serverless.cli.log('Successfully synced to the S3 bucket');
    } else {
      throw new Error('Failed syncing to the S3 bucket');
    }
  }

This is why I suggested up the top that it might be an issue with spawnSync parsing the arguments. As I think it might just doing aws --region ap-southeast-2 when { shell: true } isn't passed in - at least for me in my environment.

[pgrzesik]

Thanks for the clarification, I'm still not sure about the root cause, but unfortunately, I don't have the capacity to dive deeper into this problem. Given that, let's merge your proposal as it seems to resolve the issue and should not break anything.

[arthur-cheung]

Thank you!