OpenNext V3 #402

conico974 · 2024-04-11T13:53:51Z

This PR is for OpenNext V3.

There is one remaining thing that may still need some changes it's the experimentalBundledNextServer options that is broken in Next 14.2 and it might be hard to make it work again. There is 2 way to handle this, we could just deprecate the options ( maintaining this could become an absolute nightmare), or we could keep it as is (it's experimental after all) and fix it for new versions when we get time.

One other things is that the e2e tests uses a custom CDK implementation, we may want to move to Ion, or we could keep the cdk implementation as some sort of reference implementation. (this is probably something we can do later)

I know that this will be very hard to properly review such a big PR, feel free to ask if you need some information on anything either here or on discord.

Last things, i think we should not squash merge this PR, and we probably want to manually edit the changelog in the Version Packages PR as well.

moved files around so that it makes more sense deleted a bunch of useless files added todo to remind myself of what i still need to do

added a deletes options in open-next plugin

changeset-bot · 2024-04-11T13:53:54Z

🦋 Changeset detected

Latest commit: 52641c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
open-next	Major
app-pages-router	Patch
app-router	Patch
tests-unit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-04-11T13:53:55Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
open-next	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 3, 2024 1:16pm

khuezy · 2024-04-11T13:59:20Z

Nice! There's a lot to review and I'll likely miss a bunch.

conico974 · 2024-04-11T14:12:05Z

Nice! There's a lot to review and I'll likely miss a bunch.

Yeah don't worry, worst case scenario we can still fix things later if needs be.
We need to stop having 2 separate versions of OpenNext, it's becoming a pain to manage. I've given up doing rebase in V3, because it was too much work to rebase like 80 commits every time. Also a lot of the issues in the repo can be solved by using V3.

khuezy

LGTM. I've only look at the code in isolation and not as the whole application, so please excuse me if my comments are irrelevant from missing context.

Regarding:
experimentalBundledNextServer => we should deprecate this for 14.2+. We should also draw the line for which version we're supporting since it'll be a headache to support all the potential diff that Vercel will introduce in 14.3+

e2e tests => We should get an ion test app up before we officially introduce v3

git history => That's fine, squashing is also dangerous if a single line was missed.

cc: @fwang would be nice if you could deploy a test app and do a quick sanity check of the basic functionality.

khuezy · 2024-04-11T14:04:08Z

docs/pages/v3/config.mdx

