Skip to content

Commit

Permalink
Add basePath in link component and add/remove it consistently (#9988)
Browse files Browse the repository at this point in the history
* Add basePath in link component and add/remove it consistently

* Update to not use regex for delBasePath

* Expose addBasePath as router method

* Revert "Expose addBasePath as router method"

This reverts commit 40fed59.

* Expose basePath as router field

* Apply suggestion

* Expose basePath as router field

* remove un-used vars

* Update externals

* Apply lint fix

* Update size-limit test

* Update prefetch
  • Loading branch information
ijjk authored Apr 14, 2020
1 parent 0b51b23 commit d3e308a
Show file tree
Hide file tree
Showing 11 changed files with 92 additions and 34 deletions.
7 changes: 6 additions & 1 deletion packages/next/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,12 @@ export default async function getBaseWebpackConfig(

let isNextExternal: boolean = false
if (isLocal) {
isNextExternal = /next[/\\]dist[/\\]next-server[/\\]/.test(res)
// we need to process next-server/lib/router/router so that
// the DefinePlugin can inject process.env values
isNextExternal = /next[/\\]dist[/\\]next-server[/\\](?!lib[/\\]router[/\\]router)/.test(
res
)

if (!isNextExternal) {
return callback()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ const nextServerlessLoader: loader.Loader = function() {
runtimeConfig: runtimeConfig.publicRuntimeConfig || {},
previewProps: ${encodedPreviewProps},
env: process.env,
basePath: "${basePath}",
..._renderOpts
}
let _nextData = false
Expand Down
5 changes: 3 additions & 2 deletions packages/next/client/link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getLocationOrigin,
} from '../next-server/lib/utils'
import Router from './router'
import { addBasePath } from '../next-server/lib/router/router'

function isLocal(href: string) {
const url = parse(href, false, true)
Expand Down Expand Up @@ -161,8 +162,8 @@ class Link extends Component<LinkProps> {
// as per https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
formatUrls = memoizedFormatUrl((href, asHref) => {
return {
href: formatUrl(href),
as: asHref ? formatUrl(asHref) : asHref,
href: addBasePath(formatUrl(href)),
as: asHref ? addBasePath(formatUrl(asHref)) : asHref,
}
})

Expand Down
1 change: 1 addition & 0 deletions packages/next/client/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const urlPropertyFields = [
'asPath',
'components',
'isFallback',
'basePath',
]
const routerEvents = [
'routeChangeStart',
Expand Down
1 change: 1 addition & 0 deletions packages/next/export/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ export default async function(
dev: false,
staticMarkup: false,
hotReloader: null,
basePath: nextConfig.experimental.basePath,
canonicalBase: nextConfig.amp?.canonicalBase || '',
isModern: nextConfig.experimental.modern,
ampValidatorPath: nextConfig.experimental.amp?.validator || undefined,
Expand Down
36 changes: 24 additions & 12 deletions packages/next/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,16 @@ import { isDynamicRoute } from './utils/is-dynamic'
import { getRouteMatcher } from './utils/route-matcher'
import { getRouteRegex } from './utils/route-regex'

function addBasePath(path: string): string {
// variable is always a string
const p = process.env.__NEXT_ROUTER_BASEPATH as string
return path.indexOf(p) !== 0 ? p + path : path
const basePath = process.env.__NEXT_ROUTER_BASEPATH as string

export function addBasePath(path: string): string {
return path.indexOf(basePath) !== 0 ? basePath + path : path
}

function delBasePath(path: string): string {
return path.indexOf(basePath) === 0
? path.substr(basePath.length) || '/'
: path
}

function toRoute(path: string): string {
Expand All @@ -38,6 +44,7 @@ export type BaseRouter = {
pathname: string
query: ParsedUrlQuery
asPath: string
basePath: string
}

export type NextRouter = BaseRouter &
Expand Down Expand Up @@ -133,6 +140,8 @@ export default class Router implements BaseRouter {
pathname: string
query: ParsedUrlQuery
asPath: string
basePath: string

/**
* Map of all components loaded in `Router`
*/
Expand Down Expand Up @@ -206,6 +215,7 @@ export default class Router implements BaseRouter {
this.asPath =
// @ts-ignore this is temporarily global (attached to window)
isDynamicRoute(pathname) && __NEXT_DATA__.autoExport ? pathname : as
this.basePath = basePath
this.sub = subscription
this.clc = null
this._wrapApp = wrapApp
Expand Down Expand Up @@ -361,9 +371,12 @@ export default class Router implements BaseRouter {

// If url and as provided as an object representation,
// we'll format them into the string version here.
const url = typeof _url === 'object' ? formatWithValidation(_url) : _url
let url = typeof _url === 'object' ? formatWithValidation(_url) : _url
let as = typeof _as === 'object' ? formatWithValidation(_as) : _as

url = addBasePath(url)
as = addBasePath(as)

// Add the ending slash to the paths. So, we can serve the
// "<page>/index.html" directly for the SSR page.
if (process.env.__NEXT_EXPORT_TRAILING_SLASH) {
Expand All @@ -386,7 +399,7 @@ export default class Router implements BaseRouter {
if (!options._h && this.onlyAHashChange(as)) {
this.asPath = as
Router.events.emit('hashChangeStart', as)
this.changeState(method, url, addBasePath(as), options)
this.changeState(method, url, as, options)
this.scrollToHash(as)
Router.events.emit('hashChangeComplete', as)
return resolve(true)
Expand Down Expand Up @@ -458,7 +471,7 @@ export default class Router implements BaseRouter {
}

Router.events.emit('beforeHistoryChange', as)
this.changeState(method, url, addBasePath(as), options)
this.changeState(method, url, as, options)

if (process.env.NODE_ENV !== 'production') {
const appComp: any = this.components['/_app'].Component
Expand Down Expand Up @@ -737,12 +750,10 @@ export default class Router implements BaseRouter {
if (process.env.NODE_ENV !== 'production') {
return
}

const route = delBasePath(toRoute(pathname))
Promise.all([
this.pageLoader.prefetchData(url, asPath),
this.pageLoader[options.priority ? 'loadPage' : 'prefetch'](
toRoute(pathname)
),
this.pageLoader.prefetchData(url, delBasePath(asPath)),
this.pageLoader[options.priority ? 'loadPage' : 'prefetch'](route),
]).then(() => resolve(), reject)
})
}
Expand All @@ -752,6 +763,7 @@ export default class Router implements BaseRouter {
const cancel = (this.clc = () => {
cancelled = true
})
route = delBasePath(route)

const componentResult = await this.pageLoader.loadPage(route)

Expand Down
17 changes: 10 additions & 7 deletions packages/next/next-server/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export default class Server {
previewProps: __ApiPreviewProps
customServer?: boolean
ampOptimizerConfig?: { [key: string]: any }
basePath: string
}
private compression?: Middleware
private onErrorMiddleware?: ({ err }: { err: Error }) => Promise<void>
Expand Down Expand Up @@ -179,6 +180,7 @@ export default class Server {
previewProps: this.getPreviewProps(),
customServer: customServer === true ? true : undefined,
ampOptimizerConfig: this.nextConfig.experimental.amp?.optimizer,
basePath: this.nextConfig.experimental.basePath,
}

// Only the `publicRuntimeConfig` key is exposed to the client side
Expand Down Expand Up @@ -265,14 +267,15 @@ export default class Server {
parsedUrl.query = parseQs(parsedUrl.query)
}

if (parsedUrl.pathname!.startsWith(this.nextConfig.experimental.basePath)) {
const { basePath } = this.nextConfig.experimental

// if basePath is set require it be present
if (basePath && !req.url!.startsWith(basePath)) {
return this.render404(req, res, parsedUrl)
} else {
// If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/`
parsedUrl.pathname =
parsedUrl.pathname!.replace(
this.nextConfig.experimental.basePath,
''
) || '/'
req.url = req.url!.replace(this.nextConfig.experimental.basePath, '')
parsedUrl.pathname = parsedUrl.pathname!.replace(basePath, '') || '/'
req.url = req.url!.replace(basePath, '')
}

res.statusCode = 200
Expand Down
22 changes: 16 additions & 6 deletions packages/next/next-server/server/render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class ServerRouter implements NextRouter {
pathname: string
query: ParsedUrlQuery
asPath: string
basePath: string
events: any
isFallback: boolean
// TODO: Remove in the next major version, as this would mean the user is adding event listeners in server-side `render` method
Expand All @@ -63,14 +64,15 @@ class ServerRouter implements NextRouter {
pathname: string,
query: ParsedUrlQuery,
as: string,
{ isFallback }: { isFallback: boolean }
{ isFallback }: { isFallback: boolean },
basePath: string
) {
this.route = pathname.replace(/\/$/, '') || '/'
this.pathname = pathname
this.query = query
this.asPath = as

this.isFallback = isFallback
this.basePath = basePath
}
push(): any {
noRouter()
Expand Down Expand Up @@ -156,6 +158,7 @@ export type RenderOptsPartial = {
isDataReq?: boolean
params?: ParsedUrlQuery
previewProps: __ApiPreviewProps
basePath: string
}

export type RenderOpts = LoadComponentsReturnType & RenderOptsPartial
Expand Down Expand Up @@ -309,6 +312,7 @@ export async function renderToHTML(
isDataReq,
params,
previewProps,
basePath,
} = renderOpts

const callMiddleware = async (method: string, args: any[], props = false) => {
Expand Down Expand Up @@ -452,10 +456,16 @@ export async function renderToHTML(
await Loadable.preloadAll() // Make sure all dynamic imports are loaded

// url will always be set
const asPath = req.url as string
const router = new ServerRouter(pathname, query, asPath, {
isFallback: isFallback,
})
const asPath: string = req.url as string
const router = new ServerRouter(
pathname,
query,
asPath,
{
isFallback: isFallback,
},
basePath
)
const ctx = {
err,
req: isAutoExport ? undefined : req,
Expand Down
14 changes: 9 additions & 5 deletions test/integration/basepath/pages/hello.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import Link from 'next/link'
import { useRouter } from 'next/router'

export default () => (
<Link href="/other-page">
<a id="other-page-link">
<h1>Hello World</h1>
</a>
</Link>
<>
<Link href="/other-page">
<a id="other-page-link">
<h1>Hello World</h1>
</a>
</Link>
<div id="base-path">{useRouter().basePath}</div>
</>
)
20 changes: 20 additions & 0 deletions test/integration/basepath/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/* global jasmine */
import webdriver from 'next-webdriver'
import { join } from 'path'
import url from 'url'
import {
nextServer,
launchApp,
Expand Down Expand Up @@ -49,6 +50,19 @@ const runTests = (context, dev = false) => {
}
})

it('should have correct href for a link', async () => {
const browser = await webdriver(context.appPort, '/docs/hello')
const href = await browser.elementByCss('a').getAttribute('href')
const { pathname } = url.parse(href)
expect(pathname).toBe('/docs/other-page')
})

it('should show 404 for page not under the /docs prefix', async () => {
const text = await renderViaHTTP(context.appPort, '/hello')
expect(text).not.toContain('Hello World')
expect(text).toContain('This page could not be found')
})

it('should show the other-page page under the /docs prefix', async () => {
const browser = await webdriver(context.appPort, '/docs/other-page')
try {
Expand All @@ -59,6 +73,12 @@ const runTests = (context, dev = false) => {
}
})

it('should have basePath field on Router', async () => {
const html = await renderViaHTTP(context.appPort, '/docs/hello')
const $ = cheerio.load(html)
expect($('#base-path').text()).toBe('/docs')
})

it('should navigate to the page without refresh', async () => {
const browser = await webdriver(context.appPort, '/docs/hello')
try {
Expand Down
2 changes: 1 addition & 1 deletion test/integration/size-limit/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe('Production response size', () => {
)

// These numbers are without gzip compression!
const delta = responseSizesBytes - 165 * 1024
const delta = responseSizesBytes - 166 * 1024
expect(delta).toBeLessThanOrEqual(1024) // don't increase size more than 1kb
expect(delta).toBeGreaterThanOrEqual(-1024) // don't decrease size more than 1kb without updating target
})
Expand Down

0 comments on commit d3e308a

Please sign in to comment.