Refactor custom domain logic to make actions more explicit and/or flexible

[kevin-mitchell]

Refactor custom domain logic to make actions more explicit and/or flexible

[kevin-mitchell]

@bestickley It's legit in terms of what I'm doing now, but I don't know if that means it's a common use case or frankly a "good" way of achieving my goal.

That said, I do think it's perfectly legitimate to want to allow this package to control the CloudFront Distribution but nothing in the Route53 / Certificate Manager side of things.

I might put together a small PR tomorrow (JST). I think I might have to add another configuration option though to BaseSiteDomainProps, which is perhaps not ideal - I'll take a look at least.

Thank you as always for your thoughts and time!

[kevin-mitchell]

So I started looking at this, and started breaking apart the Route53 / ACM Certificate creation "stuff" from NextjsDistribution, but realized two things:

1. The isExternalDomain option, while not entirely obvious in my case (because the domain and certificate very much are on AWS, and the certificate itself is in the same account as I'm deploying NextJs to) resolves my issue as it basically just avoids doing anything requiring a hosted zone. So for now my issue is resolved by setting this.
2. I do still think refactoring NextjsDistribution makes sense because it's doing quite a bit (as you pointed out @bestickley re: single responsibility principle), but there is a bit of a chicken and egg issue between the Distribution and the routes. You basically need to create the certificate and hosted zone first (hosted zone is required to be able to automatically do DNS validation of the certificate), then the distribution, THEN the actual Route53 DNS entries to hook up CloudFront.

This is fine i think, but I guess a follow up question is do you have any issue creating TWO new Constructs, something like (naming things is so hard :( )

NextjsDomain - validates inputs, if needed creates Hosted Zone and Certificate

and

NextjsRoute - adds the Route53 routes for the distribution, creates the redirects

I'm not sure if there is a better way to do this in terms of organizing the constructs.

[bestickley]

@kevin-mitchell, if it's legit from an AWS experience then I think we should allow it.

[bestickley]

Your use-case is not un-common. We should support this multi-account setup. I've thought before that domain/certificate logic should live outside NextjsDistribution for the sake of single responsibility principle. So if you think that makes sense I'd be open to a PR that adds a NextjsDomain that's more flexible and builds off the current solution and enables your use case.

Specifically, I'd like to see the methods: validateCustomDomainSettings, lookupHostedZone, createCertificate, and createRoute53Records moved outside NextjsDistribution into something like NextjsDomain. Additionally acm.DnsValidatedCertificate is deprecated which we're using so it would be good to "use Certificate instead" as it states in deprecation TSDoc.

[bestickley]

Splitting into 2 constructs sounds good to me because of the interdependency issue. Let me think on the naming.

[bestickley]

Or alternatively you could expose a public method addDnsRecords on a single construct, NextjsDomain, that accepts the CloudFront distribution? Something like this in top level Nextjs construct:

const nextjsDomain = new NextjsDomai(...);
const nextjsDistribution = new NextjsDistribution(...);
nextjsDomain.addDnsRecords(nextjsDistribution);

[bestickley]

Hey @kevin-mitchell,
could you try out #174 that does the work I outlined above?

[kevin-mitchell]

@bestickley I'll take a look this weekend if I can. I'm really sorry I didn't finish this, I actually started (just pushed a branch here: main...kevin-mitchell:cdk-nextjs:158-refactor-domain) but have been busy elsewhere the last two weeks.

I'll follow up ASAP.

[bestickley]

No worries, @kevin-mitchell! Thank you :)