+      },
+      experimentalBundledNextServer: true // This enables the bundled next server which is faster and reduce the size of the server
+    },
+    pageSsr: {


Is this correct:
ssr => appDir
pageSsr => pagesDir

I'm wondering if people would easily understand/get this.

Not necessarily, you can give whatever name you want here, and you can mix app and page dir as much as you want. The ssr functions for example has a page/route3 route
You can also have as many different functions as you want

oops had a dyslexic typo, should have been is this correct, not this is correct... updated comment.

I don't know how to properly explain these things, but i think with some time and people asking questions about it, we'll figure a proper way to explain it.

docs/pages/v3/config.mdx

packages/open-next/src/build.ts

khuezy · 2024-04-11T14:48:11Z

packages/open-next/src/build/copyTracedFiles.ts

+    //Page dir
+    computeCopyFilesForPage("pages/_app");
+    computeCopyFilesForPage("pages/_document");
+    computeCopyFilesForPage("pages/_error");


Do we need to copy the 404 and 500 custom error pages? Or is that already included.
ref: https://nextjs.org/docs/pages/building-your-application/routing/custom-error

Nice catch,
Yes we do, for the created functions, we only copy the page in route.
We should probably add this inside a try catch as they are usually static or SSG and in those cases, we might not have a nft.json file

We can also make computeCopyFilesForPage async, then use await Promise.all([computeCopyFilesForPage...])
to run the fs operations in parallel to speed up the build time.

Might be worth considering if the time save is significant.

Yeah there is some room for improvement in terms of performance here. But computeCopyFilesForPage doesn't actually copy anything, it just compute every file that is necessary for a given server function.
The copying is done later and that's probably where it would help us.

At the moment it is fast enough i'd say (A lot more than the actual next build)

We could even improve the runtime performance if we do this properly.
https://github.com/sst/open-next/blob/dae9d98b0519c1ecb640b8c80d7fe0acca5b3886/packages/open-next/src/build/copyTracedFiles.ts#L189-L195

I added the 404 and 500, they can be there if the user uses getStaticProps in them.

As for the performance improvement, i think we should see later if it becomes an issue. Making everything async may cause some other trouble as we may be copying thousands of files at the same time

khuezy · 2024-04-11T14:51:31Z

packages/open-next/src/build/copyTracedFiles.ts

+  if (hasAppDir) {
+    //App dir
+    try {
+      computeCopyFilesForPage("app/_not-found");


is the not-found convention prefixed with an underscore? I don't see a leading underscore in: https://nextjs.org/docs/app/api-reference/file-conventions/not-found

On the same note, do we need to handle error? https://nextjs.org/docs/app/api-reference/file-conventions/error

Yes same as the one above, we probably need to add it

I've checked and there is no separate error file generated as part of the bundle. Given that it's a client component, i think that it is bundled as one of the chunks and imported from some next built in component.

And the not-found component is always compiled as _not-found

packages/open-next/src/build/helper.ts

packages/open-next/src/build/validateConfig.ts

packages/open-next/src/converters/aws-cloudfront.ts

packages/open-next/src/core/routing/util.ts

packages/open-next/src/plugins/edge.ts

also fix an issue when both middleware and page try to set cookies OpenNextNodeResponse also implements ServerResponse

conico974 · 2024-05-03T13:42:44Z

@khuezy I think i have to do a squash merge actually.
The main branch is protected against merge commits.
But it should be fine i've tested locally and there is no issue and we still get the history in the PR

khuezy · 2024-05-03T14:12:53Z

That should be okay, nice work! SHIP IT

conico974 · 2024-05-03T14:27:38Z

Ok let's go, let's hope i didn't mess the e2e tests

conico974 added 30 commits November 29, 2023 08:56

created basic config file

a420d9a

basic wrapper and converter implementation

c2f29a8

Minimal response writable

3379459

build config

788913e

change response to transform to allow to use pipeline

b20f0b5

fix streaming for v3

a874fd7

compression support

e7fe515

better docker handler

a5622de

add converter for apigw-v1 & cloudfront

26b3386

overridable queue

a9375ec

overridable s3 cache

423831a

overridable tag cache

70d3d64

prebuild middleware

6c10eed

refactor routing and middleware

14dbaac

big refactoring

5199eb9

moved files around so that it makes more sense deleted a bunch of useless files added todo to remind myself of what i still need to do

refactor: cleanup plugins

8413270

added a deletes options in open-next plugin

make other lambdas overridable as well

a063596

externalMiddleware

8dabff0

improve plugins

2630bb7

fix proxy request and make it work with streaming

51b5d46

bugfix

bac04a2

fix host

df1142b

refactor wrapper

53b95d8

generate basic dockerfile

5b69d2f

Only build open-next config once

a3dcb26

generate basic output file for IAC to use

a50cdf7

basic splitting

d422b0b

bundled next server

3763bab

fix external middleware cloudfront

a5995db

fix image adapter rebase

6c1cbcf

conico974 added 5 commits April 11, 2024 11:08

fix redirect lambda streaming

905a6e4

fix e2e tests

8a822cb

test: improve reliability of test for revalidateTag

fbfca43

update version

dae9d98

Sync

965a014

vercel bot deployed to Preview April 11, 2024 13:55 View deployment

khuezy requested review from fwang and khuezy and removed request for fwang April 11, 2024 13:58

khuezy reviewed Apr 11, 2024

View reviewed changes

review fix

1df7d78

vercel bot deployed to Preview April 12, 2024 08:11 View deployment

conico974 added 2 commits April 18, 2024 12:18

fix cookies in streaming

050b8f5

also fix an issue when both middleware and page try to set cookies OpenNextNodeResponse also implements ServerResponse

make all write to ddb chunked

9cb86cf

vercel bot deployed to Preview April 18, 2024 11:28 View deployment

zukilover mentioned this pull request Apr 27, 2024

Documentation Correction: Causing Error - OpenNext requires edge runtime function to be defined in a separate function. sst/sst#4764

Closed

changeset

dbb96cf

vercel bot deployed to Preview May 3, 2024 11:05 View deployment

fix e2e

52641c1

vercel bot deployed to Preview May 3, 2024 13:16 View deployment

conico974 merged commit b191ba3 into opennextjs:main May 3, 2024
3 checks passed

myles2007 mentioned this pull request May 3, 2024

v3.2.x uses the latest open-next by default, but it's not compatible with open-next@3.0.0 jetbridge/cdk-nextjs#213

Closed

conico974 deleted the feat/final-V3 branch October 29, 2024 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenNext V3 #402

OpenNext V3 #402

conico974 commented Apr 11, 2024

changeset-bot bot commented Apr 11, 2024 •

edited

Loading

vercel bot commented Apr 11, 2024 •

edited

Loading

khuezy commented Apr 11, 2024

conico974 commented Apr 11, 2024

khuezy left a comment

khuezy Apr 11, 2024 •

edited

Loading

conico974 Apr 11, 2024

khuezy Apr 11, 2024

conico974 Apr 12, 2024

khuezy Apr 11, 2024

conico974 Apr 11, 2024

khuezy Apr 11, 2024

conico974 Apr 11, 2024

conico974 Apr 12, 2024

khuezy Apr 11, 2024

conico974 Apr 11, 2024

conico974 Apr 12, 2024

conico974 commented May 3, 2024

khuezy commented May 3, 2024

conico974 commented May 3, 2024

OpenNext V3 #402

OpenNext V3 #402

Conversation

conico974 commented Apr 11, 2024

changeset-bot bot commented Apr 11, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Apr 11, 2024 • edited Loading

khuezy commented Apr 11, 2024

conico974 commented Apr 11, 2024

khuezy left a comment

Choose a reason for hiding this comment

khuezy Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conico974 commented May 3, 2024

khuezy commented May 3, 2024

conico974 commented May 3, 2024

changeset-bot bot commented Apr 11, 2024 •

edited

Loading

vercel bot commented Apr 11, 2024 •

edited

Loading

khuezy Apr 11, 2024 •

edited

Loading