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

[netlify] [middleware] [edge] custom edge middleware are not setting locals (when running at edge) #486

Closed
1 task
PaulSenon opened this issue Dec 16, 2024 · 3 comments · Fixed by #488
Closed
1 task
Assignees

Comments

@PaulSenon
Copy link

PaulSenon commented Dec 16, 2024

Astro Info

Astro                    v5.0.5
Node                     v20.18.1
System                   Linux (arm64)
Package Manager          pnpm
Output                   static
Adapter                  @astrojs/netlify (patched with #481)
Integrations             none

Describe the Bug

Following up my previous issue that was closed by #481
I did try to patch the exact same fix you suggest in #481 before even raising the issue, but I did because this fix is not fixing the middleware fully. But wasn't super clear so let's move on step by step and here is ne next step investigation

TL;DR:

#481 is not fixing the issue, just avoiding a runtime error.
Here is the correct fix:

const ctx = createContext({
    request,
    params: {},
+   locals: { netlify: {context} },
});
- ctx.locals.netlify = { context }

(Never contributed and took me already a lot of time investigating that so I might create the PR if you ask, but I'll do it tomorrow)


Note

The following is just the history of my investigation just for reference. You can skip reading !

Tests

Config Label Working
static + edge ❌ 404 Server-Islands
static + not edge ❌ 404 Server-Islands
server + edge ❌ Missing Custom Local from middleware
server + not edge ✅ Working

Test Result Analysis

  • When a server-island component (ssr) is rendered inside a static page (ssg), the server-island endpoint is not created (404)
  • When a server-island component (ssr) is rendered inside a dynamic page (ssr), the server-island works but the Astro.locals set by a custom src/middleware.ts file running at edge are ignored (the code is run from edge but has no impact on the response)
  • When a server-island component (ssr) is rendered inside a dynamic page (ssr), not at edge everything works.

Test Conclusion

it looks like there are two distinct problems.

  • One regarding the server-islands rendered inside a static page/component (export const prerender = false;)
  • One regarding the custom astro middleware (src/middleware.ts) when running at edge on netlify (edgeMiddleware: true)

Issue 1: Server island endpoint not building for static page

After further investigation, this appears only when all files in src/pages are static. And this is not a problem coming from netlify adapter as the output is static from astro:config:done hook.

Important

this must be a little edge case from Astro v5 when they merged the static and hybrid mode. Now it doesn't assume we are hybrid when we don't have any ssr page (even if they contains server-islands)

I might open an issue on Astro repo if it's not yet beeing fixed...

UPDATE: I did here: withastro/astro#12744

Tip

as a workaround simply add a unused page with export const prerender = false; and it will trick Astro to think we are no 100% static

Issue 2: Locals from custom middleware are ignored when

If we apply my workaround to fix issue 1, here is the update test table for issue 2

Config Label Working
static + edge ❌ Missing Custom Local from middleware
static + not edge ✅ Working
server + edge ❌ Missing Custom Local from middleware
server + not edge ✅ Working

So the root cause is only coming from whether the middleware is run from/built for netlify edge.
So I guess we still have an issue here regarding Netlify edge and custom middleware where

Note

The issue is only affecting the Astro.locals assignation. The custom headers written from custom middleware are working as expected !

After some further debug it looks like it comes exactly from the original issue caused by the context.locals api changes addressed in #481 but it does not fix it properly. The correct way is more this:

const ctx = createContext({
    request,
    params: {},
+   locals: { netlify: {context} },
});
- ctx.locals.netlify = { context }

What's the expected result?

whether we are in edgeMiddleware mode or not, the custom locals from custom src/middleware.ts should be passed properly to ssr pages and components.

Link to Minimal Reproducible Example

https://github.com/PaulSenon/issue-reproduction-astro-netlify-adapter

Participation

  • I am willing to submit a pull request for this issue.
@ascorbic ascorbic reopened this Dec 16, 2024
@ascorbic
Copy link
Contributor

Thanks for the report and the investigation. I'll see if I can get that fix in.

@ascorbic ascorbic self-assigned this Dec 16, 2024
@ascorbic
Copy link
Contributor

Looks like this was broken in 5.0.5 withastro/astro#12647

@PaulSenon
Copy link
Author

Thank you for the speed of fix ✨

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

Successfully merging a pull request may close this issue.

2 participants