🆕 Default Props

[jamie-lynch]

Many of the constructs defined in this library set default props to simplify the process of using them. Currently there are three different approaches to doing that.

Approach 1

Some constructs use the spread operator to merge default props, passed in props and override props in the call to super in their constructor. For example:

export class GuSSMParameter extends GuParameter {
  constructor(scope: GuStack, id: string, props: GuNoTypeParameterProps) {
    super(scope, id, {
      noEcho: true,
      ...props,
      type: "AWS::SSM::Parameter::Value<String>",
    });
  }
}

Approach 2

Some constructs define a static defaultProps object which is spread with the passed in props. For example:

export class GuApplicationListener extends ApplicationListener {
  static defaultProps: Partial<GuApplicationListenerProps> = {
    port: 443,
    protocol: ApplicationProtocol.HTTPS,
  };

  constructor(scope: GuStack, id: string, props: GuApplicationListenerProps) {
    super(scope, id, { ...GuApplicationListener.defaultProps, ...props });
  }
}

Approach 3

export class GuLogShippingPolicy extends GuPolicy {
  private static getDefaultProps(scope: GuStack, props: GuLogShippingPolicyProps): PolicyProps {
    return {
      policyName: "log-shipping-policy",
      statements: [
        new PolicyStatement({
          effect: Effect.ALLOW,
          actions: ["kinesis:Describe*", "kinesis:Put*"],
          resources: [`arn:aws:kinesis:${scope.region}:${scope.account}:stream/${props.loggingStreamName}`],
        }),
      ],
    };
  }

  constructor(scope: GuStack, id: string = "LogShippingPolicy", props: GuLogShippingPolicyProps) {
    super(scope, id, {
      ...GuLogShippingPolicy.getDefaultProps(scope, props),
      ...props,
    });
  }
}

Can we reduce this to one approach everywhere?

[jamie-lynch]

My view would be that approach 2 can definitely be removed in favour of either approach 1 or approach 3. Where there are only 1 or 2 default parameters, approach 1 is a bit cleaner but happy to go with approach 3 everywhere for consistency.

[akash1810]

Approach 3 is the only way to have access to scope whilst calling super, I think. I'm not sure how to achieve the policy in GuLogShippingPolicy using approach 2?

[jamie-lynch]

Yeah, I think approach 3 is the only way to go if we want to pick one approach.

[akash1810]

I've stumbled on another approach here https://github.com/guardian/cdk/pull/91/files#diff-00129c5d6e4b1965d9e9af4033f58a32f1d96c58727dedf41802d42253d7ccd8R13: calling addStatements within the constructor after the call to super. This is necessary in this case as it made use of the emailSenderParam variable on the line above.

I actually think this forth approach provides more clarity. WDYT?

[akash1810]

Have raised this draft PR as another demonstration of using addStatements with some even more abstraction; the result is GuDynamoDBReadPolicy is super small and simple to write. WDYT?

[jamie-lynch]

Yeah, this is nice! I guess it doesn't cover exactly the same cases though...

I really like the use case in your draft PR where now, rather than passing in a statement, you can pass actions and resources. Feels like a much nicer interface.

Also, it can be used to add extra things on top of what's passed in. So in the example from option 3, the default statement get's added but you can also pass in additional statements. This changes the behaviour slightly as it moves from being a default to be a fixture of that component which will be fine in many cases and means that you can pass in extra statements without having to pass the default in explicitly too.

export class GuLogShippingPolicy extends GuPolicy {
  private static getDefaultProps(scope: GuStack, props: GuLogShippingPolicyProps): PolicyProps {
    return {
      policyName: "log-shipping-policy"
    };
  }

  constructor(scope: GuStack, id: string = "LogShippingPolicy", props: GuLogShippingPolicyProps) {
    super(scope, id, {
      ...GuLogShippingPolicy.getDefaultProps(scope, props),
      ...props,
    });

    this.addStatement(
        new PolicyStatement({
          effect: Effect.ALLOW,
          actions: ["kinesis:Describe*", "kinesis:Put*"],
          resources: [`arn:aws:kinesis:${scope.region}:${scope.account}:stream/${props.loggingStreamName}`],
        }),
    )
  }
}

It doesn't completely cover the default props use case though so I'm not sure it's a replacement. Personally, I'd favour using this wherever appropriate but still having one of the other options for defaults - not sure which now though. Maybe if we start moving to using these methods and see what defaults are left, that would give us a clearer picture?

[jamie-lynch]

I came across a new issue today 🙃 In a number of cases, we are executing some other logic in the constructor as well as providing some default props in the super call. In those cases, merging default props with those values that have been passed in within the call to super produces a gotcha as the props value used throughout the rest of the constructor is not the same.

Consider the following scenario where we set a default in the super call but then use that prop in the constructor too

class MyClass extends ParentClass {
  constructor(scope: GuStack, id: string, props: MyClassProps) {
    super(scope, id, {someBooleanProp: true, ...props})

    if (props.someBooleanProps) // do a thing
  }
}

Instead should we be doing something more like the following or is there another, better way?

class MyClass extends ParentClass {
  constructor(scope: GuStack, id: string, props: MyClassProps) {
    const mergedProps = {someBooleanProp: true, ...props}
    super(scope, id, mergedProps)

    if (mergedProps.someBooleanProps) // do a thing
  }
}

[akash1810]

Here's a Playground with another approach that works quite well. Might need a little adjusting to cater for the scenarios that depend on the scope though.