Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use existing distribution policies when provided #196

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

onhate
Copy link
Contributor

@onhate onhate commented Jan 5, 2024

Closes #194

@bestickley
Copy link
Contributor

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
Copy link
Contributor Author

onhate commented Jan 5, 2024

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

src/NextjsDistribution.ts Outdated Show resolved Hide resolved
src/NextjsDistribution.ts Outdated Show resolved Hide resolved
src/NextjsDistribution.ts Outdated Show resolved Hide resolved
@bestickley
Copy link
Contributor

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.

@onhate onhate requested a review from bestickley January 5, 2024 17:39
@bestickley bestickley merged commit 05a77d5 into jetbridge:main Jan 5, 2024
4 checks passed
@bestickley bestickley deleted the feat-reuse-policies branch January 5, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit exceeded for resource of type 'AWS::CloudFront::ResponseHeadersPolicy' when deploying multiple sites
2 participants