-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Feature/243 base path #589
Conversation
@Robjam is attempting to deploy a commit to the next-intl Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thank you so much for looking into this @Robjam! I was on vacation the last few days and need to catch up on a few things. I'll look into this PR next week and get back to you! |
Will be watching this PR, our team is running into issues with Let me know if there's anything we can do to help support this fix, seems like a very important feature to include in |
Hi @amannn, would you be able to provide an ETA for this PR to be reviewed and subsequently fix the issue with utilizing |
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.
Thank you so much for your work here! So cool! 👏
I found that NextRequest.pathname
already excludes the basePath
. I think if we're careful to construct all URL
objects with that pathname, then we could remove many checks.
I think adding the basePath
should mostly be relevant for the alternate links, but less for rewrite/redirect logic if we're careful.
Let me know what you think!
describe.each([ | ||
{ basePath: undefined }, | ||
{ basePath: '/sample' } | ||
])('when basePath is $basePath', ({ basePath }: { basePath: string }) => { |
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.
Cool idea with describe.each
!
TypeScript reports an error for me here:
test/middleware/getAlternateLinksHeaderValue.test.tsx(11,34): error TS2345: Argument of type '({ basePath }: { basePath: string; }) => void' is not assignable to parameter o…
Types of parameters '__0' and 'args' are incompatible.
Type '{ basePath: undefined; } | { basePath: string; }' is not assignable to type '{ basePath: string; }'.
Type '{ basePath: undefined; }' is not assignable to type '{ basePath: string; }'.
Types of property 'basePath' are incompatible.
Type 'undefined' is not assignable to type 'string'.
(see also the failing build)
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.
Does basePath
need to be undefined
here? It appears in NextIntlMiddlewareConfig.tsx
, this property is defined as non-optional, so would we just want a default /
here to test instead, or should the MiddlewareConfigWithDefaults
type be updated to accept undefined
?
The tests rely on a reference to basePath
to do string concatenation; if the intention is for basePath
to be undefined
here, you may want to have another param here called expectedPath
or something that will be defined (empty string in the case basePath
is undefined
) to use instead.
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.
good point!
This already has a default set and so /
is a better case.
// Use `basepath` to deploy to a sub-path like `/blog`. | ||
// Typically `basePath` is set to the same value in | ||
// `next.config.js` options | ||
basePath: '/blog', |
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.
Thanks for proposing docs too!
We can move this to a separate section below (see "Further configuration").
@@ -6,6 +6,11 @@ import {getHost, isLocaleSupportedOnDomain} from './utils'; | |||
|
|||
function getUnprefixedUrl(config: MiddlewareConfig, request: NextRequest) { | |||
const url = new URL(request.url); | |||
|
|||
if (config.basePath && !url.pathname.startsWith(config.basePath)) { |
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.
Do we need this check for config.basePath
if we have ''
as a default for basePath
? Maybe a smart default helps us to avoid unnecessary conditions.
About the second part of the condition: request.pathname
doesn't seem to include the basePath
(just tested in an app), can we remove this part of the condition too?
it('works for prefixed routing (as-needed)', () => { | ||
const config: MiddlewareConfigWithDefaults = { | ||
defaultLocale: 'en', | ||
locales: ['en', 'es'], | ||
alternateLinks: true, | ||
localePrefix: 'as-needed', | ||
localeDetection: true | ||
localeDetection: true, | ||
basePath |
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.
I currently get some failing tests in this file.
@@ -1,6 +1,9 @@ | |||
type LocalePrefix = 'as-needed' | 'always' | 'never'; | |||
|
|||
type RoutingBaseConfig = { | |||
/** The base path of your application (optional). */ | |||
basePath?: string; |
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.
In case this lands after #149, we could use the new plugin to automatically set this value based on basePath
in next.config.js
. I think something like webpack.DefinePlugin
could work. I can't really think of a use case where the user would want a different value here than what is configured in Next.js.
We can think about this as the last step though when everything else works.
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.
I noticed the basePath
is available on the request
object:
import type { NextRequest } from 'next/server'
export function middleware(request: NextRequest) {
// Either '' (default) or a value like '/test'
console.log({basePath: request.nextUrl.basePath});
}
That way we don't have to make this configurable in the first place.
As mentioned in the other comment, I think we only need it for the alternate links though.
@@ -73,6 +80,10 @@ export default function getAlternateLinksHeaderValue( | |||
localizePathname(url); | |||
} | |||
|
|||
if(config.basePath && !url.pathname.startsWith(config.basePath)) { |
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.
There are btw. some small formatting issues, you can fix them via ESLint:
pnpm eslint src test --fix
… or autofix them on save in case you use VSCode:
"[typescript]": {
"editor.defaultFormatter": "dbaeumer.vscode-eslint",
"editor.formatOnSave": true
},
I should really consider setting up https://github.com/wearerequired/lint-action to make it easier for contributors.
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.
Thanks for the how-to on setting up VsCode.
Opening VSCode from the repository root path gives an error with Parsing error: Cannot read file '/users/robjam/code/community/next-intl/tsconfig.json'. eslint
so I use the cli to format things. Maybe we can fix this with a root tsconfig.json and extend it from each workspace?
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.
My understanding of monorepos is that individual folders should be opened when working inside of them. E.g. when working on the next-intl
package, you'd open this as your workspace root. If that isn't the case, ESLint will not work correctly.
I've added an info about this to the contributors guide for now.
@@ -37,6 +40,7 @@ export type MiddlewareConfigWithDefaults = MiddlewareConfig & { | |||
alternateLinks: boolean; | |||
localePrefix: LocalePrefix; | |||
localeDetection: boolean; | |||
basePath: string; |
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.
I did a first test with example-next-13
in the repo. With localePrefix: 'always'
this already seems to work quite well! Pretty cool that Link
automatically considers the basePath
if it's set in next.config.js
(source).
With localePrefix: 'as-necessary'
I run into infinite redirects though when trying to access the app root. Can you reproduce that?
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.
I didn't test locally with localePrefix: 'as-necessary'
as we require the locale to be in the path. I'll scaffold a new next app and reproduce it.
@@ -68,7 +69,10 @@ export default function createMiddleware(config: MiddlewareConfig) { | |||
} | |||
|
|||
function rewrite(url: string) { | |||
return NextResponse.rewrite(new URL(url, request.url), getResponseInit()); |
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.
I think the problem here is the construction with request.url
. If we apply request.pathname
then maybe we can avoid introducing more conditions.
@@ -93,7 +97,7 @@ export default function createMiddleware(config: MiddlewareConfig) { | |||
bestMatchingDomain.defaultLocale === locale && | |||
configWithDefaults.localePrefix === 'as-needed' | |||
) { | |||
urlObj.pathname = urlObj.pathname.replace(`/${locale}`, ''); | |||
urlObj.pathname = urlObj.pathname.replace(`${configWithDefaults.basePath}/${locale}`, ''); |
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.
Also here above we're using request.url
, which includes the basePath
.
return NextResponse.redirect(urlObj.toString()); | ||
} | ||
|
||
let response; | ||
if (isRoot) { | ||
let pathWithSearch = `/${locale}`; | ||
let pathWithSearch = `${configWithDefaults.basePath}/${locale}`; |
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.
Maybe we can avoid this change too, pathWithSearch
is passed to rewrite
and redirect
below, which—as mentioned above—could likely be adjusted to automatically consider the basePath
.
Hey @Robjam, did you have another minute to look into this? next-intl 3.0 has been released in the meantime with support for Server Components, as well as some improvements for the middleware. It seems like that has caused some conflicts with this PR. Can you figure out how to incorporate the changes or can I help with something? |
When will this pr be merged? This feature is very important to me. |
Due to lack cookie customization mentioned with #486, I think the base path should also be natively supported in the response cookie in order to make it only available to the desired base path and not to the entire domain since this can cause conflicts between websites of the same domain. For now, I'm sticking with this workaround but a native support would be welcome to be included in this pull request. const response = handleI18nRouting( request );
response.cookies.set(
"NEXT_LOCALE",
response.headers.get( "x-middleware-request-x-next-intl-locale" ) ?? defaultLocale,
{
path: process.env.__NEXT_ROUTER_BASEPATH,
maxAge: 60 * 60 * 24 * 365,
sameSite: "strict"
}
);
return response; |
by @Robjam and @amannn) There's no need for a new config option, `next-intl` will automatically pick up the `basePath` you've configured in `next.config.js`. Note however that you should make sure that you match `/` explicitly in your middleware `matcher` (see [the updated docs](https://next-intl-docs.vercel.app/docs/routing/middleware#base-path)). The only integration points are: - Apply the `basePath` for redirects, as Next.js doesn't handle this internally - Apply the `basePath` for rewrites, as Next.js doesn't handle this internally - Apply the `basePath` for alternate links This PR is based on the initial work in #589 by @Robjam Closes #589 Closes #243 --------- Co-authored-by: Robjam <rjames@smartshopping.co.jp> Co-authored-by: James Roberts <github@robjam.dev>
This PR add support for
basePath
in the next-intl configuration and solves #243.Documentation regarding middleware is also included (59f25d1).