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

Middleware dependencies are incorrectly bundled #10164

Open
1 task
danielo515 opened this issue Feb 20, 2024 · 19 comments
Open
1 task

Middleware dependencies are incorrectly bundled #10164

danielo515 opened this issue Feb 20, 2024 · 19 comments
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)

Comments

@danielo515
Copy link

danielo515 commented Feb 20, 2024

Astro Info

Astro                    v4.4.1
Node                     v18.18.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/vercel/serverless
Integrations             @astrojs/tailwind
                         @astrojs/svelte       

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

I am using a middleware to handle auth. In local dev mode, everything works as expected. However, when deployed to vercel, one of the imported constants becomes noop instead of being an array.

To give you an idea of how it looks.
Here is the original middleware

import { errors, jwtVerify, type JWTPayload } from "jose";
import { defineMiddleware } from "astro/middleware";
import { TOKEN, PUBLIC_ROUTES } from "./constants";

const secret = new TextEncoder().encode(import.meta.env.JWT_SECRET);

//... etc

Then, the constants files contains:

export const TOKEN = "token";
export const PUBLIC_ROUTES = [
  "/",
  "/login",
  "/api/login",
];

However, this is the generated middleware (part)

import { jwtVerify, errors } from 'jose';
import './astro_BxjT5Sqv.mjs';
import { P as PUBLIC_ROUTES, T as TOKEN } from './prerender_BwN9fO_L.mjs'; //<- key part

// bla bla bla

And this is the file where PUBLIC_ROUTES is being imported from:

const noop = () => {};
export const P = noop;
export const T = noop;
export const _ = noop;
export const a = noop;
export const b = noop;
export const c = noop;
export const d = noop;
export const i = noop;
export const l = noop;

What's the expected result?

I expect the constants to be inlined in the generated middleware so it can properly work

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-t5xhyw

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 20, 2024
@ematipico
Copy link
Member

ematipico commented Feb 20, 2024

@danielo515, please replace the ## Astro info block with the information returned by the command astro info. This information is vital to us so we can triage your issue.

@ematipico ematipico added - P3: minor bug An edge case that only affects very specific usage (priority) feat: middleware Related to middleware (scope) and removed needs triage Issue needs to be triaged labels Feb 20, 2024
@mikenicklas
Copy link

mikenicklas commented Feb 20, 2024

I am running into a separate issue (trying to npx astro add something) and stumbled upon this crypto error. FWIW, astro info doesn't seem to be working...I'm getting the same error @danielo515 put in the issue. It's also the error I got when I tried to npx astro add something. This project is heavily under development and yesterday everything was fine 🤷‍♂️

mikenicklas@Mikes-MacBook-Pro fe % npm run astro info

> fe@0.0.1 astro
> astro info

