fix: destroy with no bundling

[onhate]

Fixes #175

this is an attempt, it worked fine on the tests I've made but I'll need help validating it more.

[bestickley]

@onhate, this looks good to me, but what about other constructs like NextjsImage? Why doesn't the same logic need to be applied there? I'm assuming it doesn't need to because in your testing it worked?

Not having tested this out, I'd think that anywhere we reference one of the nextBuilder.nextImageFnDir, etc. paths we'd need to add this check. But is that not the case?

[onhate]

https://github.com/jetbridge/cdk-nextjs/blob/main/src/NextjsStaticAssets.ts#L68 the NextStaticAsset already has this check

[bestickley]

it worked fine on the tests I've made

@onhate, How did you test? When I test this out by deleting .open-next directory and running cdk destroy with your new code I get the error:

Error: Cannot find asset at /Users/stickb/Code/cdk-nextjs/open-next/examples/app-router/.open-next/image-optimization-function
    at AssetStaging (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/core/lib/asset-staging.js:1:2119)
    at Asset (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-s3-assets/lib/asset.js:1:1080)
    at AssetCode.bind (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-lambda/lib/code.js:1:4881)
    at Function (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:9161)
    at NextjsImage (/Users/stickb/Code/cdk-nextjs/src/NextjsImage.ts:30:5)
    at Nextjs (/Users/stickb/Code/cdk-nextjs/src/Nextjs.ts:173:38)
    at HighSecurityStack (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/stack.ts:14:20)
    at <anonymous> (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/app.ts:7:1)
    at Object.<anonymous> (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/app.ts:8:62)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)

[bestickley]

I'm wondering if there is a better way to handle these cases? The goal of using the conditional, Stack.of(this).bundlingRequired, is to make it so the Next.js build process only runs when it needs to. But it's annoying to have to handle all these edge cases... However, I think it's worth it. Opening up for discussion though.

To fix NextjsImage, it may require some additional refactoring because we're using LambdaFunction as a base class so we cannot just skip calling super. Therefore, we may need to do NextjsImage extends Construct and then optionally create LambdaFunction based on Stack.of(this).bundlingRequired. What do you think?

[onhate]

The main issue is that there are no possible way to detect it it is a destroy command, the bundlingRequired (as I understood) is only true for synth and deploy any other command is false... but we also have the option to skip bundling on the construct which could be used to tell to reuse the existing bundle.

It's complicated because the framework is not helpful in this scenario.

[onhate]

another option is to build even on destroy

[bestickley]

@onhate, yes it is unfortunate. What if we created "dummy" empty version of the required directories in NextjsBuild if !Stack.of(this).bundlingRequired, skipped open-next build, and then built everything else like normal using those empty directories?

That way we'd keep logic for cdk destroy to one place instead of spreading it out in all the constructs.

[onhate]

that could be an option, I can try it, more like !fs.exists .open-next means it was not built, because we have both skipBundling and skipBuild (reuse existing build) options, so if a build exists it goes as usual, otherwise mock it to destroy (very likely it is destroy in this case)

[bestickley]

@onhate, do we want to mock if skipBuild is true? I'm not sure. I'd only mock if !Stack.of(this).bundlingRequired but could be convinced otherwise. The only fix we're trying to solve here is being able to successfully run cdk destroy if .open-next doesn't exist, right?

[onhate]

no, I mean this:

| bundlingRequired | skipBuild | Scenario                        | Action                                            |
|------------------|-----------|---------------------------------|---------------------------------------------------|
| true             | true      | deploy/synth with reused bundle | no build, check .open-next exists, fail if not    |
| true             | false     | regular deploy/synth            | build, .open-next will exist                      |
| false            | false     | destroy                         | no build, check if .open-next exists, if not mock |
| false            | true      | destroy with reused bundle?     | no build, check if .open-next exists, if not mock |

basically bundlingRequired === false mock and fail when bundlingRequired === true && skipBuild === true and .open-next is missing (do not mock in this case)

[onhate]

it worked fine on the tests I've made

@onhate, How did you test? When I test this out by deleting .open-next directory and running cdk destroy with your new code I get the error:

Error: Cannot find asset at /Users/stickb/Code/cdk-nextjs/open-next/examples/app-router/.open-next/image-optimization-function
    at AssetStaging (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/core/lib/asset-staging.js:1:2119)
    at Asset (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-s3-assets/lib/asset.js:1:1080)
    at AssetCode.bind (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-lambda/lib/code.js:1:4881)
    at Function (/Users/stickb/Code/cdk-nextjs/node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:9161)
    at NextjsImage (/Users/stickb/Code/cdk-nextjs/src/NextjsImage.ts:30:5)
    at Nextjs (/Users/stickb/Code/cdk-nextjs/src/Nextjs.ts:173:38)
    at HighSecurityStack (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/stack.ts:14:20)
    at <anonymous> (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/app.ts:7:1)
    at Object.<anonymous> (/Users/stickb/Code/cdk-nextjs/examples/high-security/src/app.ts:8:62)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)

I missed this comment, that's weird I was able to destroy... let me double check

[bestickley]

fail when bundlingRequired === true && skipBuild === true and .open-next is missing (do not mock in this case)

100% agree. thank you!

[onhate]

what a coincidence, we've been talking about this before without knowing each other https://github.com/aws/aws-cdk/issues/18125#issuecomment-1669929603

[bestickley]

@onhate, any progress on this?

[onhate]

I didn't have time yet to work on it but it's on my radar

[onhate]

I'll work on a fresh branch, closing this one