fix: use correct matcher in middleware.ts

[balazsorban44]

Description

The Next.js Middleware did not correctly export a matcher. There is no top-level export of matcher.

Based on

nodejs.org/middlewares/detectLanguage.ts

Line 47 in 625b2ab

matcher: request => request.nextUrl.pathname === '/',

you only want to run Middleware for the index page. This PR ensures that Middleware is not invoked unnecessarily before other pages.

Validation

Refer to the Next.js docs: https://nextjs.org/docs/app/building-your-application/routing/middleware#matcher

Check List

• I have read the Contributing Guidelines and made commit messages that follow the guideline.
• I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
• I have run npx turbo format to ensure the code follows the style guide.
• I have run npx turbo test to check if all tests are passing, and/or npx turbo test:snapshot to update snapshots if I created and/or updated React Components.
• I've covered new added functionality with unit tests if necessary.

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 8, 2023 9:25pm

[ovflowd]

Sorry, but I aint going to approve this change. I understand that this enables the static analysis at build-time, but this PR doesn't fix any issue, actually, it breaks the custom "middleware" matching we've introduced.

The whole point of the createMiddleware is allowing us to merge multiple middlewares or have one or more. Since very unfortunately, Next.js does not support more than one middleware.

[ovflowd]

Let me know if there are any alternatives or if this change is required for well, route matching to work at all, and if that's the case, I still cannot approve this PR without a solution that would allow us to have more than one matcher, or a matcher that comes from our "custom" middleware creator.

(I'm genuinely all fine with any change on the custom setup, if you feel like it, you can completely refactor this code in a way that makes you happy or that solves our issues 🙇)

Now, suppose there's no possibility at all to have something similar to what we're designing. In that case, I'd request the change to altogether remove our custom middleware engine and simply apply the only middleware we have now. (And I'd request pretty please for Vercel to consider supporting multiple middlewares)

[ovflowd]

@balazsorban44 a quick question, since the matcher is ignored, does that mean the middleware runs for every route?

Wouldn't https://nextjs.org/docs/app/building-your-application/routing/middleware#conditional-statements conditional statements still apply here? Because the docs say this:

So would we still get "billed" (aka the middleware run, even if there are these conditional statements)?

[ovflowd]

Hey @styfle sorry for pinging you! Not sure if @balazsorban44 has been busy lately. Do you have any extra insights here?

I'm definitely curious about this, because I legit see a huge amount of invocations on the Edge Middleware and I wonder if (by any chance) this would alleviate it?

[balazsorban44]

@balazsorban44 a quick question, since the matcher is ignored, does that mean the middleware runs for every route?

Wouldn't https://nextjs.org/docs/app/building-your-application/routing/middleware#conditional-statements conditional statements still apply here? Because the docs say this:

So would we still get "billed" (aka the middleware run, even if there are these conditional statements)?

Sorry, haven't seen this before now.

When you have a conditional statement within the Middleware code, it's still going to run for the route, you can just choose to not do execute any logic by returning early.

The matcher config is the one that can determine on the platform (like Vercel) when the Middleware should be skipped. This configuration needs to be static so it's predictable at build-time.

[styfle]

I legit see a huge amount of invocations on the Edge Middleware and I wonder if (by any chance) this would alleviate it?

Yes, it would reduce middleware invocations. Thats why the matcher needs to be statically analyzable, because its not invoked at runtime like the current implementation you're using.

Learn more

[ovflowd]

I legit see a huge amount of invocations on the Edge Middleware and I wonder if (by any chance) this would alleviate it?

Yes, it would reduce middleware invocations. Thats why the matcher needs to be statically analyzable, because its not invoked at runtime like the current implementation you're using.

Learn more

Ty. I've read that documentation, some parts are kinda confusing, hence I thought that dynamic matching was going to work due to making rules during the invocation. Anyhow, can you review this PR? #5613

[styfle]

@ovflowd Just to be clear, you only want middleware to run on the homepage (/) and not on other pages, correct?

[ovflowd]

@ovflowd Just to be clear, you only want middleware to run on the homepage (/) and not on other pages, correct?

Yup, at least for that one.

[github-actions]

📦 Next.js Bundle Analysis for nodejs.org

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

[ovflowd]

Closing due to #5613