jetbridge / cdk-nextjs

Deploy a NextJS application using AWS CDK
https://constructs.dev/packages/cdk-nextjs-standalone
Apache License 2.0
260 stars 43 forks source link

feat: use existing distribution policies when provided #196

Closed onhate closed 6 months ago

onhate commented 6 months ago

Closes #194

bestickley commented 6 months ago

Thanks, @onhate. Sorry to nitpick here, but I prefer simpler if statements as I laid out in previous PR instead of custom Mutable type (although that's a cool trick) with casting. I think if statements are simpler to understand what's going on and more approachable for new devs to the codebase. Do you agree?

onhate commented 6 months ago

in this one I will disagree, I see this as simpler solution because you avoid creating the extra overhead of the let variables that overrides the overrides, see.

return {
      ...this.commonBehaviorOptions,
      origin,
      allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL,
      originRequestPolicy: cloudfront.OriginRequestPolicy.ALL_VIEWER_EXCEPT_HOST_HEADER,
      cachePolicy, // overrides the overrides
      edgeLambdas: this.edgeLambdas.length ? this.edgeLambdas : undefined,
      functionAssociations: this.createCloudFrontFnAssociations(),
      responseHeadersPolicy, // // overrides the overrides
      ...this.props.overrides?.serverBehaviorOptions, // if those properties are being set isn't it confusing that this is overriding all?
    };

The 'mutable' approach is more like, try the defaults, if something is missing, then fallback.

But it's ok for me, I can change it for keeping the standards

bestickley commented 6 months ago

I see this as simpler solution because you avoid creating the extra overhead of the let variables that overrides the overrides

I see your point. But IMO, less code does not alway mean simpler. I think less code is often better, but not at cost of simplicity. Extreme example, but this is why we don't use minified code as source code :) Appreciate your flexibility.