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: resolve next-server from next app directory and not from plugin #2059

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
8d8d8ae
fix: resolve next-server from next app directory and not from plugin
pieh Apr 21, 2023
e7ae048
fix: update api handler
pieh Apr 21, 2023
15c1bdd
chore: export type from server module
pieh Apr 21, 2023
4b64c17
fix: server.spec.ts
pieh Apr 21, 2023
1933824
chore: move next server throwing to runtime
pieh Apr 21, 2023
e06157d
fix: restore trying to resolve from plugin and not appdir
pieh Apr 21, 2023
ffae6a2
fix: use relative import paths not absolute
pieh Apr 21, 2023
d50c047
fix: server.spec.ts (again)
pieh Apr 21, 2023
c5a4afd
fix: index.spec.ts
pieh Apr 21, 2023
48468bc
chore: diff cleanup
pieh Apr 21, 2023
33c28ae
fix: handle function config parsing as well
pieh Apr 25, 2023
be8bc6e
fix: adjust import
pieh Apr 25, 2023
caef767
fix: server.spec.ts (again vol 2)
pieh Apr 25, 2023
56aa4f7
test: add integration test
pieh Apr 26, 2023
1e48952
Merge remote-tracking branch 'origin/main' into fix/use-next-director…
pieh Apr 26, 2023
22e2fa2
test: log npm version
pieh Apr 26, 2023
6e229c9
test: install newer npm/node
pieh Apr 26, 2023
74ab70e
test: bump timeout for setup
pieh Apr 26, 2023
e4fadf4
test: ensure test page is ssr
pieh Apr 26, 2023
1774f47
refactor: get rid of unneded new helper
pieh Apr 26, 2023
bbff3a9
chore: cleanup some debugging logs
pieh Apr 27, 2023
ade1a2e
fix: add fallback in case findModuleBase will be false
pieh Apr 27, 2023
610d73f
refactor: use one-parameter object for makeHandler functions
pieh Apr 27, 2023
917d3ce
Merge remote-tracking branch 'origin/main' into fix/use-next-director…
pieh Apr 27, 2023
1b01485
Merge remote-tracking branch 'origin/main' into fix/use-next-director…
pieh Apr 28, 2023
04f3164
refactor: don't rely on MODULE_NOT_FOUND for lack of advanced API rou…
pieh Apr 28, 2023
92dcf3a
Update packages/runtime/src/templates/server.ts
pieh May 2, 2023
66a9670
fix: streamline no-shadow handling
pieh May 2, 2023
6e4d78a
Merge branch 'main' into fix/use-next-directory-when-resolving-server…
LekoArts May 3, 2023
8f71783
chore: automatic linting
LekoArts May 3, 2023
9efa6cb
chore: fix linting
LekoArts May 3, 2023
a92dbac
chore: post-commit linting :rolleyes:
LekoArts May 3, 2023
767dca9
Merge branch 'main' into fix/use-next-directory-when-resolving-server…
LekoArts May 3, 2023
40c6469
Merge branch 'main' into fix/use-next-directory-when-resolving-server…
LekoArts May 4, 2023
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
48 changes: 48 additions & 0 deletions packages/runtime/src/helpers/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,54 @@ const getServerFile = (root: string, includeBase = true) => {
return findModuleFromBase({ candidates, paths: [root] })
}

/**
* Try to find next-server module in few locations (to support different next versions) and in few context (try to resolve from app location and from this module)
*/
export const getNextServerModulePath = (root: string): string | null => {
// first let's try to use app location directory to find next-server
try {
const nextServerModuleLocation = getServerFile(root, false)
if (nextServerModuleLocation) {
pieh marked this conversation as resolved.
Show resolved Hide resolved
return nextServerModuleLocation
}
} catch (error) {
if (!error.message.includes('Cannot find module')) {
// A different error, so rethrow it
throw error
}
}

// if we didn't find it, let's try to resolve "next" package from this module
try {
// next >= 11.0.1. Yay breaking changes in patch releases!
const nextServerModuleLocation = require.resolve('next/dist/server/next-server')
if (nextServerModuleLocation) {
return nextServerModuleLocation
}
} catch (error) {
if (!error.message.includes("Cannot find module 'next/dist/server/next-server'")) {
// A different error, so rethrow it
throw error
}
// Probably an old version of next, so fall through and find it elsewhere.
}

try {
// next < 11.0.1
// eslint-disable-next-line n/no-missing-require
const nextServerModuleLocation = require.resolve('next/dist/next-server/server/next-server')
if (nextServerModuleLocation) {
return nextServerModuleLocation
}
} catch (error) {
if (!error.message.includes("Cannot find module 'next/dist/next-server/server/next-server'")) {
throw error
}
}

return null
}

