feat: extract NextjsDomain from NextjsDistribution

[bestickley]

Fixes #158

[kevin-mitchell]

I checked out the branch, built it locally, and attempted to cdk synth and ended up with an error.

TypeError: The "path" argument must be of type string. Received undefined
...
...cdk-nextjs-standalone/src/NextjsDistribution.ts:424:45```

which seems to refer to this line (I think?):

const publicFiles = fs.readdirSync(path.join(this.props.nextjsPath, NEXTJS_BUILD_DIR, NEXTJS_STATIC_DIR), {
  withFileTypes: true,
});
```

I'm guessing this is user error on my part, I'm probably doing something dumb, but I promised I'd take a look this weekend and I hit this so figured I'd throw this out there.

I'll take a closer look tomorrow (maybe I'm missing a config or something).

[kevin-mitchell]

And of course indeed the issue was / is with the config changes required. I'm going to update my config to make sure I put the right configuration values where they need to go and will update again shortly..

[bestickley]

I'm guessing this is user error on my part, I'm probably doing something dumb, but I promised I'd take a look this weekend and I hit this so figured I'd throw this out there.

I think this was an issue on my end! I'm not passing nextjsPath into NextjsDistribution

[kevin-mitchell]

I think this was an issue on my end! I'm not passing nextjsPath into NextjsDistribution

that is true, I assumed that was intentional on your part. If not, then yeah that's missing!

[bestickley]

@kevin-mitchell, go ahead and try out this new version and let me know what you think

[kevin-mitchell]

@kevin-mitchell, go ahead and try out this new version and let me know what you think

Just redeployed, my certificate was added back (yay!) and things seem to work for me!

Thanks a ton for your work, sorry again I didn't end up putting up a PR for this! FYI I'm hoping to get back to paying a bit more attention to this in the next few weeks, just got a bit busy.

[bestickley]

Thanks a ton for your work, sorry again I didn't end up putting up a PR for this! FYI I'm hoping to get back to paying a bit more attention to this in the next few weeks, just got a bit busy.

No problem at all. Totally understand that open source contributions ebb and flow :)

[bestickley]

@kevin-mitchell and @revmischa, I noticed that the domainAliases property creates an L3 HttpsRedirect construct which from the docs:

The HttpsRedirect constructs creates:

• Amazon CloudFront distribution - makes website available from data centres around the world
• Amazon S3 bucket - empty bucket used for website hosting redirect (websiteRedirect) capabilities.
• Amazon Route 53 A/AAAA Alias records - routes traffic to the CloudFront distribution
• AWS Certificate Manager certificate - SSL/TLS certificate used by CloudFront for your domain

I don't think users passing in a value to domainAliases will expect all of those resources to be created from one prop, do you? What's the use case for this? The TSDoc says:

An alternative domain to be assigned to the website URL. Visitors to the alias will be redirected to the main domain. (ie. www.domain.com). Use this to create a www. version of your domain and redirect visitors to the root domain.

but if I want trying to create a www. version of my website, I don't think I'd want a separate distribution created.

Based on this "heavy abstraction", I think we should remove this prop and let users utilize the HttpsRedirect constuct directly if they so desire. My newest commit will include this change.

[bestickley]

Some other changes:

• I've improved the TS Doc comment to be more clear/detailed (IMO) so please review.
• Currently, the certificate that is created isn't a wildcard certificate. This means that alternateNames cannot be used as explained here. (Has anyone used this feature before???) Therefore I recommend we create a wildcard certificate which simply means prefixing the domain with a wildcard like *.example.com. isWildcardCertificate will control this behavior and by default will be true
• createDnsRecords was only creating DNS records for the domainName passed in, not the alternativeNames. I'd expect as a user of this construct that DNS records would be created for any domains passed into NextjsDomain so I'm now creating A and AAAA records for those alternative domain names.

These are more changes than I wanted to make, but I want all props of this construct to be functional, make sense, and offer no surprises.

@kevin-mitchell, do you mind re-testing?

[kevin-mitchell]

@bestickley testing now. Actually I just ran a diff and I see:

[~] AWS::CloudFront::Distribution prod-web/Distribution/Distribution prodwebDistribution1CB9742B 
 └─ [~] DistributionConfig
     ├─ [-] Removed: .Aliases
     └─ [-] Removed: .ViewerCertificate

but I think I'm perhaps not entirely certain what the expected config is at this point for the use case I described previously. The above result is from this config:

const nextjs = new Nextjs(this, `${props.prefix}-web`, {
      nextjsPath: "./apps/web",
      environment: {
        PARAMETERS_SECRETS_EXTENSION_LOG_LEVEL: "warn",
       ...
      },
      defaults: {
        lambda: {
          paramsAndSecrets: paramsAndSecretsLayerVersion,
        },
        distribution: {
          certificate: domainCert,
          domainNames: [
            "domain.io",
            "www.domain.io",
            "prod.domain.io",
          ],
        },
      },
    });

Note I did not include the domainProps. This is expected I think looking at the code, but just confirming.

If I do include domainProps, e.g.

