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

fix: use a client-side navigation when redirecting to a rewriten URL #25990

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
12fb755
fix: client-side navigation when redirecting to a rewriten URL
Jun 10, 2021
066846c
refactor: move gssp redirect with rewrites tests next to gssp-redirec…
Jun 13, 2021
84a2234
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
antoinechalifour Jun 13, 2021
fa14159
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
antoinechalifour Jun 14, 2021
658b9d6
revert: component name in documentation updated while refactoring
Jun 14, 2021
6120fd3
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
antoinechalifour Jun 15, 2021
94c2fa8
reove check known route as there is a fallback anyway
Jun 15, 2021
b1e8c77
update build output size
Jun 15, 2021
31c46a0
Update test/integration/gssp-redirect-with-rewrites/test/index.test.js
antoinechalifour Jun 15, 2021
682935b
Update test/integration/gssp-redirect-with-rewrites/test/index.test.js
antoinechalifour Jun 15, 2021
9a85b56
Update test/integration/gssp-redirect-with-rewrites/test/index.test.js
antoinechalifour Jun 15, 2021
9544294
fix typo
Jun 15, 2021
8823e06
add test for redirects to unknown route
Jun 15, 2021
9a862d3
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
antoinechalifour Jun 16, 2021
a3f6a61
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
ijjk Jun 16, 2021
fd722c4
update size test
ijjk Jun 16, 2021
efa698b
Merge remote-tracking branch 'upstream/canary' into redirect-getserve…
ijjk Jun 16, 2021
ab5954d
Update test
ijjk Jun 16, 2021
f932956
lint-fix
ijjk Jun 16, 2021
1f46641
Merge branch 'canary' into redirect-getserverside-props-with-rewrites
ijjk Jun 16, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions packages/next/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1112,14 +1112,12 @@ export default class Router implements BaseRouter {
pages
)

if (pages.includes(parsedHref.pathname)) {
const { url: newUrl, as: newAs } = prepareUrlAs(
this,
destination,
destination
)
return this.change(method, newUrl, newAs, options)
}
const { url: newUrl, as: newAs } = prepareUrlAs(
this,
destination,
destination
)
return this.change(method, newUrl, newAs, options)
}

window.location.href = destination
Expand Down
4 changes: 2 additions & 2 deletions test/integration/build-output/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ describe('Build Output', () => {
expect(parseFloat(err404Size)).toBeCloseTo(gz ? 3.17 : 8.51, 1)
expect(err404Size.endsWith('kB')).toBe(true)

expect(parseFloat(err404FirstLoad)).toBeCloseTo(gz ? 66.9 : 205, 1)
expect(parseFloat(err404FirstLoad)).toBeCloseTo(gz ? 66.9 : 204, 1)
expect(err404FirstLoad.endsWith('kB')).toBe(true)

expect(parseFloat(sharedByAll)).toBeCloseTo(gz ? 63.7 : 196, 1)
Expand All @@ -149,7 +149,7 @@ describe('Build Output', () => {
true
)

expect(parseFloat(mainSize)).toBeCloseTo(gz ? 20.1 : 62.8, 1)
expect(parseFloat(mainSize)).toBeCloseTo(gz ? 20.1 : 62.7, 1)
expect(mainSize.endsWith('kB')).toBe(true)

expect(parseFloat(frameworkSize)).toBeCloseTo(gz ? 42.0 : 130, 1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ const runTests = (isDev) => {

const curUrl = await browser.url()
const { pathname } = url.parse(curUrl)
expect(pathname).toBe('/missing')
expect(pathname).toBe('/docs/missing')
})

it('should apply redirect when fallback GSP page is visited directly (external domain)', async () => {
Expand Down
10 changes: 10 additions & 0 deletions test/integration/gssp-redirect-with-rewrites/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module.exports = {
async rewrites() {
return [
{
source: '/alias-to-main-content',
destination: '/main-content',
},
]
},
}
35 changes: 35 additions & 0 deletions test/integration/gssp-redirect-with-rewrites/pages/main-content.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import Link from 'next/link'

export default function MainContent({ message }) {
return (
<main>
<h1>Hello {message}</h1>

<ul>
<li>
<Link href="/redirector?redirect=/alias-to-main-content&message=refreshed">
<a id="link-with-rewritten-url" className={message}>
Link with rewritten target url
</a>
</Link>
</li>

<li>
<Link href="/redirector?redirect=/main-content&message=refreshWithClientSideNavigation">
<a>Link with client side navigation</a>
</Link>
</li>

<li>
<Link href="/redirector?redirect=/unknown-route">
<a id="link-unknown-url">Link to unknown internal navigation</a>
</Link>
</li>
</ul>
</main>
)
}

export const getServerSideProps = ({ query }) => ({
props: { message: query.message || 'World ' },
})
16 changes: 16 additions & 0 deletions test/integration/gssp-redirect-with-rewrites/pages/redirector.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export default function Redirector() {
return (
<main>
<h1>Hello world</h1>
</main>
)
}

export const getServerSideProps = ({ query }) => {
return {
redirect: {
destination: `${query.redirect}?message=${query.message}`,
permanent: false,
},
}
}
62 changes: 62 additions & 0 deletions test/integration/gssp-redirect-with-rewrites/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/* eslint-env jest */

import { join } from 'path'
import {
renderViaHTTP,
findPort,
launchApp,
killApp,
check,
} from 'next-test-utils'
import webdriver from 'next-webdriver'

// test suites

const context = {}
jest.setTimeout(1000 * 60 * 5)

describe('getServerSideProps redirects', () => {
beforeAll(async () => {
context.appPort = await findPort()
context.server = await launchApp(join(__dirname, '../'), context.appPort, {
env: { __NEXT_TEST_WITH_DEVTOOL: 1 },
})

// pre-build all pages at the start
await Promise.all([
renderViaHTTP(context.appPort, '/alias-to-main-content'),
renderViaHTTP(context.appPort, '/main-content'),
])
})
afterAll(() => killApp(context.server))

it('should use a client-side navigation for a rewritten URL', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a similar test but instead ensure the /unknown-route being returned as a redirect does do a full navigation instead of a client navigation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ! Thanks for your suggestions 😄

const browser = await webdriver(context.appPort, '/alias-to-main-content')

// During a browser navigation global variables are reset,
// So by checking that the __SAME_PAGE variable is still defined
// then the client-side navigation has happened
await browser.eval('window.__SAME_PAGE = true')

await browser.elementByCss('#link-with-rewritten-url').click()

// Wait until the new props are rendered
await browser.waitForElementByCss('.refreshed')

expect(await browser.eval('window.__SAME_PAGE')).toBe(true)
})

it('should fallback to browser navigation for an unknown URL', async () => {
const browser = await webdriver(context.appPort, '/alias-to-main-content')

// then the client-side navigation has happened
await browser.eval('window.__SAME_PAGE = true')
await browser.elementByCss('#link-unknown-url').click()

// Wait until the page has be reloaded
await check(async () => {
const val = await browser.eval('window.__SAME_PAGE')
return val ? 'fail' : 'success'
}, 'success')
})
})