Skip to content

Commit 4b53c21

Browse files
piehpieh
authored
fix: reduce reliance on PACKAGE_PATH (#340)
* expose required-server-files manifest * run tests on all pull requests * feat: reduce reliance on PACKAGE_PATH * chore: rename PluginContext.resolve to PluginContext.resolve * chore: rename PluginContext.packagePath to PluginContext.relativeAppDir * chore: remove more usage of package path * chore: remove one more usage (? not sure if this is right, but for pnpm it should be) * test: fix server.test.ts * test: fix plugin-context.test.ts * test: fix static.test.ts * test: fix copy-next-code.test.ts * test: adjust test name * test: adjust test name vol 2 * fix: lint, remove unused * test: fix win32 variant for plugin-context * test: drop .only * fix: required-server-files might not have relativeAppDir in some versions before 13.5, so add fallback as we try to check it before checking version * Revert "run tests on all pull requests" This reverts commit 6b17a79baa7bdcbacb9882e8dc4415ddaeebf504. * chore: clarify source of relativeAppDir value in jsdoc * chore: add some context around check wether publishDir points to site source directory and not output directory
1 parent 9543204 commit 4b53c21

13 files changed

+337
-200
lines changed

‎src/build/content/server.test.ts

+22-24
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { NetlifyPluginOptions } from '@netlify/build'
55
import { expect, test, vi, describe, beforeEach } from 'vitest'
66

77
import { mockFileSystem } from '../../../tests/index.js'
8-
import { PluginContext } from '../plugin-context.js'
8+
import { PluginContext, RequiredServerFilesManifest } from '../plugin-context.js'
99

1010
import { copyNextServerCode, verifyHandlerDirStructure } from './server.js'
1111

@@ -79,7 +79,10 @@ const mockPluginOptions = {
7979

8080
const fixtures = {
8181
get simple() {
82-
const reqServerFiles = JSON.stringify({ config: { distDir: '.next' } })
82+
const reqServerFiles = JSON.stringify({
83+
config: { distDir: '.next' },
84+
relativeAppDir: '',
85+
} as RequiredServerFilesManifest)
8386
const reqServerPath = '.next/required-server-files.json'
8487
const reqServerPathStandalone = join('.next/standalone', reqServerPath)
8588
const buildIDPath = join('.netlify/functions-internal/___netlify-server-handler/.next/BUILD_ID')
@@ -92,7 +95,10 @@ const fixtures = {
9295
return { ...mockFS, reqServerFiles, reqServerPathStandalone, ctx }
9396
},
9497
get monorepo() {
95-
const reqServerFiles = JSON.stringify({ config: { distDir: '.next' } })
98+
const reqServerFiles = JSON.stringify({
99+
config: { distDir: '.next' },
100+
relativeAppDir: 'apps/my-app',
101+
} as RequiredServerFilesManifest)
96102
const reqServerPath = 'apps/my-app/.next/required-server-files.json'
97103
const reqServerPathStandalone = join('apps/my-app/.next/standalone', reqServerPath)
98104
const buildIDPath = join(
@@ -114,7 +120,8 @@ const fixtures = {
114120
get nxIntegrated() {
115121
const reqServerFiles = JSON.stringify({
116122
config: { distDir: '../../dist/apps/my-app/.next' },
117-
})
123+
relativeAppDir: 'apps/my-app',
124+
} as RequiredServerFilesManifest)
118125
const reqServerPath = 'dist/apps/my-app/.next/required-server-files.json'
119126
const reqServerPathStandalone = join('dist/apps/my-app/.next/standalone', reqServerPath)
120127
const buildIDPath = join(
@@ -135,7 +142,10 @@ const fixtures = {
135142
return { ...mockFS, reqServerFiles, reqServerPathStandalone, ctx }
136143
},
137144
get monorepoMissingPackagePath() {
138-
const reqServerFiles = JSON.stringify({ config: { distDir: '.next' } })
145+
const reqServerFiles = JSON.stringify({
146+
config: { distDir: '.next' },
147+
relativeAppDir: 'apps/my-app',
148+
} as RequiredServerFilesManifest)
139149
const reqServerPath = 'apps/my-app/.next/required-server-files.json'
140150
const reqServerPathStandalone = join('apps/my-app/.next/standalone', reqServerPath)
141151
const buildIDPath = join(
@@ -155,7 +165,10 @@ const fixtures = {
155165
return { ...mockFS, reqServerFiles, reqServerPathStandalone, ctx }
156166
},
157167
get simpleMissingBuildID() {
158-
const reqServerFiles = JSON.stringify({ config: { distDir: '.next' } })
168+
const reqServerFiles = JSON.stringify({
169+
config: { distDir: '.next' },
170+
relativeAppDir: 'apps/my-app',
171+
} as RequiredServerFilesManifest)
159172
const reqServerPath = 'apps/my-app/.next/required-server-files.json'
160173
const reqServerPathStandalone = join('apps/my-app/.next/standalone', reqServerPath)
161174
mockFS = mockFileSystem({
@@ -197,7 +210,7 @@ describe('copyNextServerCode', () => {
197210
const { cwd, ctx, reqServerPathStandalone } = fixtures.nxIntegrated
198211
await copyNextServerCode(ctx)
199212
expect(await readFile(join(cwd, reqServerPathStandalone), 'utf-8')).toBe(
200-
'{"config":{"distDir":".next"}}',
213+
'{"config":{"distDir":".next"},"relativeAppDir":"apps/my-app"}',
201214
)
202215
})
203216
})
@@ -231,27 +244,12 @@ describe('verifyHandlerDirStructure', () => {
231244
})
232245

233246
// case of misconfigured monorepo (no PACKAGE_PATH)
234-
test('should fail build in monorepo with PACKAGE_PATH missing with helpful guidance', async () => {
247+
test('should not fail build in monorepo with PACKAGE_PATH missing', async () => {
235248
const { ctx } = fixtures.monorepoMissingPackagePath
236249
await copyNextServerCode(ctx)
237250
await verifyHandlerDirStructure(ctx)
238251

239-
expect(mockFailBuild).toBeCalledTimes(1)
240-
expect(mockFailBuild).toHaveBeenCalledWith(
241-
`Failed creating server handler. BUILD_ID file not found at expected location "${join(
242-
process.cwd(),
243-
'.netlify/functions-internal/___netlify-server-handler/.next/BUILD_ID',
244-
)}".
245-
246-
It looks like your site is part of monorepo and Netlify is currently not configured correctly for this case.
247-
248-
Current package path: <not set>
249-
Package path candidates:
250-
- "apps/my-app"
251-
252-
Refer to https://docs.netlify.com/configure-builds/monorepos/ for more information about monorepo configuration.`,
253-
undefined,
254-
)
252+
expect(mockFailBuild).not.toHaveBeenCalled()
255253
})
256254

257255
// just missing BUILD_ID

‎src/build/content/server.ts

+6-43
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
import { createRequire } from 'node:module'
1313
// eslint-disable-next-line no-restricted-imports
1414
import { dirname, join, resolve, sep } from 'node:path'
15-
import { sep as posixSep, relative as posixRelative, join as posixJoin } from 'node:path/posix'
15+
import { sep as posixSep, join as posixJoin } from 'node:path/posix'
1616

1717
import glob from 'fast-glob'
1818

@@ -34,7 +34,8 @@ export const copyNextServerCode = async (ctx: PluginContext): Promise<void> => {
3434
// nx monorepos and other setups where the dist directory is modified
3535
const reqServerFilesPath = join(
3636
ctx.standaloneRootDir,
37-
ctx.relPublishDir,
37+
ctx.relativeAppDir,
38+
ctx.requiredServerFiles.config.distDir,
3839
'required-server-files.json',
3940
)
4041
try {
@@ -55,7 +56,7 @@ export const copyNextServerCode = async (ctx: PluginContext): Promise<void> => {
5556
// this means the path got altered by a plugin like nx and contained ../../ parts so we have to reset it
5657
// to point to the correct lambda destination
5758
if (
58-
toPosixPath(ctx.distDir).replace(new RegExp(`^${ctx.packagePath}/?`), '') !==
59+
toPosixPath(ctx.distDir).replace(new RegExp(`^${ctx.relativeAppDir}/?`), '') !==
5960
reqServerFiles.config.distDir
6061
) {
6162
// set the distDir to the latest path portion of the publish dir
@@ -161,7 +162,7 @@ export const copyNextDependencies = async (ctx: PluginContext): Promise<void> =>
161162
await cp(src, dest, { recursive: true, verbatimSymlinks: true, force: true })
162163

163164
if (entry === 'node_modules') {
164-
await recreateNodeModuleSymlinks(ctx.resolve('node_modules'), dest)
165+
await recreateNodeModuleSymlinks(ctx.resolveFromSiteDir('node_modules'), dest)
165166
}
166167
})
167168

@@ -282,46 +283,8 @@ export const verifyHandlerDirStructure = async (ctx: PluginContext) => {
282283

283284
const expectedBuildIDPath = join(ctx.serverHandlerDir, runConfig.distDir, 'BUILD_ID')
284285
if (!existsSync(expectedBuildIDPath)) {
285-
let additionalGuidance = ''
286-
try {
287-
const paths = await glob('**/BUILD_ID', {
288-
cwd: ctx.serverHandlerRootDir,
289-
dot: true,
290-
absolute: true,
291-
ignore: ['**/node_modules/**'],
292-
})
293-
294-
const potentiallyNeededPackagePaths = paths
295-
.map((path) => {
296-
const relativePathToBuildID = posixRelative(
297-
toPosixPath(ctx.serverHandlerDir),
298-
toPosixPath(path),
299-
)
300-
// remove <distDir>/BUILD_ID from the path to get the potential package path
301-
const removedDistDirBuildId = relativePathToBuildID.replace(
302-
new RegExp(`/?${toPosixPath(runConfig.distDir)}/BUILD_ID$`),
303-
'',
304-
)
305-
return removedDistDirBuildId === relativePathToBuildID ? '' : removedDistDirBuildId
306-
})
307-
.filter(Boolean)
308-
309-
if (potentiallyNeededPackagePaths.length !== 0) {
310-
additionalGuidance = `\n\nIt looks like your site is part of monorepo and Netlify is currently not configured correctly for this case.\n\nCurrent package path: ${
311-
ctx.packagePath ? `"${ctx.packagePath}"` : '<not set>'
312-
}\nPackage path candidates:\n${potentiallyNeededPackagePaths
313-
.map((path) => ` - "${path}"`)
314-
.join(
315-
'\n',
316-
)}\n\nRefer to https://docs.netlify.com/configure-builds/monorepos/ for more information about monorepo configuration.`
317-
}
318-
} catch {
319-
// ignore error when generating additional guidance - we do best effort to provide the guidance, but if generating guidance fails,
320-
// we still want to show proper error message (even if it lacks the additional guidance)
321-
}
322-
323286
ctx.failBuild(
324-
`Failed creating server handler. BUILD_ID file not found at expected location "${expectedBuildIDPath}".${additionalGuidance}`,
287+
`Failed creating server handler. BUILD_ID file not found at expected location "${expectedBuildIDPath}".`,
325288
)
326289
}
327290
}

‎src/build/content/static.test.ts

+33-3
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,21 @@
11
import { Buffer } from 'node:buffer'
22
import { join } from 'node:path'
3+
import { inspect } from 'node:util'
34

45
import type { NetlifyPluginOptions } from '@netlify/build'
56
import glob from 'fast-glob'
6-
import { beforeEach, describe, expect, test, vi } from 'vitest'
7+
import { Mock, beforeEach, describe, expect, test, vi } from 'vitest'
78

89
import { mockFileSystem } from '../../../tests/index.js'
910
import { FixtureTestContext, createFsFixture } from '../../../tests/utils/fixture.js'
10-
import { PluginContext } from '../plugin-context.js'
11+
import { PluginContext, RequiredServerFilesManifest } from '../plugin-context.js'
1112

1213
import { copyStaticAssets, copyStaticContent } from './static.js'
1314

1415
type Context = FixtureTestContext & {
1516
pluginContext: PluginContext
1617
publishDir: string
18+
relativeAppDir: string
1719
}
1820
const createFsFixtureWithBasePath = (
1921
fixture: Record<string, string>,
@@ -24,6 +26,13 @@ const createFsFixtureWithBasePath = (
2426
{
2527
...fixture,
2628
[join(ctx.publishDir, 'routes-manifest.json')]: JSON.stringify({ basePath }),
29+
[join(ctx.publishDir, 'required-server-files.json')]: JSON.stringify({
30+
relativeAppDir: ctx.relativeAppDir,
31+
appDir: ctx.relativeAppDir,
32+
config: {
33+
distDir: ctx.publishDir,
34+
},
35+
} as Pick<RequiredServerFilesManifest, 'relativeAppDir' | 'appDir'>),
2736
},
2837
ctx,
2938
)
@@ -37,25 +46,45 @@ async function readDirRecursive(dir: string) {
3746
return paths
3847
}
3948

49+
let failBuildMock: Mock<
50+
Parameters<PluginContext['utils']['build']['failBuild']>,
51+
ReturnType<PluginContext['utils']['build']['failBuild']>
52+
>
53+
54+
const dontFailTest: PluginContext['utils']['build']['failBuild'] = () => {
55+
return undefined as never
56+
}
57+
4058
describe('Regular Repository layout', () => {
4159
beforeEach<Context>((ctx) => {
60+
failBuildMock = vi.fn((msg, err) => {
61+
expect.fail(`failBuild should not be called, was called with ${inspect({ msg, err })}`)
62+
})
4263
ctx.publishDir = '.next'
64+
ctx.relativeAppDir = ''
4365
ctx.pluginContext = new PluginContext({
4466
constants: {
4567
PUBLISH_DIR: ctx.publishDir,
4668
},
47-
utils: { build: { failBuild: vi.fn() } as unknown },
69+
utils: {
70+
build: {
71+
failBuild: failBuildMock,
72+
} as unknown,
73+
},
4874
} as NetlifyPluginOptions)
4975
})
5076

5177
test<Context>('should clear the static directory contents', async ({ pluginContext }) => {
78+
failBuildMock.mockImplementation(dontFailTest)
5279
const { vol } = mockFileSystem({
5380
[`${pluginContext.staticDir}/remove-me.js`]: '',
5481
})
5582
await copyStaticAssets(pluginContext)
5683
expect(Object.keys(vol.toJSON())).toEqual(
5784
expect.not.arrayContaining([`${pluginContext.staticDir}/remove-me.js`]),
5885
)
86+
// routes manifest fails to load because it doesn't exist and we expect that to fail the build
87+
expect(failBuildMock).toBeCalled()
5988
})
6089

6190
test<Context>('should link static content from the publish directory to the static directory (no basePath)', async ({
@@ -196,6 +225,7 @@ describe('Regular Repository layout', () => {
196225
describe('Mono Repository', () => {
197226
beforeEach<Context>((ctx) => {
198227
ctx.publishDir = 'apps/app-1/.next'
228+
ctx.relativeAppDir = 'apps/app-1'
199229
ctx.pluginContext = new PluginContext({
200230
constants: {
201231
PUBLISH_DIR: ctx.publishDir,

‎src/build/content/static.ts

+4-2
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,10 @@ export const copyStaticAssets = async (ctx: PluginContext): Promise<void> => {
4242
try {
4343
await rm(ctx.staticDir, { recursive: true, force: true })
4444
const { basePath } = await ctx.getRoutesManifest()
45-
if (existsSync(ctx.resolve('public'))) {
46-
await cp(ctx.resolve('public'), join(ctx.staticDir, basePath), { recursive: true })
45+
if (existsSync(ctx.resolveFromSiteDir('public'))) {
46+
await cp(ctx.resolveFromSiteDir('public'), join(ctx.staticDir, basePath), {
47+
recursive: true,
48+
})
4749
}
4850
if (existsSync(join(ctx.publishDir, 'static'))) {
4951
await cp(join(ctx.publishDir, 'static'), join(ctx.staticDir, basePath, '_next/static'), {

‎src/build/functions/server.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ const copyHandlerDependencies = async (ctx: PluginContext) => {
3030
// The distDir must not be the package path therefore we need to rely on the
3131
// serverHandlerDir instead of the serverHandlerRootDir
3232
// therefore we need to remove the package path from the filePath
33-
join(ctx.serverHandlerDir, relative(ctx.packagePath, filePath)),
33+
join(ctx.serverHandlerDir, relative(ctx.relativeAppDir, filePath)),
3434
{
3535
recursive: true,
3636
force: true,
@@ -84,7 +84,7 @@ const getHandlerFile = async (ctx: PluginContext): Promise<string> => {
8484

8585
// In this case it is a monorepo and we need to use a own template for it
8686
// as we have to change the process working directory
87-
if (ctx.packagePath.length !== 0) {
87+
if (ctx.relativeAppDir.length !== 0) {
8888
const template = await readFile(join(templatesDir, 'handler-monorepo.tmpl.js'), 'utf-8')
8989

9090
return template

‎src/build/image-cdn.test.ts

+31-25
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { NextConfigComplete } from 'next/dist/server/config-shared.js'
44
import { TestContext, beforeEach, describe, expect, test } from 'vitest'
55

66
import { setImageConfig } from './image-cdn.js'
7-
import { PluginContext } from './plugin-context.js'
7+
import { PluginContext, type RequiredServerFilesManifest } from './plugin-context.js'
88

99
type ImageCDNTestContext = TestContext & {
1010
pluginContext: PluginContext
@@ -20,12 +20,14 @@ describe('Image CDN', () => {
2020
})
2121

2222
test<ImageCDNTestContext>('adds redirect to Netlify Image CDN when default image loader is used', async (ctx) => {
23-
ctx.pluginContext._buildConfig = {
24-
images: {
25-
path: '/_next/image',
26-
loader: 'default',
27-
},
28-
} as NextConfigComplete
23+
ctx.pluginContext._requiredServerFiles = {
24+
config: {
25+
images: {
26+
path: '/_next/image',
27+
loader: 'default',
28+
},
29+
} as NextConfigComplete,
30+
} as RequiredServerFilesManifest
2931

3032
await setImageConfig(ctx.pluginContext)
3133

@@ -46,13 +48,15 @@ describe('Image CDN', () => {
4648
})
4749

4850
test<ImageCDNTestContext>('does not add redirect to Netlify Image CDN when non-default loader is used', async (ctx) => {
49-
ctx.pluginContext._buildConfig = {
50-
images: {
51-
path: '/_next/image',
52-
loader: 'custom',
53-
loaderFile: './custom-loader.js',
54-
},
55-
} as NextConfigComplete
51+
ctx.pluginContext._requiredServerFiles = {
52+
config: {
53+
images: {
54+
path: '/_next/image',
55+
loader: 'custom',
56+
loaderFile: './custom-loader.js',
57+
},
58+
} as NextConfigComplete,
59+
} as RequiredServerFilesManifest
5660

5761
await setImageConfig(ctx.pluginContext)
5862

@@ -73,17 +77,19 @@ describe('Image CDN', () => {
7377
})
7478

7579
test<ImageCDNTestContext>('handles custom images.path', async (ctx) => {
76-
ctx.pluginContext._buildConfig = {
77-
images: {
78-
// Next.js automatically adds basePath to images.path (when user does not set custom `images.path` in their config)
79-
// if user sets custom `images.path` - it will be used as-is (so user need to cover their basePath by themselves
80-
// if they want to have it in their custom image endpoint
81-
// see https://github.com/vercel/next.js/blob/bb105ef4fbfed9d96a93794eeaed956eda2116d8/packages/next/src/server/config.ts#L426-L432)
82-
// either way `images.path` we get is final config with everything combined so we want to use it as-is
83-
path: '/base/path/_custom/image/endpoint',
84-
loader: 'default',
85-
},
86-
} as NextConfigComplete
80+
ctx.pluginContext._requiredServerFiles = {
81+
config: {
82+
images: {
83+
// Next.js automatically adds basePath to images.path (when user does not set custom `images.path` in their config)
84+
// if user sets custom `images.path` - it will be used as-is (so user need to cover their basePath by themselves
85+
// if they want to have it in their custom image endpoint
86+
// see https://github.com/vercel/next.js/blob/bb105ef4fbfed9d96a93794eeaed956eda2116d8/packages/next/src/server/config.ts#L426-L432)
87+
// either way `images.path` we get is final config with everything combined so we want to use it as-is
88+
path: '/base/path/_custom/image/endpoint',
89+
loader: 'default',
90+
},
91+
} as NextConfigComplete,
92+
} as RequiredServerFilesManifest
8793

8894
await setImageConfig(ctx.pluginContext)
8995

0 commit comments

Comments
 (0)