RFC: overrides

[bestickley]

Hi, I'd like to get the community's thoughts on a new top level prop called overrides. This prop would enabled overriding the props passed to every construct instantiated in Nextjs and would be likely remove the need for defaults. This would close multiple issues (#118, #104, #73) requesting greater levels of customization. Here is the gist of my proposal:

import * as cloudfront from 'aws-cdk-lib/aws-cloudfront';
/**
Customize every construct instantiated within `Nextjs`. Props specified here will be deep merged into targeted construct overriding default props. Warning: `overrides` may or may not break intended behavior. Use with caution.
*/
interface NextjsOverrides {
  distribution?: {
    construct?: cloudfront.DistributionProps;
    staticBehavior: {
      cachePolicy?: cloudfront.CachePolicyProps;
      construct?: cloudfront.BehaviorOptions;
      responseHeadersPolicy?: cloudfront.ResponseHeadersPolicy;
      ...
    };
    ...
  };
  ...
}

cc: @revmischa, @kevin-mitchell, @khuezy

Note: Partial<> versions of types needed for proposal can be generated. See here.

[khuezy]

Great idea, I would propose making the cache policy more user friendly. Usually you'd just want to pass a list of headers/cookies/queries that you can to forward to the origin. So instead of having the user create the CachePolicy construct, just let them pass in an array of strings for each of the caches.

[bestickley]

@khuezy, great! To customize the cache policy a user would need to do something like:

new Nextjs(this, "Nextjs", {
  overrides: {
    distribution: {
      staticBehavior: {
        cachePolicy: {
          queryStringBehavior: cloudfront.CacheQueryStringBehavior.allowList('...'),
          headerBehavior: cloudfront.CacheHeaderBehavior.allowList('...'),
          cookieBehavior: cloudfront.CacheCookieBehavior.allowList('...'),
        },
      },
    },
  },
});

That doesn't seem too bad to me. We could potentially in the future provide a helper class on top of this like NextjsOverrides that access simpler options but I think giving user full customization first is best.

[khuezy]

Yea that's not bad, but if you compare it to this: headerBehavior: ['next-url', ...], then it becomes (relatively) bad.

[bestickley]

Fair point. The way I see it, this feature would lay the ground work for additional features that make configuration simpler. For example, we could create a NextjsOverrides class that has a fluent API with methods that allow for simpler configuration of overrides. For example:

new Nextjs(this, "Nextjs", {
  overrides: new NextjsOverrides(this, "NextjsOverrides").addStaticBehaviorAllowedHeaders('next-url'),
  },
});

In summary, as long as we have the ability to configure each construct, we can make it easier later. Since we're about to releave v4 and are in pre-release, I want to get this done now to prevent another breaking change soon.

[khuezy]

imo, that's still a bit too much for the consumer of the Nextjs construct. If I were designing it, I'd let overrides be just a plain typed object w/ the attributes that the user cares about, they shouldn't need to know about the CDK constructs.
Just my 2c

[bestickley]

@khuezy, I agree that it may be too much for some consumers of the construct. For those users, they can use the simplified API. Different users will care about different aspects of the construct, so it's hard to know what users will care about. We can also create multiple simplified overrides for different types of users: i.e. users who are more security conscious or users who are more performance minded. My proposal for overrides provides a base that we can build more targeted abstractions on later.

[kevin-mitchell]

Overall this seems like a nice option to have - being able to customize everything would likely save people form having to fork this project to deploy exactly what they need. What you propose seems like a very positive thing, period.

I guess my only general high level question are people who really need to customize the way these Constructs work going to just fork the project anyway? Being able to override everything is great, but at the same time I feel like there could be an argument to just provide customization hooks / overrides for (as an example) the top 5-10 things people might actually want to customize and provide solid documentation around how / why you might customize those things?

Domain name setup, caching behavior, Lambda runtime / performance / scaling configuration, etc (those are just things that come to mind). Provide meaningful customization hooks for 75% of users who might not need absolute control over everything, and provide useful off-boarding instructions (tbqh not sure what that means here 😂) for those who might need to go deeper?

[bestickley]

Kevin, thanks for the feedback. I agree with you, I cannot imagine why a user would want to be able to customize the lower level constructs, but they may. I don't think it would be a very heavy lift to implement, especially when we give the caveat that this is advanced customization level and user is responsible if the break functionality by overriding important props.

I doubt users will want to off-board completely and have to implement all of these constructs themselves. I think they'll want to continue using this construct, they'll just want customize a construct for their specific use case.

[kevin-mitchell]

This makes sense, and it would be great off people didn't off-board / fork, but could do everything they needed with this project.

I'm just thinking out loud here: Maybe the intersection of people who know about CDK are choose to use this over SST and people who aren't super comfortable with basic AWS infra stuff is pretty small... but just having some gentle "here are some top / likely customizations you might be interested in" type content in the readme might be nice. Possible performance tuning stuff, or "here is how to get a domain name setup with this project in a sane way", etc.

Anyway, love the idea of being able to do everything within the framework these constructs provide.

[bestickley]

You read my mind a month ago :) ! See Customization section of the docs here. If you have any ideas for customization use cases, please contribute!

[revmischa]

I think it's a fine idea, maybe it could match the cdk: { distribution: { ... } } format of SST even.
The current types are broken due to jsii limitations so this would be a better solution than what's in place now

I would just say make it clear if the overrides are being merged with existing options or overwriting the defaults.

[bestickley]

I'd be ok with matching SST, but semantically I don't think cdk makes sense. For SST it does, because it's telling you that your customizing the CDK constructs under the hood (which the SST abstracts aware normally). Concur with the types.

overrides will always be deep merged so they have complete control but could also break default functionality. I agree I should make that explicit in JS Doc comment. Will update.

[bestickley]

Released in https://github.com/jetbridge/cdk-nextjs/releases/tag/v4.0.0-beta.18