diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 80518cf..a11fc09 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,7 @@ jobs: path: appmap-node-${{ env.SHORT_SHA }}.tgz - run: yarn pkg-pr-new publish --commentWithDev --yarn - run: yarn lint + - run: yarn typecheck test: needs: build-and-lint diff --git a/package.json b/package.json index 610ba42..4ef01a2 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "scripts": { "prepack": "tsc -p tsconfig.build.json", "lint": "eslint src test", + "typecheck": "tsc --noEmit", "test": "jest", "watch": "tsc --watch -p tsconfig.build.json" }, diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 8b60e30..fa11e53 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -154,7 +154,9 @@ describe(Config, () => { let dir: string; beforeEach(() => { chdir((dir = tmp.dirSync().name)); - jest.replaceProperty(process, "env", {}); + // The cast is needed because next's types (pulled in through the test apps) + // augment ProcessEnv to make NODE_ENV required. + jest.replaceProperty(process, "env", {} as NodeJS.ProcessEnv); }); const origCwd = cwd(); diff --git a/src/__tests__/helpers.ts b/src/__tests__/helpers.ts index f16b195..b3df3e5 100644 --- a/src/__tests__/helpers.ts +++ b/src/__tests__/helpers.ts @@ -8,7 +8,6 @@ export function createTestFn(name: string, ...args: string[]): FunctionInfo { generator: false, id: identifier(name), params: args.map(identifier), - type: "FunctionDeclaration", }, { path: "test.js", lineno: 42 }, ); diff --git a/src/__tests__/jsxInstrument.test.ts b/src/__tests__/jsxInstrument.test.ts index 17ac79e..daa7ab1 100644 --- a/src/__tests__/jsxInstrument.test.ts +++ b/src/__tests__/jsxInstrument.test.ts @@ -19,7 +19,8 @@ function isInstrumented(program: ReturnType): boolean { return program.body.some( (n) => n.type === "VariableDeclaration" && - n.declarations?.[0]?.id?.name === "__appmapFunctionRegistry", + n.declarations[0]?.id.type === "Identifier" && + n.declarations[0].id.name === "__appmapFunctionRegistry", ); } diff --git a/src/__tests__/parameter.test.ts b/src/__tests__/parameter.test.ts index 571fe91..4e36ee0 100644 --- a/src/__tests__/parameter.test.ts +++ b/src/__tests__/parameter.test.ts @@ -1,4 +1,4 @@ -import AppMap from "../AppMap"; +import type * as AppMap from "../AppMap"; import { objectId, optParameter, parameter } from "../parameter"; class Klass { diff --git a/src/hooks/next.ts b/src/hooks/next.ts index 3b8aba8..c0e615a 100644 --- a/src/hooks/next.ts +++ b/src/hooks/next.ts @@ -20,6 +20,11 @@ interface NextConfig { webpack?: (config: WebpackConfiguration, context: WebpackContext) => WebpackConfiguration; } +// The third argument of next's loadConfig(phase, dir, opts). +interface LoadConfigOptions { + rawConfig?: boolean; +} + import { call_, identifier, literal, member, ret } from "../generate"; import { warn } from "../message"; @@ -47,7 +52,9 @@ export function transform(program: ESTree.Program): ESTree.Program { }; const thisFile = call_(identifier("require"), literal(__filename)); const injectConfig = member(thisFile, "injectConfig"); - fun.body.body = [ret(call_(injectConfig, orig))]; + // Forward the arguments rather than the declared parameters, so that we + // don't depend on how a given version of next happens to name them. + fun.body.body = [ret(call_(injectConfig, orig, identifier("arguments")))]; }, }); return program; @@ -55,8 +62,26 @@ export function transform(program: ESTree.Program): ESTree.Program { const jsExtensions = ["tsx", "ts", "js", "cjs", "mjs", "jsx"]; -export async function injectConfig(loadConfig: () => Promise): Promise { +export async function injectConfig( + loadConfig: () => Promise, + args?: IArguments, +): Promise { const result = await loadConfig(); + + // With rawConfig next asks for the config module's exports rather than the + // resolved configuration, to validate what the user configured. That object + // is a module namespace and hence not extensible, and injecting a webpack + // config into it would in any case make next believe the user has one. + const opts = args?.[2] as LoadConfigOptions | undefined; + if (opts?.rawConfig) return result; + + // Any other config we can't extend is unexpected. Don't throw, the caller + // handles it poorly, but do say so: nothing would be instrumented otherwise. + if (!Object.isExtensible(result)) { + warn("The Next.js configuration cannot be modified, AppMap will not be injected into it."); + return result; + } + const loaderPath = resolve(__dirname, "../webpack.js"); // Webpack loader injection (Next.js with --webpack or older Next.js) diff --git a/test/helpers.ts b/test/helpers.ts index 0ca1c1a..3cc4fb2 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -50,6 +50,23 @@ export function spawnAppmapNodeWithOptions( return result; } +/** + * Stop handling a spawned process' output, removing every stdout and stderr + * data listener: the logging ones installed by spawnAppmapNode as well as any + * the caller has added itself. The process is expected to be gone by now, so + * there should be nothing left to listen for. + * + * Call this once a spawned process is no longer needed: on Windows appmap-node + * spawns the target with shell: true, so the actual process is a grandchild + * which keeps the stdio pipes open even after the shell has exited. Anything it + * writes on the way out (eg. terminal escapes) would otherwise be logged after + * the test has finished, which makes Jest fail the whole run. + */ +export function detachOutput(child: ChildProcessWithoutNullStreams) { + child.stdout.removeAllListeners("data"); + child.stderr.removeAllListeners("data"); +} + let target = fwdSlashPath(cwd()); export function testDir(path: string) { diff --git a/test/httpServer.test.ts b/test/httpServer.test.ts index 98ad3a4..c45eaef 100644 --- a/test/httpServer.test.ts +++ b/test/httpServer.test.ts @@ -3,6 +3,7 @@ import { setTimeout as sleep } from "node:timers/promises"; import { SpawnAppmapNodeOptions, + detachOutput, fixAppmap, integrationTest, readAppmaps, @@ -141,7 +142,8 @@ async function makeRequest( return Buffer.concat(chunks).toString(); } -function killServer(server: ChildProcessWithoutNullStreams) { +async function killServer(server: ChildProcessWithoutNullStreams) { server.kill("SIGINT"); - return new Promise((r) => server.on("close", () => r())); + await new Promise((r) => server.on("close", () => r())); + detachOutput(server); } diff --git a/test/next.test.ts b/test/next.test.ts index 1f778b4..04d3c7b 100644 --- a/test/next.test.ts +++ b/test/next.test.ts @@ -1,73 +1,4 @@ -import { IncomingMessage, request } from "node:http"; -import { resolve } from "node:path"; +import { integrationTest } from "./helpers"; +import testNextApp from "./nextApp"; -import { getFreePort, integrationTest, readAppmaps, spawnAppmapNode } from "./helpers"; - -const nextBin = require.resolve("next/dist/bin/next", { - paths: [resolve(__dirname, "next")], -}); - -async function spawnNextJsApp(port: number) { - // On Windows, we give "node" argument explicitly because next is a js file with - // shebang (#!/usr/bin/env node) which does not work on Windows. - const app = - process.platform == "win32" - ? spawnAppmapNode("node", nextBin, "dev", "-p", String(port)) - : spawnAppmapNode(nextBin, "dev", "-p", String(port)); - - await new Promise((r) => { - const onData = (chunk: Buffer) => { - console.log("CHUNK", chunk.toString()); - if (chunk.toString().includes("Ready")) { - app.stdout.removeListener("data", onData); - r(); - } - }; - app.stdout.on("data", onData); - }); - return app; -} - -integrationTest( - "mapping a Next.js appmap", - async () => { - const port = await getFreePort(); - const app = await spawnNextJsApp(port); - const response = await makeRequest(port, "/hello"); - console.log("Response", response); - const pid = parseInt((JSON.parse(response) as unknown as { pid: string }).pid); - - await makeRequest(port, "/about"); - - app.kill("SIGINT"); - await new Promise((r) => app.once("exit", r)); - const appMaps = readAppmaps(); - // Delete response body captures because they will be different in every run - Object.values(appMaps).forEach( - (a) => - a.events?.forEach((e) => { - if ("http_server_response" in e) delete e.http_server_response.return_value; - if ("http_client_response" in e) delete e.http_client_response.return_value; - }), - ); - - expect(appMaps).toMatchSnapshot(); - - // We need to kill the next process explicitly on Windows - // because it's spawn-ed with "shell: true" and app is the shell process. - if (process.platform == "win32") process.kill(pid, "SIGINT"); - }, - 20000, -); - -async function makeRequest(port: number, path: string, method = "GET") { - const url = new URL(path, `http://localhost:${port}`); - const response = new Promise((resolve, reject) => { - const req = request(url, { method }, resolve).once("error", reject); - req.end(); - }); - - const chunks: Buffer[] = []; - for await (const chunk of await response) chunks.push(chunk as Buffer); - return Buffer.concat(chunks).toString(); -} +integrationTest("mapping a Next.js appmap", testNextApp, 60000); diff --git a/test/next16.test.ts b/test/next16.test.ts index d9164d7..caa5326 100644 --- a/test/next16.test.ts +++ b/test/next16.test.ts @@ -1,34 +1,5 @@ -import { IncomingMessage, request } from "node:http"; -import { resolve } from "node:path"; - -import { getFreePort, integrationTest, readAppmaps, spawnAppmapNode } from "./helpers"; - -const nextBin = require.resolve("next/dist/bin/next", { - paths: [resolve(__dirname, "next16")], -}); - -async function spawnNextJsApp(port: number) { - // On Windows, we give "node" argument explicitly because next is a js file with - // shebang (#!/usr/bin/env node) which does not work on Windows. - // Turbopack (the default bundler in Next.js 16) respects webpack loaders configured - // via next.config turbopack.rules, which we inject in src/hooks/next.ts. - const app = - process.platform == "win32" - ? spawnAppmapNode("node", nextBin, "dev", "-p", String(port)) - : spawnAppmapNode(nextBin, "dev", "-p", String(port)); - - await new Promise((r) => { - const onData = (chunk: Buffer) => { - console.log("CHUNK", chunk.toString()); - if (chunk.toString().includes("Ready")) { - app.stdout.removeListener("data", onData); - r(); - } - }; - app.stdout.on("data", onData); - }); - return app; -} +import { integrationTest } from "./helpers"; +import testNextApp from "./nextApp"; // Next.js 16 requires Node.js >= 20.9.0 const nodeSupported = (() => { @@ -36,46 +7,6 @@ const nodeSupported = (() => { return major > 20 || (major === 20 && minor >= 9); })(); -integrationTest.if(nodeSupported)( - "mapping a Next.js 16 appmap", - async () => { - const port = await getFreePort(); - const app = await spawnNextJsApp(port); - const response = await makeRequest(port, "/hello"); - console.log("Response", response); - const pid = parseInt((JSON.parse(response) as unknown as { pid: string }).pid); - - await makeRequest(port, "/about"); - - app.kill("SIGINT"); - await new Promise((r) => app.once("exit", r)); - const appMaps = readAppmaps(); - // Delete response body captures because they will be different in every run - Object.values(appMaps).forEach( - (a) => - a.events?.forEach((e) => { - if ("http_server_response" in e) delete e.http_server_response.return_value; - if ("http_client_response" in e) delete e.http_client_response.return_value; - }), - ); - - expect(appMaps).toMatchSnapshot(); - - // We need to kill the next process explicitly on Windows - // because it's spawn-ed with "shell: true" and app is the shell process. - if (process.platform == "win32") process.kill(pid, "SIGINT"); - }, - 60000, -); - -async function makeRequest(port: number, path: string, method = "GET") { - const url = new URL(path, `http://localhost:${port}`); - const response = new Promise((resolve, reject) => { - const req = request(url, { method }, resolve).once("error", reject); - req.end(); - }); - - const chunks: Buffer[] = []; - for await (const chunk of await response) chunks.push(chunk as Buffer); - return Buffer.concat(chunks).toString(); -} +// Turbopack (the default bundler in Next.js 16) respects webpack loaders configured +// via next.config turbopack.rules, which we inject in src/hooks/next.ts. +integrationTest.if(nodeSupported)("mapping a Next.js 16 appmap", testNextApp, 60000); diff --git a/test/nextApp.ts b/test/nextApp.ts new file mode 100644 index 0000000..cea6ce3 --- /dev/null +++ b/test/nextApp.ts @@ -0,0 +1,115 @@ +import { ChildProcessWithoutNullStreams } from "node:child_process"; +import { IncomingMessage, request } from "node:http"; + +import { detachOutput, getFreePort, readAppmaps, resolveTarget, spawnAppmapNode } from "./helpers"; + +/** + * The body shared by the Next.js integration tests: run `next dev` on the app + * in the test target directory, request a couple of pages and snapshot the + * resulting appmaps. + */ +export default async function testNextApp() { + const port = await getFreePort(); + const { app, waitForOutput, errorOutput } = await spawnNextJsApp(port); + const response = await makeRequest(port, "/hello"); + console.log("Response", response); + const { pid } = JSON.parse(response) as { pid: number }; + + await makeRequest(port, "/about"); + + // An appmap is written after its response has been sent, so a request being + // finished doesn't mean its appmap is on disk yet. Wait for next to report + // both of them before killing it, or we can lose the last one. + await waitForOutput("-hello.appmap.json"); + await waitForOutput("-about.appmap.json"); + + // We need to kill the next process explicitly on Windows + // because it's spawn-ed with "shell: true" and app is the shell process. + if (process.platform == "win32") process.kill(pid, "SIGINT"); + app.kill("SIGINT"); + await new Promise((r) => app.once("exit", r)); + detachOutput(app); + + // next reports an exception thrown out of the config hooks with this frame in + // the stack and then carries on regardless, so nothing else would notice. + expect(errorOutput()).not.toContain("injectConfig"); + + const appMaps = readAppmaps(); + // Delete response body captures because they will be different in every run + Object.values(appMaps).forEach( + (a) => + a.events?.forEach((e) => { + if ("http_server_response" in e) delete e.http_server_response.return_value; + if ("http_client_response" in e) delete e.http_client_response.return_value; + }), + ); + + expect(appMaps).toMatchSnapshot(); +} + +async function spawnNextJsApp(port: number) { + const nextBin = require.resolve("next/dist/bin/next", { paths: [resolveTarget()] }); + + // On Windows, we give "node" argument explicitly because next is a js file with + // shebang (#!/usr/bin/env node) which does not work on Windows. + const app = + process.platform == "win32" + ? spawnAppmapNode("node", nextBin, "dev", "-p", String(port)) + : spawnAppmapNode(nextBin, "dev", "-p", String(port)); + + const { waitForOutput, errorOutput } = watchOutput(app); + await waitForOutput("Ready"); + return { app, waitForOutput, errorOutput }; +} + +/** + * Watch the process' output, returning a function which waits for a fragment of + * its stdout to appear, and one giving everything it has said on stderr. + * + * Waiting matches against everything seen so far rather than individual chunks: + * next dev interleaves spinner frames and escape sequences with its output, so + * a message can both be split across chunk boundaries and have arrived before + * we start waiting for it. + */ +function watchOutput(app: ChildProcessWithoutNullStreams, timeout = 30000) { + let output = ""; + let errors = ""; + const waiters = new Set<() => void>(); + + app.stdout.on("data", (chunk: Buffer) => { + output += chunk.toString(); + for (const check of waiters) check(); + }); + app.stderr.on("data", (chunk: Buffer) => (errors += chunk.toString())); + + const waitForOutput = (needle: string) => + new Promise((resolve, reject) => { + // Fail before the test times out, so that we get to say what we waited for. + const timer = setTimeout(() => { + waiters.delete(check); + reject(new Error(`Timed out waiting for ${needle} in the output:\n${output}`)); + }, timeout); + const check = () => { + if (!output.includes(needle)) return; + clearTimeout(timer); + waiters.delete(check); + resolve(); + }; + waiters.add(check); + check(); + }); + + return { waitForOutput, errorOutput: () => errors }; +} + +async function makeRequest(port: number, path: string, method = "GET") { + const url = new URL(path, `http://localhost:${port}`); + const response = new Promise((resolve, reject) => { + const req = request(url, { method }, resolve).once("error", reject); + req.end(); + }); + + const chunks: Buffer[] = []; + for await (const chunk of await response) chunks.push(chunk as Buffer); + return Buffer.concat(chunks).toString(); +} diff --git a/test/simple.test.ts b/test/simple.test.ts index 33ad478..47ee0c9 100644 --- a/test/simple.test.ts +++ b/test/simple.test.ts @@ -5,6 +5,7 @@ import { join } from "node:path"; import tmp from "tmp"; import { + detachOutput, getAppMapBinPath, integrationTest, readAppmap, @@ -46,6 +47,7 @@ integrationTestSkipOnWindows("forwarding signals to the child", async () => { daemon.kill("SIGINT"); await new Promise((r) => daemon.once("exit", r)); + detachOutput(daemon); expect(daemon.exitCode).toBe(42); expect(readAppmap()).toMatchSnapshot(); @@ -103,6 +105,7 @@ integrationTestSkipOnWindows("finish signal is handled", async () => { server.kill("SIGINT"); await new Promise((r) => server.once("exit", r)); + detachOutput(server); expect(readAppmap()).toMatchSnapshot(); }); diff --git a/tsconfig.json b/tsconfig.json index 03966f0..fb448b1 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,6 +2,9 @@ "extends": "@tsconfig/node18", "compilerOptions": { "declaration": true, + // Only needed to type check the .tsx pages in the Next.js test apps; the + // apps themselves are built by next with their own tsconfig. + "jsx": "preserve", "outDir": "dist", "noImplicitAny": true, "sourceMap": true,