Skip to content

Commit

Permalink
fix: Make sure AppMap root stays constant after launch
Browse files Browse the repository at this point in the history
When the current directory was changed after first launching
appmap-node appmaps would have been written to a subdirectory
of that new location. This was unexpected and has been corrected.
  • Loading branch information
dividedmind committed Jan 6, 2024
1 parent 53021ad commit 1429711
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 26 deletions.
4 changes: 2 additions & 2 deletions src/Recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { inspect } from "node:util";
import type AppMap from "./AppMap";
import AppMapStream from "./AppMapStream";
import { makeClassMap } from "./classMap";
import { appMapDir } from "./config";
import config from "./config";
import { makeCallEvent, makeExceptionEvent, makeReturnEvent } from "./event";
import { defaultMetadata } from "./metadata";
import type { FunctionInfo } from "./registry";
Expand All @@ -16,7 +16,7 @@ export default class Recording {
constructor(type: AppMap.RecorderType, recorder: string, ...names: string[]) {
const dirs = [recorder, ...names].map(quotePathSegment);
const name = dirs.pop()!; // it must have at least one element
this.path = join(appMapDir, ...dirs, makeAppMapFilename(name));
this.path = join(config.appmapDir, ...dirs, makeAppMapFilename(name));
this.partPath = this.path + ".part";
this.stream = new AppMapStream(this.partPath);
this.metadata = {
Expand Down
4 changes: 3 additions & 1 deletion src/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ import { kill, pid } from "node:process";

import { info } from "./message";
import { version } from "./metadata";
import { readPkgUp } from "./util/readPkgUp";
import forwardSignals from "./util/forwardSignals";
import { readPkgUp } from "./util/readPkgUp";
import config from "./config";

const registerPath = resolve(__dirname, "../dist/register.js");
const loaderPath = resolve(__dirname, "../dist/loader.js");
Expand All @@ -19,6 +20,7 @@ export function main() {

info("Running with appmap-node version %s", version);
addNodeOptions("--require", registerPath);
config.export();

// FIXME: Probably there should be a way to remove this altogether
// by changing our custom loader implementation
Expand Down
19 changes: 15 additions & 4 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,19 @@ import { cwd } from "node:process";

import { readPkgUp } from "./util/readPkgUp";

export const root = cwd();
export const appMapDir = join(root, "tmp", "appmap");
export class Config {
public readonly appmapDir: string;
public readonly appName: string;

export const targetPackage = readPkgUp(root);
export const appName = targetPackage?.name ?? dirname(root);
constructor(public readonly root = process.env.APPMAP_ROOT ?? cwd()) {
this.appmapDir = join(root, "tmp", "appmap");
const targetPackage = readPkgUp(root);
this.appName = targetPackage?.name ?? dirname(root);
}

export() {
process.env.APPMAP_ROOT = this.root;
}
}

export default new Config();
7 changes: 5 additions & 2 deletions src/hooks/__tests__/instrument.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { full as walk } from "acorn-walk";
import { ESTree, parse } from "meriyah";

import * as instrument from "../instrument";
import config from "../../config";
import * as registry from "../../registry";
import * as instrument from "../instrument";

describe(instrument.shouldInstrument, () => {
instrument.setRoot("/test");
jest.replaceProperty(config, "root", "/test");
test.each([
["node:test", false],
["file:///test/test.json", false],
Expand Down Expand Up @@ -125,3 +126,5 @@ function stripLocations(program: ESTree.Program): ESTree.Program {
});
return program;
}

jest.mock("../../config");
12 changes: 3 additions & 9 deletions src/hooks/instrument.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import assert from "node:assert";
import path from "node:path";
import { cwd } from "node:process";
import { fileURLToPath } from "node:url";

import { ancestor as walk } from "acorn-walk";
import { ESTree, parse } from "meriyah";
import { SourceMapConsumer } from "source-map-js";

import config from "../config";
import {
args as args_,
call_,
Expand All @@ -19,7 +19,7 @@ import {
toArrowFunction,
yieldStar,
} from "../generate";
import { FunctionInfo, SourceLocation, createMethodInfo, createFunctionInfo } from "../registry";
import { FunctionInfo, SourceLocation, createFunctionInfo, createMethodInfo } from "../registry";
import findLast from "../util/findLast";

const __appmapFunctionRegistryIdentifier = identifier("__appmapFunctionRegistry");
Expand Down Expand Up @@ -147,19 +147,13 @@ function wrapWithRecord(
return wrapped;
}

let root = cwd();

export function setRoot(path: string) {
root = path;
}

export function shouldInstrument(url: URL): boolean {
if (url.protocol !== "file:") return false;
if (url.pathname.endsWith(".json")) return false;

const filePath = fileURLToPath(url);
if (filePath.includes("node_modules") || filePath.includes(".yarn")) return false;
if (isUnrelated(root, filePath)) return false;
if (isUnrelated(config.root, filePath)) return false;

return true;
}
Expand Down
16 changes: 10 additions & 6 deletions src/loader.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import path from "node:path";
import { URL } from "node:url";
import path, { dirname } from "node:path";
import { URL, fileURLToPath } from "node:url";

import type { NodeLoaderHooksAPI2 } from "ts-node";

import { targetPackage } from "./config";
import { warn } from "./message";
import transform, { findHook } from "./transform";
import { readPkgUp } from "./util/readPkgUp";

export const load: NodeLoaderHooksAPI2["load"] = async function load(url, context, defaultLoad) {
const urlObj = new URL(url);
Expand All @@ -15,9 +15,13 @@ export const load: NodeLoaderHooksAPI2["load"] = async function load(url, contex
// CommonJS modules. This leads to TypeError [ERR_UNKNOWN_FILE_EXTENSION].
// To fix this, we give the context.format hint to defaultLoad if we decide
// that the file is a CommonJS module.
const ext = path.extname(urlObj.pathname).slice(1);
if (ext == "" && urlObj.protocol == "file:" && targetPackage?.type != "module") {
context.format = "commonjs";
if (urlObj.protocol === "file:") {
const targetPath = fileURLToPath(urlObj);
if (
path.extname(targetPath).slice(1) === "" &&
readPkgUp(dirname(targetPath))?.type !== "module"
)
context.format = "commonjs";
}

const hook = findHook(urlObj);
Expand Down
4 changes: 2 additions & 2 deletions src/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import process from "node:process";
import type { PackageJson } from "type-fest";

import type AppMap from "./AppMap";
import { appName } from "./config";
import config from "./config";
import { examineException } from "./event";
import pick from "./util/pick";

Expand All @@ -24,7 +24,7 @@ export const defaultMetadata: Partial<AppMap.Metadata> & { client: AppMap.Client
engine: "Node.js",
version: process.version,
},
app: appName,
app: config.appName,
};

export function exceptionMetadata(error: unknown): AppMap.ExceptionMetadata | undefined {
Expand Down
7 changes: 7 additions & 0 deletions test/simple.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,10 @@ integrationTest("mapping an extensionless CommonJS file", () => {
expect(runAppmapNode("./extensionless").status).toBe(0);
expect(readAppmap()).toMatchSnapshot();
});

integrationTest("running a script after changing the current directory", () => {
// Need to make sure the appmap "root" stays the same after
// appmap-node is run, even if the current directory changes.
expect(runAppmapNode("bash", "-c", "cd subproject; node index.js").status).toBe(0);
expect(readAppmap()).toBeDefined();
});
1 change: 1 addition & 0 deletions test/simple/subproject/index.js

0 comments on commit 1429711

Please sign in to comment.