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

Deprecate page parameter for Middleware #34521

Closed
javivelasco opened this issue Feb 18, 2022 · 7 comments · Fixed by #37349
Closed

Deprecate page parameter for Middleware #34521

javivelasco opened this issue Feb 18, 2022 · 7 comments · Fixed by #37349
Labels
Middleware Related to Next.js Middleware.

Comments

@javivelasco
Copy link
Member

Currently, in Middleware, NextRequest holds a page property that can be used to foresee if the incoming request will match a page or not. This feature is intended to help users detect if a certain page will be render and, in the case of dynamic routes, which ones are the parameters that they will be matching.

This information is based on simply checking if there is a static/dynamic page route that matches the request. The problem is that when the request matches a static file, it will still check if there is a page match and it is not possible to detect if there is a static file that matches at that specific stage.

This feature can be replaced by using path-to-regexp or any other library or regexp and it is adding logic to the middleware bundle that many users may not need, growing its size when there is no need to.

We should entirely remove the feature so that when request.page is accessed we can fail with an error and a link that provides a code snippet with an alternative to the feature.

@tavvy
Copy link

tavvy commented Feb 18, 2022

The rest of your comment about path-to-regexp makes sense, but could I ask for some clarity on how one would go about retrieving the page / route manifest from within a custom middleware?

I've noted anecdotally that request.page is not reliable, in that for some paths that I know are served by Next I am getting { name: undefined, params: undefined }. For this reason I was about to rewrite my own path-to-regexp style logic anyway, which is how I stumbled upon this issue.

@javivelasco
Copy link
Member Author

That's good to know @tavvy as it confirms that this feature is not as useful as initially expected. What I mean referring to path-to-regexp is that the feature simply relies on checking against all dynamic routes and usually a user would use it check wether it matches or not a specific page. Being that the case one can transform, i.e. /dashboard/:slug to a regexp and check against request.nextUrl.pathname to see if it is matching or not. Within that case one could also discard static file matches.

We can also add a snippet for a helper in the error link to actually make it worth with the exact Next.js page route like pages/dashboard/[slug].

@tavvy
Copy link

tavvy commented Feb 18, 2022

Thanks @javivelasco and I can see both the problem and the solution you are proposing now.

Perhaps our use case is a little unique so let me explain as it may be a valid use case of the feature;

We use rewrites to fallback to a legacy system. Allowing incremental adoption of Next.js.
_middleware runs before everything so that includes before rewrites / redirects (although this behaviour differs between vercel dev and when actually deployed ... thats one for another day).

We would like a "global" root level middleware to handle session setting. But we only want to do this for the Next.js handled pages, when we rewrite to the legacy system it performs its own session handling that we do not want to clash with.

The easiest and most intuitive way to achieve this was to stick an "early exit" handler at the top of the middleware.

if (!request.page.name) return NextResponse.next();

// ... session setting logic 

Im sure there are different variations on this usage. Basically it is useful to have some kind of indicator that the page is a known route to the Next.js app.

Hence my question about how I might access the page manifest from within the middleware to construct my own path-to-regexp matching logic, otherwise I'll need to somehow pass the page manifest into the middleware at build time.


You could just scope your _middleware to each page rather than have a root level one?
Yes we could, but its a lot of duplication. And I do envisage requirements soon to perform different logic in the root _middleware depending on whether or not its a Next.js page or a path going to be picked up by the fallback rewrite.

What if you do the inverse and match on a known fallback route?
It won't work if you have a catchall: {source: '/:path*', destination: 'https://legacy-system.com:path*'} as every incoming request would match.

@balazsorban44 balazsorban44 added the Middleware Related to Next.js Middleware. label Feb 19, 2022
@jake-nz
Copy link

jake-nz commented Feb 22, 2022

although this behaviour differs between vercel dev and when actually deployed ... thats one for another day
Ah ha! That little discrepancy had me chasing my tail for far too long!

Should issues that are specific to Vercel deployment rather than Next be reported here?

My Vercel issue: vercel/vercel#7452

@jake-nz
Copy link

jake-nz commented Feb 22, 2022

I've realised that Next itself needs to know which page will be matched in middleware.

Given folder structure like this:

  • pages
    • index.tsx
    • page.tsx
    • _middleware.ts
    • [slug]
      • index.tsx
      • _middleware.ts

When a user requests /page request.page.name = '/page'
When a user requests /page2 request.page.name = '/[slug]'
Next needs to know this so that it can decide whether to run [slug]/_middleware.ts or not.

My suggestion would be to keep request.page because it needs to be computed and it's useful to pass on to middleware. Having some indication that there is a static file that matches would be a more useful solution.

@leerob
Copy link
Member

leerob commented Mar 1, 2022

Related: #29750 (comment)

@kodiakhq kodiakhq bot closed this as completed in #37349 Jun 1, 2022
kodiakhq bot pushed a commit that referenced this issue Jun 1, 2022
It uses `URLSearchParams` to have the same behavior.

closes #34521
@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2022

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Middleware Related to Next.js Middleware.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants