refactor(handler): address PR #784 review feedback

- Rename `userIsEsm()` to `shouldUseEsmHandler()`. The function returns a
  routing decision based on heuristics (env regex + package.json sniff),
  not an authoritative fact about the user's code, so the new name reads
  more honestly at the call site.

- Inline the former `src/handler.cjs` body into the CJS branch of the
  shim and delete `src/handler.cjs`. Lambda's resolver doesn't auto-resolve
  `.cjs`, so handler.cjs was only ever reachable via this shim's
  `require("./handler.cjs")`. Keeping it as a separate file made the load
  graph confusing — future readers grepping for "what loads handler.cjs?"
  would find one caller in the same directory. The heavy `require()`s
  remain scoped to the CJS branch so ESM users don't pay the cost of
  pulling in the full tracer at require-time.

- Add `src/handler.spec.ts` covering shouldUseEsmHandler's four-case
  matrix (.mjs in _HANDLER / DD_LAMBDA_HANDLER, package.json type=module,
  package.json without type field, package.json missing, package.json
  unparseable). Detection is the load-bearing piece — a regression there
  means either ESM users keep seeing ERR_REQUIRE_ESM or CJS users pay an
  unnecessary async-import cold-start cost.

- Tighten `scripts/update_dist_version.sh`'s `cp src/handler.*` to list
  the runtime artifacts explicitly. The broader glob also picked up
  `src/handler.spec.ts`, which is a test fixture, not a runtime file —
  shipping it would also be silently inconsistent with the Dockerfile's
  hard-coded `rm /nodejs/.../handler.js`.

- Expand the Dockerfile comment to point at
  `scripts/update_dist_version.sh` as the source of the file being
  removed, so a future change to how `src/handler.js` is copied into
  `dist/` is visibly tied to the layer-side `rm`.

Author: zarirhamza

Dockerfile

@@ -19,6 +19,10 @@ RUN cp -r dist /nodejs/node_modules/datadog-lambda-js
 # resolves `handler.handler` to handler.mjs as long as handler.js is absent,
 # which handles both CJS and ESM user code via the async load(). Drop the
 # shim here so the layer keeps that direct-to-mjs behavior.
+#
+# The file being removed is shipped into dist/ by the `cp src/handler.* dist/`
+# step in scripts/update_dist_version.sh — if that script changes how
+# src/handler.js gets copied, this rm must be revisited.
 RUN rm /nodejs/node_modules/datadog-lambda-js/handler.js
 RUN cp ./src/runtime/module_importer.js /nodejs/node_modules/datadog-lambda-js/runtime
 RUN node <<'EOF'

scripts/update_dist_version.sh

@@ -16,6 +16,10 @@ echo "$TRACE_CONSTANTS" |
   sed "s/\(ddtraceVersion =\) \"\(X\.X\.X\)\"/\1 \"$DD_TRACE_VERSION\"/" > ./dist/trace/constants.js
 
 echo "Copying handler js files"
-cp src/handler.* dist/
+# Be explicit about which handler files ship — a broader `src/handler.*` glob
+# would also pick up `src/handler.spec.ts`, which is a test fixture, not a
+# runtime artifact. The Dockerfile's layer build also assumes `dist/handler.js`
+# is the shim from `src/handler.js`, not some other file.
+cp src/handler.js src/handler.mjs dist/
 cp src/init.js dist/init.js
 cp src/runtime/module_importer.js dist/runtime/

src/handler.cjs

@@ -1,24 +1,26 @@
-// CJS entry shim used by AWS Lambda's bootstrap when the handler string
-// resolves to `node_modules/datadog-lambda-js/dist/handler.handler`.
+// CJS entry used by AWS Lambda's bootstrap when the handler string resolves
+// to `node_modules/datadog-lambda-js/dist/handler.handler` from the published
+// npm package. Lambda's resolver picks `.js` before `.mjs`, so this file is
+// what gets loaded for both CJS and ESM user functions. We branch at runtime:
 //