6:02:24 AM [vite] Error when evaluating SSR module /Users/mikenicklas/Sites/Work/fe/astro.config.mjs:
|- ReferenceError: crypto is not defined
    at Module.netlifyIntegration (file:///Users/mikenicklas/Sites/Work/fe/node_modules/@astrojs/netlify/dist/index.js:16:30)
    at /Users/mikenicklas/Sites/Work/fe/astro.config.mjs:16:19
    at async instantiateModule (file:///Users/mikenicklas/Sites/Work/fe/node_modules/vite/dist/node/chunks/dep-stQc5rCc.js:54696:9)

[astro] Unable to load your Astro config

crypto is not defined
  Stack trace:
    at Module.netlifyIntegration (file:///Users/mikenicklas/Sites/Work/fe/node_modules/@astrojs/netlify/dist/index.js:16:30)
    at async instantiateModule (file:///Users/mikenicklas/Sites/Work/fe/node_modules/vite/dist/node/chunks/dep-stQc5rCc.js:54696:9)

@ematipico
Copy link
Member

This project is heavily under development and yesterday everything was fine 🤷‍♂️

Does that mean that you updated some dependency?

@mikenicklas
Copy link

mikenicklas commented Feb 20, 2024

@ematipico The only dependency change I made yesterday was via npx astro add netlify. When I tried deploying I ran into a very similar (maybe the same?) crypto is not defined bug in the netlify build logs. Found this issue and was able to get it working. Everything was fine locally the rest of the day.

Then this morning I tried to npx astro add @qwikdev/astro and saw that error for the first time in development.

@ematipico ematipico added pkg: vercel Related to Vercel adapter (scope) and removed feat: middleware Related to middleware (scope) labels Feb 20, 2024
@ematipico
Copy link
Member

It seems that the issue is around the vercel adapter. I tried with the node adapter, and the module is correctly kept in the source code. cc @lilnasy

@danielo515
Copy link
Author

danielo515 commented Feb 21, 2024

@danielo515, please replace the ## Astro info block with the information returned by the command astro info. This information is vital to us so we can triage your issue.

As someone mentioned, I can't put the output of the command because I get an error about crypto not being defined.
I was initially using the netlify adapter, but because their SSR was very slow, I moved to vercel. I didn't removed the netlify yet, so that may be causing some conflicts, I am not sure.

EDIT:

Removed netlify, re-installed, astro info still fails at the vercel adapter:

> astro "info"

1:43:03 PM [vite] Error when evaluating SSR module /Users/danielo/GIT/boda-ali-dani/astro.config.mjs:
|- ReferenceError: crypto is not defined
    at Module.vercelServerless (file:///Users/danielo/GIT/boda-ali-dani/node_modules/.pnpm/@astrojs+vercel@7.3.1_astro@4.3.7/node_modules/@astrojs/vercel/dist/serverless/adapter.js:88:28)
    at /Users/danielo/GIT/boda-ali-dani/astro.config.mjs:11:18
    at async instantiateModule (file:///Users/danielo/GIT/boda-ali-dani/node_modules/.pnpm/vite@5.1.2/node_modules/vite/dist/node/chunks/dep-Glf1enhJ.js:54696:9)

@ematipico
Copy link
Member

ematipico commented Feb 21, 2024

It's possible you're running an old version of Node.js and vercel adapter, and the polyfill isn't triggered. What's your version?

@ematipico ematipico added the needs response Issue needs response from OP label Feb 21, 2024
@lilnasy
Copy link
Contributor

lilnasy commented Feb 21, 2024

The error about crypto being undefined is fixed in astro 4.4.1.

I'm guessing you meant to place src/ap/login.ts in src/pages/api/login.ts, and in your real project you have non-prerendered routes as well, which makes the middleware run.

I see the issue with the node adapter as well. And it seems to be the same underlying bug as #8596 - rollup crudely combining user-written modules and astro naively turning the entire combined chunk into no-ops.

@danielo515
Copy link
Author

It's possible you're running an old version of Node.js and vercel adapter, and the polyfill isn't triggered. What's your version?

My node version is v18.18.0 I don't think that is old, it it?

@danielo515
Copy link
Author

I'm guessing you meant to place src/ap/login.ts in src/pages/api/login.ts, and in your real project you have non-prerendered routes as well, which makes the middleware run.

Where are you seeing that route? login.ts is inded under src/pages/api/login.ts
Screenshot 2024-02-21 at 16 11 58

in your real project you have non-prerendered routes as well, which makes the middleware run.

Yes, I noticed that the middleware runs at build time, which is super weird and probably a bug on the hybrid mode.

@danielo515
Copy link
Author

➜ pnpm ls astro
Legend: production dependency, optional only, dev only

dependencies:
astro 4.3.7

The error about crypto being undefined is fixed in astro 4.4.1.

How lucky I am 😂 can I update to 4.4.1? When was it released? my project is quite recent

@lilnasy
Copy link
Contributor

lilnasy commented Feb 21, 2024

Where are you seeing that route?

The stackblitz reproduction.

@danielo515
Copy link
Author

Awesome, updating to astro 4.4 allows me to finally run astro info:

Astro                    v4.4.1
Node                     v18.18.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/vercel/serverless
Integrations             @astrojs/tailwind
                         @astrojs/svelte

Not very useful information though, I could have told you that if you asked for my astro.config.mjs
Well, I guess one less problem! Now just need to fix the bundling one

@danielo515
Copy link
Author

Where are you seeing that route?

The stackblitz reproduction.

Oh, that is then a mistake when I made the reproduction

@lilnasy lilnasy removed needs response Issue needs response from OP pkg: vercel Related to Vercel adapter (scope) regression labels Feb 21, 2024
@danielo515
Copy link
Author

Updated the reproduction to be correct @lilnasy
You can check the output doin this in the console

npm build
cat .vercel/output/functions/_render.func/.vercel/output/_functions/chunks/prerender_Li4whjEZ.mjs

@danielo515
Copy link
Author

It's possible you're running an old version of Node.js and vercel adapter, and the polyfill isn't triggered. What's your version?

For completeness, here are all my versions:

dependencies:
@astrojs/check 0.5.4                  @lucia-auth/adapter-postgresql 3.1.0  jose 5.2.2                            tailwindcss 3.4.1                     
@astrojs/svelte 5.0.3                 @vercel/postgres 0.7.2                lucia 3.0.1                           typescript 5.3.3                      
@astrojs/tailwind 5.1.0               astro 4.4.1                           nanoid 5.0.5                          
@astrojs/vercel 7.3.1                 effect 2.3.6                          oslo 1.1.2                            
@effect/schema 0.62.8                 fp-ts 2.16.2                          svelte 4.2.10         

@danielo515
Copy link
Author

It seems the vercel adapter has some more issues. For your reference, I am reporting another problem here:
https://discord.com/channels/830184174198718474/1209861306065231902

@ematipico
Copy link
Member

Yes, I noticed that the middleware runs at build time, which is super weird and probably a bug on the hybrid mode.

No, that's expected. In hybrid mode, static generation is run by default, and you have to opt-out if you want to have a route that needs to run in SSR.

@danielo515
Copy link
Author

danielo515 commented Feb 21, 2024

Yes, I noticed that the middleware runs at build time, which is super weird and probably a bug on the hybrid mode.

No, that's expected. In hybrid mode, static generation is run by default, and you have to opt-out if you want to have a route that needs to run in SSR.

But the middleware is something runtime specific, I don't see a reason to run it at build time, not even for static pages

In any case, that is probably off topic for the scope of this issue, so let's keep it focused

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

No branches or pull requests

4 participants