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

Handle defaultLocale on client router filter #47180

Merged
merged 3 commits into from
Mar 16, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/next/src/shared/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,9 @@ export default class Router implements BaseRouter {
// a hard navigation
if (matchesBflStatic || matchesBflDynamic) {
handleHardNavigation({
url: addBasePath(addLocale(as, locale || this.locale)),
url: addBasePath(
addLocale(as, locale || this.locale, this.defaultLocale)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addLocale skips adding the locale if locale is equal to defaultLocale:

if (
locale &&
locale !== defaultLocale &&
(ignorePrefix ||
(!pathHasPrefix(path.toLowerCase(), `/${locale.toLowerCase()}`) &&
!pathHasPrefix(path.toLowerCase(), '/api')))
) {
return addPathPrefix(path, `/${locale}`)
}

),
router: this,
})
return new Promise(() => {})
Expand Down
19 changes: 19 additions & 0 deletions test/e2e/i18n-default-locale-redirect/app/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module.exports = {
i18n: {
locales: ['en', 'nl'],
defaultLocale: 'en',
},
experimental: {
clientRouterFilter: true,
clientRouterFilterRedirects: true,
},
redirects() {
return [
{
source: '/to-new',
destination: '/new',
permanent: false,
},
]
},
}
18 changes: 18 additions & 0 deletions test/e2e/i18n-default-locale-redirect/app/pages/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import Link from 'next/link'

export default function Home() {
return (
<div>
<div>
<Link href="/to-new" id="to-new">
To new (Default Locale)
</Link>
</div>
<div>
<Link href="/to-new" id="to-new-nl" locale="nl">
To new (NL)
</Link>
</div>
</div>
)
}
3 changes: 3 additions & 0 deletions test/e2e/i18n-default-locale-redirect/app/pages/new.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function New() {
return <div id="new">New</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { join } from 'path'
import { createNextDescribe, FileRef } from 'e2e-utils'

createNextDescribe(
'i18-default-locale-redirect',
{
files: {
pages: new FileRef(join(__dirname, './app/pages')),
'next.config.js': new FileRef(join(__dirname, './app/next.config.js')),
},
},
({ next }) => {
it('should not request a path prefixed with default locale', async () => {
const browser = await next.browser('/')
let requestedDefaultLocalePath = false
browser.on('request', (request: any) => {
if (new URL(request.url(), 'http://n').pathname === '/en/to-new') {
requestedDefaultLocalePath = true
}
})

await browser.elementByCss('#to-new').click().waitForElementByCss('#new')
expect(await browser.elementByCss('#new').text()).toBe('New')
expect(requestedDefaultLocalePath).toBe(false)
Copy link
Member Author

@chibicode chibicode Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that this test passes with my changes but fails with the current canary:

 FAIL  test/e2e/i18n-default-locale-redirect/i18n-default-locale-redirect.test.ts (17.271 s)
  i18-default-locale-redirect
    ✕ should not request a path prefixed with default locale (899 ms)
    ✓ should request a path prefixed with non-default locale (150 ms)

  ● i18-default-locale-redirect › should not request a path prefixed with default locale

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

      22 |       await browser.elementByCss('#to-new').click().waitForElementByCss('#new')
      23 |       expect(await browser.elementByCss('#new').text()).toBe('New')
    > 24 |       expect(requestedDefaultLocalePath).toBe(false)
         |                                          ^
      25 |     })
      26 |
      27 |     it('should request a path prefixed with non-default locale', async () => {

      at Object.<anonymous> (e2e/i18n-default-locale-redirect/i18n-default-locale-redirect.test.ts:24:42)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failure Demo (current canary)

Clicking on <Link href="/to-new" id="to-new"> hard-navigates to /en/to-new instead of /to-new (before redirecting to /new):

demo.mp4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Success Demo (after my changes)

It now hard-navigates to /to-new instead, omitting defaultLocale.

success.mp4

})

it('should request a path prefixed with non-default locale', async () => {
const browser = await next.browser('/')
let requestedNonDefaultLocalePath = false
browser.on('request', (request: any) => {
if (new URL(request.url(), 'http://n').pathname === '/nl/to-new') {
requestedNonDefaultLocalePath = true
}
})

await browser
.elementByCss('#to-new-nl')
.click()
.waitForElementByCss('#new')
expect(await browser.elementByCss('#new').text()).toBe('New')
expect(requestedNonDefaultLocalePath).toBe(true)
})
}
)
2 changes: 1 addition & 1 deletion test/lib/browsers/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export class Playwright extends BrowserInterface {
if (!this.eventCallbacks[event]) {
throw new Error(
`Invalid event passed to browser.on, received ${event}. Valid events are ${Object.keys(
event
this.eventCallbacks
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but noticed this bug while debugging. Object.keys must be called on this.eventCallbacks since that's what you're looking up in.

)}`
)
}
Expand Down