-// Lambda's resolver searches for `.js` before `.mjs`, so this file is what
-// gets loaded for both CJS and ESM user functions. We branch at runtime:
-//
-//   - CJS user code: synchronously delegate to handler.cjs (preserves the
-//     prior fast-path behavior — no extra dynamic import on cold start).
+//   - CJS user code: synchronously load and wrap the handler in-place (the
+//     prior fast-path behavior, formerly delegated to handler.cjs).
 //
 //   - ESM user code: expose an async handler that lazily `import()`s
 //     handler.mjs on the first invocation. handler.mjs's async `load()`
-//     transparently handles both CJS and ESM user modules, so this also
+//     transparently handles both CJS and ESM user modules, so this is what
 //     fixes the ERR_REQUIRE_ESM failure mode reported in
 //     https://github.com/DataDog/datadog-lambda-js/issues/782.
+//
+// In the published layer this file is removed by the Dockerfile so Lambda's
+// resolver falls through to handler.mjs directly.
 
 "use strict";
 
 const fs = require("fs");
 const path = require("path");
 
-function userIsEsm() {
+function shouldUseEsmHandler() {
   const handlerEnv = process.env.DD_LAMBDA_HANDLER || process.env._HANDLER || "";
   if (/\.mjs(\..*)?$/.test(handlerEnv)) {
     return true;
@@ -27,8 +29,7 @@ function userIsEsm() {
   const taskRoot = process.env.LAMBDA_TASK_ROOT || process.cwd();
   try {
     const pkgPath = path.join(taskRoot, "package.json");
-    const raw = fs.readFileSync(pkgPath, "utf8");
-    const pkg = JSON.parse(raw);
+    const pkg = JSON.parse(fs.readFileSync(pkgPath, "utf8"));
     if (pkg && pkg.type === "module") {
       return true;
     }
@@ -39,7 +40,10 @@ function userIsEsm() {
   return false;
 }
 
-if (userIsEsm()) {
+// Exported for unit tests. Not part of the public surface.
+exports._shouldUseEsmHandler = shouldUseEsmHandler;
+
+if (shouldUseEsmHandler()) {
   let cachedHandler;
   let cachedError;
 
@@ -59,5 +63,53 @@ if (userIsEsm()) {
     return cachedHandler(event, context);
   };
 } else {
-  module.exports = require("./handler.cjs");
+  // Inlined former handler.cjs body. Lambda's resolver doesn't auto-resolve
+  // `.cjs` for the handler string, so handler.cjs was only ever reachable
+  // via the shim's `require("./handler.cjs")` — keeping it as a separate
+  // file made the load graph confusing. Requires are scoped to this branch
+  // so ESM users don't pay the cost of pulling in the full tracer at
+  // require-time when they only need handler.mjs.
+  const {
+    datadog,
+    datadogHandlerEnvVar,
+    lambdaTaskRootEnvVar,
+    traceExtractorEnvVar,
+    emitTelemetryOnErrorOutsideHandler,
+    getEnvValue,
+  } = require("./index.js");
+  const { logDebug, logError } = require("./utils/index.js");
+  const { loadSync } = require("./runtime/index.js");
+  const { initTracer } = require("./runtime/module_importer");
+
+  if (process.env.DD_TRACE_DISABLED_PLUGINS === undefined) {
+    process.env.DD_TRACE_DISABLED_PLUGINS = "fs";
+    logDebug("disabled the dd-trace plugin 'fs'");
+  }
+
+  if (getEnvValue("DD_TRACE_ENABLED", "true").toLowerCase() === "true") {
+    initTracer();
+  }
+
+  const taskRootEnv = getEnvValue(lambdaTaskRootEnvVar, "");
+  const handlerEnv = getEnvValue(datadogHandlerEnvVar, "");
+  const extractorEnv = getEnvValue(traceExtractorEnvVar, "");
+  let traceExtractor;
+
+  if (extractorEnv) {
+    try {
+      traceExtractor = loadSync(taskRootEnv, extractorEnv);
+      logDebug("loaded custom trace context extractor", { extractorEnv });
+    } catch (error) {
+      logError("an error occurred while loading the custom trace context extractor", { error, extractorEnv });
+    }
+  }
+
+  try {
+    exports.handler = datadog(loadSync(taskRootEnv, handlerEnv), { traceExtractor });
+  } catch (error) {
+    emitTelemetryOnErrorOutsideHandler(error, handlerEnv, Date.now()).catch(
+      logDebug("failed to error telemetry on error outside handler"),
+    );
+    throw error;
+  }
 }

src/handler.js

@@ -0,0 +1,92 @@
+import * as fs from "fs";
+import * as os from "os";
+import * as path from "path";
+
+// Load handler.js with environment that forces the ESM branch on first
+// require. The ESM branch only defines an async handler — it does not call
+// loadSync or initTracer — so the module loads safely in a test process
+// without a real user app.
+//
+// shouldUseEsmHandler reads process.env / fs at call time, not module-load
+// time, so each test case can mutate the environment freely after the
+// initial require.
+const ORIGINAL_ENV = { ...process.env };
+process.env.DD_LAMBDA_HANDLER = "_handler_spec_init.mjs.handler";
+delete process.env._HANDLER;
+process.env.LAMBDA_TASK_ROOT = os.tmpdir();
+
+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const handlerModule: { _shouldUseEsmHandler: () => boolean } = require("./handler");
+const { _shouldUseEsmHandler: shouldUseEsmHandler } = handlerModule;
+
+describe("handler.js shouldUseEsmHandler", () => {
+  let tmpDirs: string[];
+
+  beforeEach(() => {
+    tmpDirs = [];
+    process.env = { ...ORIGINAL_ENV };
+  });
+
+  afterEach(() => {
+    for (const dir of tmpDirs) {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  afterAll(() => {
+    process.env = { ...ORIGINAL_ENV };
+  });
+
+  function makeTaskRoot(packageJson?: object): string {
+    const dir = fs.mkdtempSync(path.join(os.tmpdir(), "handler-spec-"));
+    tmpDirs.push(dir);
+    if (packageJson !== undefined) {
+      fs.writeFileSync(path.join(dir, "package.json"), JSON.stringify(packageJson));
+    }
+    return dir;
+  }
+
+  it("returns true when _HANDLER points at a .mjs file", () => {
+    process.env._HANDLER = "app.mjs.handler";
+    delete process.env.DD_LAMBDA_HANDLER;
+    process.env.LAMBDA_TASK_ROOT = makeTaskRoot();
+    expect(shouldUseEsmHandler()).toBe(true);
+  });
+
+  it("returns true when DD_LAMBDA_HANDLER points at a .mjs file", () => {
+    process.env.DD_LAMBDA_HANDLER = "app.mjs.handler";
+    delete process.env._HANDLER;
+    process.env.LAMBDA_TASK_ROOT = makeTaskRoot();
+    expect(shouldUseEsmHandler()).toBe(true);
+  });
+
+  it('returns true when LAMBDA_TASK_ROOT/package.json has type: "module"', () => {
+    process.env.DD_LAMBDA_HANDLER = "app.handler";
+    delete process.env._HANDLER;
+    process.env.LAMBDA_TASK_ROOT = makeTaskRoot({ type: "module" });
+    expect(shouldUseEsmHandler()).toBe(true);
+  });
+
+  it("returns false when package.json exists without type: module", () => {
+    process.env.DD_LAMBDA_HANDLER = "app.handler";
+    delete process.env._HANDLER;
+    process.env.LAMBDA_TASK_ROOT = makeTaskRoot({ name: "app" });
+    expect(shouldUseEsmHandler()).toBe(false);
+  });
+
+  it("returns false when package.json is missing from LAMBDA_TASK_ROOT", () => {
+    process.env.DD_LAMBDA_HANDLER = "app.handler";
+    delete process.env._HANDLER;
+    process.env.LAMBDA_TASK_ROOT = makeTaskRoot();
+    expect(shouldUseEsmHandler()).toBe(false);
+  });
+
+  it("returns false when package.json is unparseable", () => {
+    process.env.DD_LAMBDA_HANDLER = "app.handler";
+    delete process.env._HANDLER;
+    const dir = makeTaskRoot();
+    fs.writeFileSync(path.join(dir, "package.json"), "{ not valid json");
+    process.env.LAMBDA_TASK_ROOT = dir;
+    expect(shouldUseEsmHandler()).toBe(false);
+  });
+});