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(next-server): Fix priority for edge routes #39462

Merged
merged 14 commits into from Aug 16, 2022
2 changes: 1 addition & 1 deletion packages/next/server/base-server.ts
Expand Up @@ -127,7 +127,7 @@ export interface BaseRequestHandler {
): Promise<void>
}

type RequestContext = {
export type RequestContext = {
req: BaseNextRequest
res: BaseNextResponse
pathname: string
Expand Down
1 change: 1 addition & 0 deletions packages/next/server/dev/next-dev-server.ts
Expand Up @@ -386,6 +386,7 @@ export default class DevServer extends Server {
routedPages.push(pageName)
},
onEdgeServer: () => {
routedPages.push(pageName)
edgeRoutesSet.add(pageName)
},
})
Expand Down
9 changes: 9 additions & 0 deletions packages/next/server/dev/on-demand-entry-handler.ts
@@ -1,4 +1,5 @@
import type ws from 'ws'
import * as Log from '../../build/output/log'
import type { webpack } from 'next/dist/compiled/webpack/webpack'
import type { NextConfigComplete } from '../config-shared'
import { EventEmitter } from 'events'
Expand Down Expand Up @@ -514,6 +515,13 @@ export function onDemandEntryHandler({

return {
async ensurePage(page: string, clientOnly: boolean): Promise<void> {
const stalledTime = 60
const stalledEnsureTimeout = setTimeout(() => {
Log.warn(
`Ensuring ${page} has taken longer than ${stalledTime}s, if this continues to stall this may be a bug`
)
}, stalledTime * 1000)

const pagePathData = await findPagePathData(
rootDir,
pagesDir,
Expand Down Expand Up @@ -630,6 +638,7 @@ export function onDemandEntryHandler({
invalidator.invalidate([...added.keys()])
await Promise.all(invalidatePromises)
}
clearTimeout(stalledEnsureTimeout)
},

onHMR(client: ws) {
Expand Down
124 changes: 51 additions & 73 deletions packages/next/server/next-server.ts
Expand Up @@ -73,6 +73,7 @@ import BaseServer, {
prepareServerlessUrl,
RoutingItem,
NoFallbackError,
RequestContext,
} from './base-server'
import { getPagePath, requireFontManifest } from './require'
import { denormalizePagePath } from '../shared/lib/page-path/denormalize-page-path'
Expand All @@ -95,7 +96,6 @@ import { checkIsManualRevalidate } from './api-utils'
import { shouldUseReactRoot, isTargetLikeServerless } from './utils'
import ResponseCache from './response-cache'
import { IncrementalCache } from './lib/incremental-cache'
import { getSortedRoutes } from '../shared/lib/router/utils/sorted-routes'

if (shouldUseReactRoot) {
;(process.env as any).__NEXT_REACT_ROOT = 'true'
Expand Down Expand Up @@ -673,16 +673,22 @@ export default class NextNodeServer extends BaseServer {
page: string,
builtPagePath: string
): Promise<boolean> {
const handledAsEdgeFunction = await this.runEdgeFunction({
req,
res,
query,
params,
page,
})
const edgeFunctions = this.getEdgeFunctions()

for (const item of edgeFunctions) {
if (item.page === page) {
const handledAsEdgeFunction = await this.runEdgeFunction({
req,
res,
query,
params,
page,
})

if (handledAsEdgeFunction) {
return true
if (handledAsEdgeFunction) {
return true
}
}
}

const pageModule = await require(builtPagePath)
Expand Down Expand Up @@ -801,6 +807,28 @@ export default class NextNodeServer extends BaseServer {
)
}

protected async renderPageComponent(
ctx: RequestContext,
bubbleNoFallback: boolean
) {
const edgeFunctions = this.getEdgeFunctions()

for (const item of edgeFunctions) {
if (item.page === ctx.pathname) {
await this.runEdgeFunction({
req: ctx.req,
res: ctx.res,
query: ctx.query,
params: ctx.renderOpts.params,
page: ctx.pathname,
})
return null
}
}

return super.renderPageComponent(ctx, bubbleNoFallback)
}

protected async findPageComponents(
pathname: string,
query: NextParsedUrlQuery = {},
Expand Down Expand Up @@ -1452,20 +1480,16 @@ export default class NextNodeServer extends BaseServer {
return manifest
}

/**
* Return a list of middleware routing items. This method exists to be later
* overridden by the development server in order to use a different source
* to get the list.
*/
balazsorban44 marked this conversation as resolved.
Show resolved Hide resolved
/** Returns the middleware routing item if there is one. */
protected getMiddleware(): RoutingItem | undefined {
const manifest = this.getMiddlewareManifest()
const rootMiddleware = manifest?.middleware?.['/']
if (!rootMiddleware) {
const middleware = manifest?.middleware?.['/']
if (!middleware) {
return
}

return {
match: getMiddlewareMatcher(rootMiddleware),
match: getMiddlewareMatcher(middleware),
page: '/',
}
}
Expand All @@ -1476,13 +1500,10 @@ export default class NextNodeServer extends BaseServer {
return []
}

// Make sure to sort function routes too.
return getSortedRoutes(Object.keys(manifest.functions)).map((page) => {
return {
match: getMiddlewareMatcher(manifest.functions[page]),
page,
}
})
return Object.keys(manifest.functions).map((page) => ({
match: getMiddlewareMatcher(manifest.functions[page]),
page,
}))
}

/**
Expand Down Expand Up @@ -1857,45 +1878,6 @@ export default class NextNodeServer extends BaseServer {

routes.push(middlewareCatchAllRoute)
}
if (this.getEdgeFunctions().length) {
const edgeCatchAllRoute: Route = {
match: getPathMatch('/:path*'),
type: 'route',
name: 'edge functions catchall',
fn: async (req, res, _params, parsed) => {
const edgeFunctions = this.getEdgeFunctions()
if (!edgeFunctions.length) return { finished: false }

const { query, pathname } = parsed
const normalizedPathname = removeTrailingSlash(pathname || '')
let page = normalizedPathname
let params: Params | undefined = undefined

for (const edgeFunction of edgeFunctions) {
const matched = edgeFunction.match(page)
if (matched) {
params = matched
page = edgeFunction.page
break
}
}

const edgeSSRResult = await this.runEdgeFunction({
req,
res,
query,
params,
page,
})

return {
finished: !!edgeSSRResult,
}
},
}

routes.push(edgeCatchAllRoute)
}
}

return routes
Expand Down Expand Up @@ -1946,15 +1928,11 @@ export default class NextNodeServer extends BaseServer {
}): Promise<FetchEventResult | null> {
let middlewareInfo: ReturnType<typeof this.getEdgeFunctionInfo> | undefined

try {
await this.ensureEdgeFunction(params.page)
middlewareInfo = this.getEdgeFunctionInfo({
page: params.page,
middleware: false,
})
} catch {
return null
}
await this.ensureEdgeFunction(params.page)
middlewareInfo = this.getEdgeFunctionInfo({
page: params.page,
middleware: false,
})

if (!middlewareInfo) {
return null
Expand Down
11 changes: 2 additions & 9 deletions packages/next/server/router.ts
Expand Up @@ -220,8 +220,7 @@ export default class Router {
- User rewrites (checking filesystem and pages each match)
*/

const [middlewareCatchAllRoute, edgeSSRCatchAllRoute] =
this.catchAllMiddleware
const [middlewareCatchAllRoute] = this.catchAllMiddleware
const allRoutes = [
...(middlewareCatchAllRoute
? this.fsRoutes
Expand All @@ -244,7 +243,6 @@ export default class Router {
// disabled
...(this.useFileSystemPublicRoutes
? [
...(edgeSSRCatchAllRoute ? [edgeSSRCatchAllRoute] : []),
balazsorban44 marked this conversation as resolved.
Show resolved Hide resolved
{
type: 'route',
name: 'page checker',
Expand Down Expand Up @@ -299,12 +297,7 @@ export default class Router {

// We only check the catch-all route if public page routes hasn't been
// disabled
...(this.useFileSystemPublicRoutes
? [
...(edgeSSRCatchAllRoute ? [edgeSSRCatchAllRoute] : []),
this.catchAllRoute,
]
: []),
...(this.useFileSystemPublicRoutes ? [this.catchAllRoute] : []),
]

for (const testRoute of allRoutes) {
Expand Down
33 changes: 33 additions & 0 deletions test/e2e/edge-vs.-non-edge-api-route-priority/index.test.ts
@@ -0,0 +1,33 @@
import { createNext } from 'e2e-utils'
import { NextInstance } from 'test/lib/next-modes/base'
import { fetchViaHTTP } from 'next-test-utils'

describe('Edge vs. non-Edge API route priority', () => {
let next: NextInstance

beforeAll(async () => {
next = await createNext({
files: {
'pages/api/user/login.js': `
export default async function handler(_, res) {
res.send('from login.js')
}
`,
'pages/api/user/[id].js': `
export const config = {
runtime: 'experimental-edge',
}
export default async function handler() {
return new Response('from [id].js')
}`,
},
dependencies: {},
})
})
afterAll(() => next.destroy())

it('more specific route should match', async () => {
const res = await fetchViaHTTP(next.url, '/api/user/login')
expect(await res.text()).toBe('from login.js')
})
})
1 change: 1 addition & 0 deletions test/lib/next-test-utils.js
Expand Up @@ -114,6 +114,7 @@ export function renderViaHTTP(appPort, pathname, query, opts) {
return fetchViaHTTP(appPort, pathname, query, opts).then((res) => res.text())
}

/** @return {Promise<Response & {buffer: any} & {headers: any}>} */
export function fetchViaHTTP(appPort, pathname, query, opts) {
const url = `${pathname}${
typeof query === 'string' ? query : query ? `?${qs.stringify(query)}` : ''
Expand Down