Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudFront returns "null" for dynamic routes in Node 20 and NextJs #43

Closed
sethcarlton opened this issue Mar 20, 2024 · 23 comments
Closed
Assignees

Comments

@sethcarlton
Copy link

sethcarlton commented Mar 20, 2024

When upgrading from ion version 0.0.169 to 0.0.170 (and confirmed the issue still exists in 0.0.188), certain dynamic routes are simply returning "null" - no logs have shown up SST Console and I haven't found anything pertinent to the cause yet

image

Downgrading to version 0.0.169 resolves the issue. From there, manually upgrading the Default Site function to Node 20 (x86) causes it again. I'm running open-next@3.0.0-rc.8


Initial discord link: https://discord.com/channels/983865673656705025/1177071497974648952/1219813905036214413

From SyncLinear.com | ION-213

@conico974
Copy link
Contributor

Could you try enabling streaming in open-next.config.ts, it seems that there is an issue with next not properly awaiting for the end before returning.
It would help if you could provide a sample repro of this issue as well.

// This is an example `open-next.config.ts` with streaming
import type { OpenNextConfig } from 'open-next/types/open-next.d.ts';

const config = {
  default: {
    override: {
      wrapper: "aws-lambda-streaming"
    }
  },
} satisfies OpenNextConfig;

export default config;
export type Config = typeof config;

@fwang
Copy link
Contributor

fwang commented Mar 21, 2024

@sethcarlton can you also try using Node.js 18 in v0.0.188, and see if that helps?

@Hazmatyre
Copy link

@conico974 this works! I copied your config into my project root.

For context, I setup an existing NextJS project with Ion v0.0.189 and considering all my routes were dynamic, all my pages on sst deploy were returning null. Dev mode works fine.

@sethcarlton
Copy link
Author

0.0.189, Node 20, No Streaming > returns null
0.0.189, Node 18, No Streaming > works as expected
0.0.189, Node 20, With Streaming > no longer shows null, but the behavior is odd. It seems like the page server component is throwing an exception before middleware runs, but this might be NextAuth working poorly. I've been having some issues with it.

@fwang
Copy link
Contributor

fwang commented Mar 22, 2024

Temporarily reverted the server function to use Node 18 until this is resolved.

@sethcarlton Can you give 0.0.194 a try?

@sethcarlton
Copy link
Author

@fwang

When on 0.0.193, the issue is happening when I have a conditional component linked inside a layout file. Removing that component gets rid of the issue, however, I still see the following in the logs no matter how simple the middleware is in Node 20:

Routing failed. TypeError: Cannot set property crypto of #<Object> which has only a getter
    at file:///var/task/middleware.mjs:18:2060

When moving to 0.0.194, initially the issue, but another exception was being thrown in the middleware and it was masking it. When I simplified the middleware down to the minimum, I am getting the "null" response (but there is another error in the logs)

I know this isn't necessarily concrete or helpful info yet, but it seems to be some combination of middleware exceptions, dynamic components, and potentially streaming. In any one of my tests that had issues there was an error in middleware.mjs so that seems to be the ultimate culprit.

Still working on getting a stripped down example to reproduce it, maybe @Hazmatyre can share theirs?

@conico974
Copy link
Contributor

@sethcarlton
Pretty sure that it has nothing to do with the middleware, this is a different issue and it should be fixed in open-next@3.0.0-rc.9 and node 20.

I haven't been able to reproduce the issue exactly (but i've managed to recreate a kind of similar issue), so everything that i say here must be taken with a grain of salt.

From what i've understood, it looks like the issue might be in node itself. Next use some detached promise to await for the finish event of the response.
In node 20 it seems that on this finish event, node is silently crashing. Everything is under a try catch so it should have returned an Internal server error and print some logs on the server but it doesn't seem to be the case.
If node is silently crashing it explains everything that's happening here.

There is a bunch of similar issue in the node repo itself.

@sethcarlton
Copy link
Author

@conico974 I'd buy that, thanks. I was just coming here to say that the middleware was a red herring. Also, my issue only occurs when navigating to a problem page directly. If it gets pre rendered by next and navigated to via a Link then it works fine.

@danielstevenberger

This comment was marked as resolved.

@sethcarlton
Copy link
Author

I tested with open-next@3.0.0-rc.9 and Node 20 and still have the same issue unfortunately

@danielstevenberger
Copy link

danielstevenberger commented Mar 23, 2024