const nextjs = new Nextjs(this, `${props.prefix}-web`, {
     ...snip snip...
      domainProps: {
        certificate: domainCert,
        isWildcardCertificate: true,
        domainName: "domain.io",
        alternateNames: ["www.domain.io", "prod.domain.io"],
      },
      defaults: {
        lambda: {
          paramsAndSecrets: paramsAndSecretsLayerVersion,
        },
        distribution: {
          certificate: domainCert,
          domainNames: [
            "domain.io",
            "www.domain.io",
            "prod.domain.io",
          ],
        },
      },
    });

Then this fails with

[Error at /Web-prod/prod-web/Domain] Found zones: [] for dns:domain.io, privateZone:undefined, vpcId:undefined, but wanted exactly 1 zone

I haven't stepped through the code yet but can dig deeper tomorrow. I'm guessing this is user error.

---

One small comment re:

but if I want trying to create a www. version of my website, I don't think I'd want a separate distribution created.

I'm fairly certain that's what the AWS construct does, but I think you're saying just remove the feature entirely, if somebody wants a redirect they can just add it themselves easily with this construct? That makes sense to me, I'm all for less magic.

[kevin-mitchell]

@bestickley just to follow up from my comment yesterday, but at least for my use case (and going back to what I thought you were originally intending), can you just add an OPtiONAL override default property to the NextjsDistribution props? If somebody supplies these values then they'll be used instead of the NextjsDomain derived values.

In my situation at least this allows me to completely skip over creating the NextjsDomain for my production environment by simply not supplying domainProps to the Nextjs construct.

(is there a better way of supplying a patch without opening a PR against your PR or something? I should probably know this :/)

diff --git a/src/NextjsDistribution.ts b/src/NextjsDistribution.ts
index 28dfbfb..b73f76f 100644
--- a/src/NextjsDistribution.ts
+++ b/src/NextjsDistribution.ts
@@ -1,6 +1,7 @@
 import * as fs from 'node:fs';
 import * as path from 'path';
 import { Duration, Fn, RemovalPolicy } from 'aws-cdk-lib';
+import { ICertificate } from 'aws-cdk-lib/aws-certificatemanager';
 import * as cloudfront from 'aws-cdk-lib/aws-cloudfront';
 import { Distribution, ResponseHeadersPolicy } from 'aws-cdk-lib/aws-cloudfront';
 import * as origins from 'aws-cdk-lib/aws-cloudfront-origins';
@@ -95,6 +96,16 @@ export interface NextjsDistributionProps {
    * Must be provided if you want to serve static files.
    */
   readonly staticAssetsBucket: s3.IBucket;
+  /**
+   * Explicitly set the domain names to be associated with the Distribution. Note that
+   * if this is set it will override anything created by {@see NextjsDomain }, but this may
+   * be what you want if you are managing the domains outside of the {@see Nextjs } construct.
+   */
+  readonly domainNames: string[];
+  /**
+   * Explicitly supply the certificate to be assigned to the Distribution.
+   */
+  readonly certificate: ICertificate;
 }

 /**
@@ -378,8 +389,8 @@ export class NextjsDistribution extends Construct {
       // defaultRootObject: "index.html",
       defaultRootObject: '',
       minimumProtocolVersion: cloudfront.SecurityPolicyProtocol.TLS_V1_2_2021,
-      domainNames: this.props.nextDomain?.domainNames,
-      certificate: this.props.nextDomain?.certificate,
+      domainNames: this.props.domainNames || this.props.nextDomain?.domainNames,
+      certificate: this.props.certificate || this.props.nextDomain?.certificate,

       // Override props.
       ...cfDistributionProps,

[bestickley]

Thanks for reporting the error and your suggestion. I'm on travel the next couple days. I will respond early next week.

[bestickley]

can you just add an OPtiONAL override default property to the NextjsDistribution props?

@kevin-mitchell, what's the difference between this and passing in via defaults.distribution?

[bestickley]

Also, because NextjsDomain is not created by default, what do you think about just leaving it as a separate construct that a user can instantiate if needed? This makes more sense to me than requiring the user to pass in props to top level Nextjs construct. UPDATE: Cannot do this simply/easily because NextjsDistribution needs to know details from NextjsDomain.

[bestickley]

re: the diff you saw above:

[~] AWS::CloudFront::Distribution prod-web/Distribution/Distribution prodwebDistribution1CB9742B 
 └─ [~] DistributionConfig
     ├─ [-] Removed: .Aliases
     └─ [-] Removed: .ViewerCertificate

It seems like the defaults.distribution are not being passed to the Distribution construct. Can you look at outputted CloudFormation JSON template to confirm? Will be in cdk.out/ folder.

UPDATE: I see this issue. NextjsDistribution is expecting any custom props for the underlying CloudFront Distribution construct to come in as the prop cdk.distribution see here. So you need to update your code to something like this:

defaults: {
        lambda: {
          paramsAndSecrets: paramsAndSecretsLayerVersion,
        },
        distribution: {
          cdk: {
            distribution: {
              certificate: domainCert,
              domainNames: [
                "domain.io",
                "www.domain.io",
                "prod.domain.io",
              ],
           },
         },
        },
      },

Note, I don't like defaults.distribution.cdk.distribution. I'll soon be working on Overrides which will improve this API.

[bestickley]

I added the below TS Doc to NextjsDomain:

/**
 * Use a custom domain with `Nextjs`. Requires a Route53 hosted zone to have been
 * created within the same AWS account. For DNS setups where you cannot use a
 * Route53 hosted zone in the same AWS account, use the `defaults.distribution`
 * prop of {@link NextjsProps}.
 *
 * See {@link NextjsDomainProps} TS Doc comments for detailed docs on how to customize.
 * This construct is helpful to user to not have to worry about interdependencies
 * between Route53 Hosted Zone, CloudFront Distribution, and Route53 Hosted Zone Records.
 *
 * Note, if you're using another service for domain name registration, you can
 * still create a Route53 hosted zone. Please see [Configuring DNS Delegation from CloudFlare to AWS Route53](https://veducate.co.uk/dns-delegation-route53/)
 * as an example.
 */