/**
* Find the source file for a given page route
*/
Expand Down
15 changes: 13 additions & 2 deletions packages/runtime/src/helpers/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { getHandler } from '../templates/getHandler'
import { getResolverForPages, getResolverForSourceFiles } from '../templates/getPageResolver'

import { ApiConfig, ApiRouteType, extractConfigFromFile, isEdgeConfig } from './analysis'
import { getSourceFileForPage } from './files'
import { getNextServerModulePath, getSourceFileForPage } from './files'
import { writeFunctionConfiguration } from './functionsMetaData'
import { getFunctionNameForPage } from './utils'

Expand All @@ -41,6 +41,11 @@ export const generateFunctions = async (
const functionDir = join(functionsDir, HANDLER_FUNCTION_NAME)
const publishDir = relative(functionDir, publish)

const nextServerModuleAbsoluteLocation = getNextServerModulePath(appDir)
const nextServerModuleRelativeLocation = nextServerModuleAbsoluteLocation
? relative(functionDir, nextServerModuleAbsoluteLocation)
: undefined

Comment on lines +46 to +49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if nextServerModuleAbsoluteLocation is falsy we could warn here that non-SSG routes will fail (tho that might be noisy, so if we want warning like that maybe we add one once we know if we need lambdas for routes)

for (const { route, config, compiled } of apiRoutes) {
// Don't write a lambda if the runtime is edge
if (isEdgeConfig(config.runtime)) {
Expand All @@ -51,6 +56,7 @@ export const generateFunctions = async (
config,
publishDir,
appDir: relative(functionDir, appDir),
nextServerModuleRelativeLocation,
})
const functionName = getFunctionNameForPage(route, config.type === ApiRouteType.BACKGROUND)
await ensureDir(join(functionsDir, functionName))
Expand Down Expand Up @@ -80,7 +86,12 @@ export const generateFunctions = async (
}

const writeHandler = async (functionName: string, functionTitle: string, isODB: boolean) => {
const handlerSource = await getHandler({ isODB, publishDir, appDir: relative(functionDir, appDir) })
const handlerSource = await getHandler({
isODB,
publishDir,
appDir: relative(functionDir, appDir),
nextServerModuleRelativeLocation,
})
await ensureDir(join(functionsDir, functionName))

// write main handler file (standard or ODB)
Expand Down
22 changes: 13 additions & 9 deletions packages/runtime/src/templates/getApiHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ const { URLSearchParams, URL } = require('url')

const { Bridge } = require('@vercel/node-bridge/bridge')

const { getMultiValueHeaders, getNextServer } = require('./handlerUtils')
const { getMultiValueHeaders } = require('./handlerUtils')
/* eslint-enable @typescript-eslint/no-var-requires */

type Mutable<T> = {
-readonly [K in keyof T]: T[K]
}

// We return a function and then call `toString()` on it to serialise it as the launcher function

const makeHandler = (conf: NextConfig, app, pageRoot, page) => {
// eslint-disable-next-line max-params
const makeHandler = (conf: NextConfig, app, pageRoot, page, NextServer: NextServerType) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since another parameter is being added, consider converting the parameters to one parameter object.

Suggested change
// eslint-disable-next-line max-params
const makeHandler = (conf: NextConfig, app, pageRoot, page, NextServer: NextServerType) => {
const makeHandler = ({ conf, app, pageRoot, page, NextServer }: MakeHandlerParams) => {

and update at the call sites.

Copy link
Contributor Author

@pieh pieh Apr 27, 2023

Choose a reason for hiding this comment

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

done in 610d73f

// Change working directory into the site root, unless using Nx, which moves the
// dist directory and handles this itself
const dir = path.resolve(__dirname, app)
Expand Down Expand Up @@ -64,7 +64,6 @@ const makeHandler = (conf: NextConfig, app, pageRoot, page) => {
const url = event.rawUrl ? new URL(event.rawUrl) : new URL(path, process.env.URL || 'http://n')
const port = Number.parseInt(url.port) || 80

const NextServer: NextServerType = getNextServer()
const nextServer = new NextServer({
conf,
dir,
Expand Down Expand Up @@ -118,18 +117,23 @@ export const getApiHandler = ({
config,
publishDir = '../../../.next',
appDir = '../../..',
nextServerModuleRelativeLocation,
}: {
page: string
config: ApiConfig
publishDir?: string
appDir?: string
}): string =>
// This is a string, but if you have the right editor plugin it should format as js
javascript/* javascript */ `
nextServerModuleRelativeLocation: string | undefined
}): string => javascript/* javascript */ `
if (!${JSON.stringify(nextServerModuleRelativeLocation)}) {
throw new Error('Could not find Next.js server')
}

const { Server } = require("http");
// We copy the file here rather than requiring from the node module
const { Bridge } = require("./bridge");
const { getMultiValueHeaders, getNextServer } = require('./handlerUtils')
const { getMultiValueHeaders } = require('./handlerUtils')
const NextServer = require(${JSON.stringify(nextServerModuleRelativeLocation)}).default

${config.type === ApiRouteType.SCHEDULED ? `const { schedule } = require("@netlify/functions")` : ''}

Expand All @@ -138,7 +142,7 @@ export const getApiHandler = ({
let staticManifest
const path = require("path");
const pageRoot = path.resolve(path.join(__dirname, "${publishDir}", "server"));
const handler = (${makeHandler.toString()})(config, "${appDir}", pageRoot, ${JSON.stringify(page)})
const handler = (${makeHandler.toString()})(config, "${appDir}", pageRoot, ${JSON.stringify(page)}, NextServer)
exports.handler = ${
config.type === ApiRouteType.SCHEDULED ? `schedule(${JSON.stringify(config.schedule)}, handler);` : 'handler'
}
Expand Down
43 changes: 32 additions & 11 deletions packages/runtime/src/templates/getHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import { outdent as javascript } from 'outdent'

import type { NextConfig } from '../helpers/config'

import { NextServerType } from './handlerUtils'
import type { NetlifyNextServerType } from './server'

/* eslint-disable @typescript-eslint/no-var-requires */
const { promises } = require('fs')
const { Server } = require('http')
Expand All @@ -21,16 +24,24 @@ const {
getPrefetchResponse,
normalizePath,
} = require('./handlerUtils')
const { NetlifyNextServer } = require('./server')
const { getNetlifyNextServer } = require('./server')
/* eslint-enable @typescript-eslint/no-var-requires */

type Mutable<T> = {
-readonly [K in keyof T]: T[K]
}

// We return a function and then call `toString()` on it to serialise it as the launcher function
// eslint-disable-next-line max-params, max-lines-per-function
const makeHandler = (conf: NextConfig, app, pageRoot, staticManifest: Array<[string, string]> = [], mode = 'ssr') => {
// eslint-disable-next-line max-lines-per-function
const makeHandler = (
conf: NextConfig,
app: string,
pageRoot,
NextServer: NextServerType,
staticManifest: Array<[string, string]> = [],
mode = 'ssr',
// eslint-disable-next-line max-params
) => {
// Change working directory into the site root, unless using Nx, which moves the
// dist directory and handles this itself
const dir = path.resolve(__dirname, app)
Expand All @@ -44,6 +55,8 @@ const makeHandler = (conf: NextConfig, app, pageRoot, staticManifest: Array<[str
require.resolve('./pages.js')
} catch {}

const NetlifyNextServer: NetlifyNextServerType = getNetlifyNextServer(NextServer)

const ONE_YEAR_IN_SECONDS = 31536000

// React assumes you want development mode if NODE_ENV is unset.
Expand Down Expand Up @@ -86,7 +99,7 @@ const makeHandler = (conf: NextConfig, app, pageRoot, staticManifest: Array<[str
port,
},
{
revalidateToken: customContext.odb_refresh_hooks,
revalidateToken: customContext?.odb_refresh_hooks,
pieh marked this conversation as resolved.
Show resolved Hide resolved
},
)
const requestHandler = nextServer.getRequestHandler()
Expand Down Expand Up @@ -177,15 +190,23 @@ const makeHandler = (conf: NextConfig, app, pageRoot, staticManifest: Array<[str
}
}

export const getHandler = ({ isODB = false, publishDir = '../../../.next', appDir = '../../..' }): string =>
// This is a string, but if you have the right editor plugin it should format as js
javascript/* javascript */ `
export const getHandler = ({
isODB = false,
publishDir = '../../../.next',
appDir = '../../..',
nextServerModuleRelativeLocation,
}): string => javascript/* javascript */ `
if (!${JSON.stringify(nextServerModuleRelativeLocation)}) {
throw new Error('Could not find Next.js server')
Copy link
Contributor Author

@pieh pieh Apr 21, 2023

Choose a reason for hiding this comment

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

In current state of this PR, this is thrown on lambda invocation and not during the build, so it restores the that behavior prior to #1950 on when the error is thrown - not the hill I will die on and I can move it back to build time.

The other changes in this PR make it more likely to find next-server than before, but I don't have 100% confidence in it, so that's why I move this error throwing back here as that was behavior for a long time.

Without that move, builds for fully static sites that hit current error are just blocked until we implement logic to determine wether we need page route handlers at all and skip it altogether if not needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, i think that makes sense tbh. We only need the server at runtime so let's go with this.

}

const { Server } = require("http");
const { promises } = require("fs");
// We copy the file here rather than requiring from the node module
const { Bridge } = require("./bridge");
const { augmentFsModule, getMaxAge, getMultiValueHeaders, getPrefetchResponse, getNextServer, normalizePath } = require('./handlerUtils')
const { NetlifyNextServer } = require('./server')
const { augmentFsModule, getMaxAge, getMultiValueHeaders, getPrefetchResponse, normalizePath } = require('./handlerUtils')
const { getNetlifyNextServer } = require('./server')
const NextServer = require(${JSON.stringify(nextServerModuleRelativeLocation)}).default

${isODB ? `const { builder } = require("@netlify/functions")` : ''}
const { config } = require("${publishDir}/required-server-files.json")
Expand All @@ -197,7 +218,7 @@ export const getHandler = ({ isODB = false, publishDir = '../../../.next', appDi
const pageRoot = path.resolve(path.join(__dirname, "${publishDir}", "server"));
exports.handler = ${
isODB
? `builder((${makeHandler.toString()})(config, "${appDir}", pageRoot, staticManifest, 'odb'));`
: `(${makeHandler.toString()})(config, "${appDir}", pageRoot, staticManifest, 'ssr');`
? `builder((${makeHandler.toString()})(config, "${appDir}", pageRoot, NextServer, staticManifest, 'odb'));`
: `(${makeHandler.toString()})(config, "${appDir}", pageRoot, NextServer, staticManifest, 'ssr');`
}
`
31 changes: 0 additions & 31 deletions packages/runtime/src/templates/handlerUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,37 +160,6 @@ export const augmentFsModule = ({
}) as typeof promises.stat
}

/**
* Next.js has an annoying habit of needing deep imports, but then moving those in patch releases. This is our abstraction.
*/
export const getNextServer = (): NextServerType => {
let NextServer: NextServerType
try {
// next >= 11.0.1. Yay breaking changes in patch releases!
// eslint-disable-next-line @typescript-eslint/no-var-requires
NextServer = require('next/dist/server/next-server').default
} catch (error) {
if (!error.message.includes("Cannot find module 'next/dist/server/next-server'")) {
// A different error, so rethrow it
throw error
}
// Probably an old version of next, so fall through and find it elsewhere.
}

if (!NextServer) {
try {
// next < 11.0.1
// eslint-disable-next-line n/no-missing-require, import/no-unresolved, @typescript-eslint/no-var-requires
NextServer = require('next/dist/next-server/server/next-server').default
} catch (error) {
if (!error.message.includes("Cannot find module 'next/dist/next-server/server/next-server'")) {
throw error
}
throw new Error('Could not find Next.js server')
}
}
return NextServer
}
/**
* Prefetch requests are used to check for middleware redirects, and shouldn't trigger SSR.
*/
Expand Down