Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ site/src/**/*.md
.verdaccio-storage
.eslintcache
_test_out/**
tests/unit/utils/tmp
*.crt
*.key

Expand Down
2 changes: 1 addition & 1 deletion src/utils/telemetry/report-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const dirPath = dirname(fileURLToPath(import.meta.url))
*/
// @ts-expect-error TS(7006) FIXME: Parameter 'error' implicitly has an 'any' type.
export const reportError = async function (error, config = {}) {
if (isCI) {
if (isCI || process.env.CI) {
return
}
// convert a NotifiableError to an error class
Expand Down
11 changes: 7 additions & 4 deletions tests/integration/commands/dev/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,14 +382,15 @@ describe.concurrent('command/dev', () => {
t.expect(nodeVer).toBeGreaterThanOrEqual(18)

await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
const externalServer = startExternalServer({
host: '127.0.0.1',
port: 4567,
port: targetPort,
})
await builder.build()

await withDevServer(
{ cwd: builder.directory, command: 'node', framework: '#custom', targetPort: 4567, skipWaitPort: true },
{ cwd: builder.directory, command: 'node', framework: '#custom', targetPort, skipWaitPort: true },
async (server) => {
const response = await fetch(`${server.url}/test`)
t.expect(response.status).toBe(200)
Expand Down Expand Up @@ -828,13 +829,15 @@ describe.concurrent('command/dev', () => {

test('deploy environment variables injected by onDev plugin hooks are injected into functions', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()

await builder
.withNetlifyToml({
config: {
plugins: [{ package: './plugins/plugin' }],
dev: {
command: 'node index.mjs',
targetPort: 4445,
targetPort,
},
},
})
Expand Down Expand Up @@ -889,7 +892,7 @@ describe.concurrent('command/dev', () => {
res.end();
})

server.listen(4445)
server.listen(${targetPort.toString()})
`,
})
.build()
Expand Down
68 changes: 47 additions & 21 deletions tests/integration/commands/dev/redirects.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { readFile, writeFile } from 'fs/promises'
import { join } from 'path'

import getPort from 'get-port'
import fetch from 'node-fetch'
import { describe, expect, test } from 'vitest'
Expand Down Expand Up @@ -32,27 +35,50 @@ describe('redirects', async () => {
})
})

await setupFixtureTests('next-app', { devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } } }, () => {
test<FixtureTestContext>('should prefer local files instead of redirect when not forced', async ({ devServer }) => {
const response = await fetch(`http://localhost:${devServer!.port}/test.txt`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.trim()).toEqual('hello world')
})

test<FixtureTestContext>('should check for the dynamic page existence before doing redirect', async ({
devServer,
}) => {
const response = await fetch(`http://localhost:${devServer!.port}/`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.toLowerCase()).not.toContain('netlify')
})
})
await setupFixtureTests(
'next-app',
{
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Env value should be a string for consistency and to match NodeJS.ProcessEnv typing.

Line 122 of this same file uses NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1'. Node coerces numeric values at spawn time, but TS's ProcessEnv type expects strings and downstream code typically does strict equality against '1'.

-      devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
+      devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' } },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' } },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/integration/commands/dev/redirects.test.ts` at line 41, The env value
for NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS is currently a number; change it to a
string to match NodeJS.ProcessEnv and existing tests (use '1' instead of 1).
Locate the devServer config object (devServer: { env: {
NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: ... } }) in the test and update the
value to '1' so it matches the usage on line with
NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' and any strict equality checks
elsewhere.

setup: async ({ fixture }) => {
const targetPort = await getPort()
const packageJsonPath = join(fixture.directory, 'package.json')
const netlifyTomlPath = join(fixture.directory, 'netlify.toml')
const packageJson = JSON.parse(await readFile(packageJsonPath, 'utf8')) as { scripts: { dev: string } }

packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
await writeFile(
netlifyTomlPath,
(
await readFile(netlifyTomlPath, 'utf8')
).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
)
Comment on lines +50 to +55
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fragile string replace — silently no-ops if targetPort = 6123 is ever reformatted.

String.prototype.replace with a literal needle returns the original string unchanged when no match is found, so a future reformat (e.g., targetPort=6123, different whitespace, or bumping the placeholder value) would leave netlify.toml pointing at the old port while next dev listens on the dynamic one — and the failure mode is a mysterious timeout rather than a clear error.

🛠️ Suggested fix: assert the replacement happened (or use a regex with a post-check)
-        await writeFile(
-          netlifyTomlPath,
-          (await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
-        )
+        const originalToml = await readFile(netlifyTomlPath, 'utf8')
+        const updatedToml = originalToml.replace(/targetPort\s*=\s*\d+/, `targetPort = ${targetPort.toString()}`)
+        if (updatedToml === originalToml) {
+          throw new Error(`Failed to substitute targetPort in ${netlifyTomlPath}`)
+        }
+        await writeFile(netlifyTomlPath, updatedToml)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await writeFile(
netlifyTomlPath,
(await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
)
const originalToml = await readFile(netlifyTomlPath, 'utf8')
const updatedToml = originalToml.replace(/targetPort\s*=\s*\d+/, `targetPort = ${targetPort.toString()}`)
if (updatedToml === originalToml) {
throw new Error(`Failed to substitute targetPort in ${netlifyTomlPath}`)
}
await writeFile(netlifyTomlPath, updatedToml)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/integration/commands/dev/redirects.test.ts` around lines 50 - 53, The
current test uses a fragile literal string replace on the netlify toml contents
via readFile/netlifyTomlPath and writeFile which will silently no-op if
formatting changes; update the logic in the test to perform the replacement with
a regex (matching variations like whitespace around `targetPort` and the value)
or run the replace and then assert that the result differs from the original
(using targetPort.toString() as the new value), and if no change occurred
throw/assert a failure before calling writeFile so the test fails loudly rather
than leaving netlify.toml unchanged.

},
},
() => {
test<FixtureTestContext>('should prefer local files instead of redirect when not forced', async ({
devServer,
}) => {
const response = await fetch(`http://localhost:${devServer!.port}/test.txt`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.trim()).toEqual('hello world')
})

test<FixtureTestContext>('should check for the dynamic page existence before doing redirect', async ({
devServer,
}) => {
const response = await fetch(`http://localhost:${devServer!.port}/`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.toLowerCase()).not.toContain('netlify')
})
},
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test('should not check the endpoint existence for hidden proxies', async (t) => {
await withSiteBuilder(t, async (builder) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,18 @@ import {
createAIGatewayTestData,
} from '../../utils/ai-gateway-helpers.js'

const DEFAULT_PORT = 9999
const SERVE_TIMEOUT = 180_000

const withFunctionsServer = async (
{
args = [],
builder,
port = DEFAULT_PORT,
port,
env = {},
}: {
args?: string[]
builder: SiteBuilder
port?: number
port: number
env?: NodeJS.ProcessEnv
},
testHandler: () => Promise<unknown>,
Expand Down Expand Up @@ -74,8 +73,9 @@ describe.concurrent('functions:serve command', () => {
})
.build()

await withFunctionsServer({ builder }, async () => {
const response = await fetch(`http://localhost:9999/.netlify/functions/ping`)
const port = await getPort()
await withFunctionsServer({ builder, args: ['--port', port.toString()], port }, async () => {
const response = await fetch(`http://localhost:${port.toString()}/.netlify/functions/ping`)
t.expect(await response.text()).toEqual('ping')
})
})
Expand Down
23 changes: 17 additions & 6 deletions tests/integration/framework-detection.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import execa from 'execa'
Comment thread
coderabbitai[bot] marked this conversation as resolved.
import getPort from 'get-port'
import fetch from 'node-fetch'
import { describe, test } from 'vitest'

Expand Down Expand Up @@ -89,6 +90,7 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should warn if using static server and `targetPort` is configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder
.withContentFile({
path: 'public/index.html',
Expand All @@ -97,7 +99,7 @@ describe.concurrent('frameworks/framework-detection', () => {
.build()

await withDevServer(
{ cwd: builder.directory, args: ['--dir', 'public', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--dir', 'public', '--target-port', targetPort.toString()] },
async ({ output, url }) => {
const response = await fetch(url)
const responseContent = await response.text()
Expand All @@ -111,11 +113,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should run `command` when both `command` and `targetPort` are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { build: { publish: 'public' } } }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand Down Expand Up @@ -185,10 +188,15 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should throw if framework=#custom but command is missing', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { dev: { framework: '#custom' } } }).build()

try {
await withDevServer({ cwd: builder.directory, args: ['--target-port', '3000'] }, async () => {}, true)
await withDevServer(
{ cwd: builder.directory, args: ['--target-port', targetPort.toString()] },
async () => {},
true,
)
t.expect.unreachable()
} catch (err) {
t.expect(err).toHaveProperty('stdout')
Expand Down Expand Up @@ -217,11 +225,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should start custom command if framework=#custom, command and targetPort are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { dev: { framework: '#custom', publish: 'public' } } }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand All @@ -237,6 +246,7 @@ describe.concurrent('frameworks/framework-detection', () => {

test(`should print specific error when command doesn't exist`, async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.build()

try {
Expand All @@ -247,7 +257,7 @@ describe.concurrent('frameworks/framework-detection', () => {
'--command',
'oops-i-did-it-again forgot-to-use-a-valid-command',
'--target-port',
'3000',
targetPort.toString(),
'--framework',
'#custom',
],
Expand Down Expand Up @@ -345,11 +355,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should not run framework detection if command and targetPort are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withContentFile({ path: 'config.toml', content: '' }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand Down
23 changes: 6 additions & 17 deletions tests/unit/utils/copy-template-dir/copy-template-dir.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@ import path from 'path'
import { fileURLToPath } from 'url'

import { readdirpPromise } from 'readdirp'
import { describe, expect, test } from 'vitest'
import { afterEach, describe, expect, test } from 'vitest'

import { copyTemplateDir } from '../../../../src/utils/copy-template-dir/copy-template-dir.js'

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)
const outDir = path.join(__dirname, '../tmp')

describe('copyTemplateDir', () => {
afterEach(() => {
fs.rmSync(outDir, { recursive: true, force: true })
})

test('should write a bunch of files', async () => {
const checkCreatedFileNames = (names: string[]) => {
expect(names).toContain('.a')
Expand All @@ -25,7 +30,6 @@ describe('copyTemplateDir', () => {
}

const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

const createdFiles = await copyTemplateDir(inDir, outDir, {})

Expand All @@ -38,45 +42,30 @@ describe('copyTemplateDir', () => {
// Checks that the files were created in the file system
const files = await readdirpPromise(outDir)
checkCreatedFileNames(files.map((file) => file.path))

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

const fileContent = fs.readFileSync(path.join(outDir, '1.txt'), 'utf-8').trim()
expect(fileContent).toBe('hello bar sama')

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings into filenames', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

expect(fs.existsSync(path.join(outDir, 'bar.txt'))).toBe(true)

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings into directory names', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

expect(fs.existsSync(path.join(outDir, 'bar'))).toBe(true)

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})
})
Loading