jetbridge / cdk-nextjs

Deploy a NextJS application using AWS CDK
https://constructs.dev/packages/cdk-nextjs-standalone
Apache License 2.0
273 stars 45 forks source link

feat: faster asset deployment #137

Closed bestickley closed 1 year ago

bestickley commented 1 year ago

Fixes https://github.com/jetbridge/cdk-nextjs/issues/117, https://github.com/jetbridge/cdk-nextjs/issues/113

Updates:

bestickley commented 1 year ago

Reviewers, please follow instructions in examples/README to run e2e tests to verify correct behavior. I'm still working on one issue with the HOST header with @khuezy that I need to resolve before merging.

bestickley commented 1 year ago

@kevin-mitchell, looks like you're not a part of this organization so I cannot add you as an official reviewer but would appreciate your review as well!

kevin-mitchell commented 1 year ago

Wow, this is amazing @bestickley! A huge amount of work but looks like a huge step forward in terms of removing cruft and improving the general understandability (possibly not a word) of the project. Thank you!

I took a high level look at the changes but haven't really internally processed the details at this point. I'm in the middle of a move so am sort of not in a great headspace for deeply understanding stuff quickly. THAT SAID, I did try to follow the example README (btw I think this is really nice to have for a place to sort of sanity check themselves), and ran into build issues.

Failed to compile.

./app/ssr/page.tsx
Module not found: Can't resolve '@open-next/utils'

This is probably user error or something with my local setup, looks like something I've run into in the past, but I think I fairly exactly followed the README directions so figured I'd mention it.

I'm going to try to take a closer / better look and get the example working if I can tomorrow. For now, this looks great and again thank you so much for the huge effort!

bestickley commented 1 year ago

Thank you, @kevin-mitchell! I appreciate your time reviewing. Sorry about that, I missed a step in the README. I've since updated it. You need to run pnpm build within open-next git submodule.

kevin-mitchell commented 1 year ago

Thank you, @kevin-mitchell! I appreciate your time reviewing. Sorry about that, I missed a step in the README. I've since updated it. You need to run pnpm build within open-next git submodule.

I did something dumb, which is I checked out a fresh copy of the project, followed the readme example, but I didn't actually build the main project (i.e. cdk-nextjs-standalone) before installing in the specific example. I then built the main project,reinstalled everything in the example (though worth noting this has to be forced I think?)

All of this is to say, after building the project the app-router worked / deployed!

I guess my only take away here is it MIGHT be worth mentioning in the examples readme that the actual project needs to be built before running pnpm install with the example dir. Or maybe that'll be very obvious to anybody checking out this project.


I also have a question / comment re: ## PNPM Monorepo Symlinks

I use npm 100% of the time if possible. If I absolutely must use pnpm or yarn or anything else for some reason I will, but I try not to. That said, I don't fully "get" this part of the readme to be honest. It reads like there is some extra complexity because of pnpm? Is this only relevant to those who choose to use pnpm?

bestickley commented 1 year ago

@kevin-mitchell, Definitely not dumb. I added a "Prerequisites" section in examples/README.md ensuring reader builds cdk-nextjs-standalone. Good question on PNPM Monorepo Symlinks. I added an italicized section below it that states it's only applicable is using PNPM. I use PNPM anytime I can so I wanted to call it out. It's more of a note to future developers, not for typical users of construct.