export class NextjsDomain extends Construct {

and this to NextjsProps:

/**
   * Props to configure {@link NextjsDomain}. See details on how to customize at
   * {@link NextjsDomainProps}
   */
  readonly domainProps?: NextjsDomainProps;

Am I missing anything?

[bestickley]

@kevin-mitchell, please test out with new props as I explain above and let me know if it works for you. I think this is ready.

[kevin-mitchell]

@bestickley first thanks as always for the work on the project.

Second, you changes worked both:

1. In my prod use case where I'm creating everything and just passing the values to the Distribution construct to have the certificate and domain names set in CloudFront. This is working perfectly now. Thank you!
2. In my dev use case, where the hosted zone is looked up and Route53 records are added to point to the new Distribution. Awesome!

RE: defaults, I have a similar question / general lack of love for the "defaults" / deeply nested values. It feels weird to me (I'll admit I haven't thought very hard about it) to have domainProps as a top level, then defaults elsewhere. But "overrides" seems like that'll address that!

Otherwise the only annoying that that I'm still not sure about is the fact that isWildcardCertificate is required even when you provide a certificate. I guess this is just a matter of it being complicated to support different use cases, but my issue here is that it's weird to have to pass in isWildcardCertificate when you're passing in a certificate that you already created. If you looked at where isWildcardCertificate is used, it's only used in the get certificateDomainName, which ultimately is only used if don't explicitly pass in a certificate:

...
if (!this.props.certificate) {
      return new Certificate(this, 'Certificate', {
        domainName: this.certificateDomainName,
        validation: CertificateValidation.fromDns(this.hostedZone),
      });
    }
...

But perhaps this is a can that could be kicked down the road?

Comments look great and are much improved IMHO.

Thanks again!

👍

[bestickley]

@kevin-mitchell, so glad they both worked!

I have a similar question / general lack of love for the "defaults" / deeply nested values. It feels weird to me (I'll admit I haven't thought very hard about it) to have domainProps as a top level, then defaults elsewhere. But "overrides" seems like that'll address that!

So domainProps I think should be separate from defaults or the future overrides because domainProps controls whether a secondary construct/feature of Nextjs is created. defaults customizes how primary constructs/features are created. We could put domainProps in overrides in future but semantically it wouldn't make sense because you're not overriding anything, you're just creating something. Make sense?

Otherwise the only annoying that that I'm still not sure about is the fact that isWildcardCertificate is required even when you provide a certificate.

I don't think this is true. If you pass in your own certificate, that prop should have no impact because isWildcardCertificate is used in the getter certificateDomainName which is only used in the construction of Certificate if certificate isn't passed in. Right?

[bestickley]

Thinking through isWildcardCertificate more, I don't think it's wise to have a wildcard certificate created by default. It can be a security issue to some and this construct should take a "secure by default" approach when possible. I've replaced it with certificateDomainName so that user can easily specify domainName when Certificate is created. I added a note on alternateNames that certificateDomainName will need to be specified for alternateNames to work. Make sense?

[kevin-mitchell]

... If you pass in your own certificate, that prop should have no impact ...

Sorry possible miscommunication on my part, but this is exactly what I'm saying. It has no impact, but is required. It's a really minor thing, just feels a bit weird to me to have a required property that isn't actually required. That said your comment on the property is clear, so I feel like it's not a problem.

I think in my mind ideally that property would only be required in a situation where the other properties passed in would be such that a certificate WOULD be created. e.g. the type to domainProps.certificate could be ICertificate | CertificateCreationOptions, where CertificateCreationOptions could be { isWildcardCertificate: boolean } - this might actually be a terrible example but hopefully the idea at least makes sense :)

Again though, I feel your comment makes it clear enough that the property isn't used if you pass in a certificate, and I'd guess your trigger finger on the merge button is getting pretty itchy by this point on this PR :)

[kevin-mitchell]

@bestickley I missed your follow up comment (sorry!), which makes my last comment irrelevant. 🚢 :shipit: :)