Update:

Being on 0.0.194 and adding the open-next.config.ts mentioned above resolved the issue for me

Note: Not recommended to use streaming in production.
https://open-next.js.org/inner_workings/streaming

@conico974
Copy link
Contributor

Not recommended to use streaming in production

This is for V2, for V3 it is safe to use

@sethcarlton
Copy link
Author

Tossing some info here before I'll be busy for a couple days. The issue occurs in both Node 18 and 20 for me so I think the default can probably be set back to 20? I currently have other issues when enabling streaming so that hasn't been a resolution for me (i'm using open-next@3.0.0-rc.9)

I can get around the issue at times but it's hard to predict. Here are some scenarios that appear to cause it in my case (on page refresh only):

  • I had a client component consuming a component which used an inline 'use server' action. No build errors, but caused the runtime null
  • Fetching an avatar image in a nested component as part of a layout
  • If a page fails to find a valid route when prefetching

@conico974
Copy link
Contributor

@sethcarlton What are your issues with streaming?

@conico974
Copy link
Contributor

I've finally managed to reproduce the issue by using a client component consuming a server component as a child which used an inline use server wrapped inside a <Suspense>. Without the suspense it was working just fine.

Anyway i've found a fix for it in open-next@3.0.0-rc.10, hopefully it fixes things for all of you. I've tested it with my reproduction both in node 18 or 20.

@sethcarlton @danielstevenberger @Hazmatyre If you could give it a try, so that we can confirm that it is fixed

@conico974
Copy link
Contributor

I've made a silly mistake, i forgot to check the status code, everything returned 500 even though it was properly returning the html.

It's fixed in open-next@3.0.0-rc.11

@sethcarlton
Copy link
Author

@conico974 just tried with open-next@3.0.0-rc.11 and it works! I still have streaming disabled. When I try to re-enable it again I'll let you know if I run into any issues.

The scenario you described definitely sounds like one of the places where I was having issues. Thank you for the fix.

@danielstevenberger
Copy link

Thanks @conico974 I've confirmed it's resolved as well in open-next@3.0.0-rc.11

@sethcarlton
Copy link
Author

@conico974 as for my streaming issues, they appear to just be with next-auth (and probably unrelated to this issue as a whole). It has been giving me fits and doesn't feel stable, so I'm just going to migrate to Lucia or SST Auth.

Login/cookies don't work appropriately with streaming, it's not sending the PKCE code_verifier cookie properly.

And when I'm already logged in, if I have next-auth in middleware I get this for certain routes that deal with auth

Routing failed. TypeError: RequestInit: duplex option is required when sending a body.

@Hazmatyre
Copy link

@conico974 thanks buddy, all working with rc11.

Added openNextVersion: "3.0.0-rc.11" to my SST config and removed the open-next config with the override.

Really silly question, but where do I check for OpenNext release candidate updates? I can't find it under tags or releases.

@conico974
Copy link
Contributor

@sethcarlton If you still have issues with the middleware in V3 you can open a new issue directly in the open-next repo.

@Hazmatyre At the moment there is no place to check for this apart from a thread in the discord. I'll start posting releases directly on the open-next channel in the sst discord.

@fwang I think you can go back to Node 20 for the construct and set the OpenNext version to 3.0.0-rc.11. The issue can probably be closed as well.

@jayair
Copy link
Contributor

jayair commented Mar 26, 2024

@fwang tell me when you upgrade? I need to update the quick start.

@thdxr thdxr added the linear Created by Linear-GitHub Sync label Mar 28, 2024
@thdxr thdxr changed the title CloudFront returns "null" for dynamic routes in Node 20 and NextJs [ION-213] CloudFront returns "null" for dynamic routes in Node 20 and NextJs Mar 28, 2024
@thdxr thdxr added linear Created by Linear-GitHub Sync and removed linear Created by Linear-GitHub Sync labels Mar 28, 2024
@thdxr thdxr changed the title [ION-213] CloudFront returns "null" for dynamic routes in Node 20 and NextJs CloudFront returns "null" for dynamic routes in Node 20 and NextJs Mar 28, 2024
@fwang
Copy link
Contributor

fwang commented Mar 28, 2024

Release v0.0.226 has the update.

@jayair assigning this back to you.

@fwang fwang assigned jayair and unassigned fwang Mar 28, 2024
@jayair jayair closed this as completed in 75fca7a Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants