-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
Thanks, @onhate. Sorry to nitpick here, but I prefer simpler |
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 |
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. |
Closes #194