-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
docs: Fix suggested middleware matcher regex #505
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
||
// Approach 4: Handle the matching within the middleware, skip processing of known extensions, | ||
// but allow the user to disable this (e.g. `createMiddleware({matcher: false})`) | ||
// See https://github.com/shuding/nextra/blob/feed42a2dbdcd0d15d6c64bd3beab400044a6977/packages/nextra/src/locales.ts#L41 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skonky I dug a bit deeper into this and tried to come up with different approaches.
They have some pros and cons, but a change from the current state is certainly necessary to avoid bad surprises.
I think approach no. 4 makes the most sense, provides a good default and no longer requires setting up a matcher. Along with this, there would be some docs how to opt out of this, e.g. for a use case like giphy.com/g/hello.gif
.
What's your opinion?
Btw. approach no. 1 shown above illustrates a workaround for your current situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some thinking I came to somewhat the same conclusion that if you want to exclude URLs that contain file extensions you need to define explicitly what it needs to look for like the regex you linked, the problem with this tho is that you can never have all available file extensions.
Like how does this piece of code handle webp images? It doesn't. So you need to keep updating this list.
https://github.com/shuding/nextra/blob/feed42a2dbdcd0d15d6c64bd3beab400044a6977/packages/nextra/src/locales.ts#L41
I was thinking maybe it is better to use the headers from the request instead of the URL, maybe it's possible to check for content-type or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can never have all available file extensions
That's correct, e.g. based on the tests I currently have in this file, it's easy to run into a false negative.
We could aggregate some lists from https://github.com/sindresorhus/is-image, https://github.com/sindresorhus/binary-extensions, https://github.com/sindresorhus/text-extensions etc., but looking through the lists, it would likely result in a situation where it's too unpredictable if the user will run into a false positive.
After all, if you have a path like /users/jane.z
, you don't want to worry if z
is a valid file extension. Even .jpg
seems too risky there.
I was thinking maybe it is better to use the headers from the request instead of the URL, maybe it's possible to check for content-type or something.
I also had that in mind some time ago, but I'm not sure it can be done in a reliable way.
E.g. for an embedded image in a web page, in Chrome I see Accept: image/avif,image/webp,image/apng,image/svg+xml,image/*,*/*;q=0.8
as the request header. However when you request the image directly in the browser, you'll get Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
. Would be really odd if you right click + open an image and then it isn't displayed anymore.
On a second thought, maybe approach number 1 is more suitable and we should educate users about this early, before they run into an issue. The biggest tradeoff there IMO is that users could forget to update the matcher when a route is added that accepts a param that can potentially include a dot.
It seems like this is a situation where there's always a tradeoff involved and we have to pick the one with the least chance that users run into an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is a situation where there's always a tradeoff involved and we have to pick the one with the least chance that users run into an issue.
Yeah, I guess you're right. There is no solution that doesn't have a tradeoff, and approach 1 seems to be a good one. I had my hopes up for using the Accept header, but not being able to open it in a new tab is a deal breaker.
I am currently using approach 2 as it works for my usecase at this moment.
We are not alone
vercel/next.js#36308 (reply in thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me. I've added a section in the docs in regard to the middleware matcher and linked to it from the getting started docs.
Would you like to have a look and let me know if you think it's helpful? Proposed docs
Many thanks for investigating and reporting this issue again! Hopefully this can save future users some headaches.
export const config = { | ||
// Skip all paths that should not be internationalized. This example skips | ||
// certain folders and all pathnames with a dot (e.g. favicon.ico) | ||
matcher: ['/((?!api|_next|_vercel|.*\\..*).*)'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO:
- Should probably mention other analytics providers too. E.g. umami uses a local rewrite.
…tcher-regex # Conflicts: # packages/next-intl/package.json # pnpm-lock.yaml
… `localePrefix: 'always'` and update middleware matcher suggestion in docs (#1720) Previously, we suggested a middleware matcher that looked like this: ```tsx // middleware.ts export const config = { // Match only internationalized pathnames matcher: ['/', '/(de|en)/:path*'] }; ``` Even though the hardcoded locales need to be updated when new locales are added, this was suggested in light of providing an error-free getting started experience. However, based on the apps I've seen over time, it seems like this choice was unpopular and users typically go for a matcher that looks like this: ```tsx export const config = { // Match all pathnames except for // - … if they start with `/api`, `/_next` or `/_vercel` // - … the ones containing a dot (e.g. `favicon.ico`) matcher: '/((?!api|_next|_vercel|.*\\..*).*)' }; ``` While this avoids hardcoding locales, it requires extra care to [match pathnames that contain a dot](https://next-intl.dev/docs/routing/middleware#matcher-config) (e.g. `/users/jane.doe`). To align better with user expectations, we now suggest the negative lookahead in the getting started docs and point out the case with pathnames containing dots. As an extra benefit, it makes it significantly easier to switch between routing strategies and add custom prefixes. With the new matcher in place, the middleware now also returns an `x-default` [alternate link](https://next-intl.dev/docs/routing#alternate-links-details) for non-root pathnames (previously only one for `/` was returned when using `localePrefix: 'always'`). Due to this, please update your middleware matcher as shown in the [getting started docs](https://next-intl.dev/docs/getting-started/app-router/with-i18n-routing#middleware) if you're using alternate links. **Related discussions:** - #1136 - #505 - #504
Fixes #504