Commit 32b9736
fix(handler)!: drop dist/handler.js to support ESM Lambda handlers (#784)
* fix: route ESM Lambdas through handler.mjs via CJS shim
AWS Lambda's CJS resolver picks `dist/handler.js` before `dist/handler.mjs`
when the handler string is `node_modules/datadog-lambda-js/dist/handler.handler`,
causing ESM user functions to fail with ERR_REQUIRE_ESM (or the surface
"Cannot find module" variant when _tryRequireSync swallows it).
Today `dist/handler.js` ships as a verbatim copy of `dist/handler.cjs`
(via `cp ./dist/handler.cjs ./dist/handler.js` in the publish scripts),
so it can never load ESM user code.
This replaces the copy with a real `src/handler.js` shim that:
- Detects ESM user code via the task root `package.json` `type` field or
a `.mjs` `DD_LAMBDA_HANDLER` / `_HANDLER`.
- For CJS user code: synchronously delegates to `handler.cjs`, preserving
the prior fast path (no cold-start regression).
- For ESM user code: exposes 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.
`scripts/update_dist_version.sh` already copies `src/handler.*` into
`dist/`, so the shim ships automatically; the explicit
`cp ./dist/handler.cjs ./dist/handler.js` lines in `publish_npm.sh` and
`publish_prod.sh` are no longer needed and are removed.
Refs: #782
Refs: SLES-2888, SLES-2895, SVLS-9238
* fix(layer): keep handler.js shim out of the published layer
The Dockerfile copies `dist/` wholesale into the layer, so the new
`dist/handler.js` shim would get included and Lambda's bootstrap would
resolve `handler.handler` to it. Inside the layer that goes through the
shim's CJS branch -> handler.cjs -> _tryRequireSync, which can't load
`.mjs` user modules (this regressed the `esm_node*` integration tests).
The layer has never shipped a `handler.js`; Lambda's resolver falls
through to `handler.mjs`, whose async `load()` handles both CJS and ESM
user code. Drop the shim from the layer to preserve that behavior. The
shim is still useful in the npm tarball, where the publish scripts used
to copy `handler.cjs` to `handler.js`.
* 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`.
* refactor: drop CJS shim and ship handler.mjs as sole entry point
After feedback from Darcy Rayner (original handler.cjs/mjs author) and
re-evaluating the cold-start trade-offs, the shim is the wrong shape.
The shim's CJS branch saved ~10-30ms of cold-start init for CJS users
on the npm-redirect path, but its ESM branch moved 300-800ms of
tracer/handler load work from Lambda's init phase into the first
invocation. That's worse in three ways for ESM users (the cohort this
PR exists to fix):
1. First-invoke timeout risk on functions with tight timeouts —
the loading work now eats into the invocation's time budget.
2. Provisioned concurrency is wasted — work deferred past init isn't
pre-warmed, so PC customers pay for warm capacity and still hit
cold-start latency on the first real request.
3. Lambda SnapStart is wasted for the same reason — snapshot is
taken after init, and the shim's lazy `import()` runs post-snapshot.
handler.mjs's async `load()` already handles both CJS and ESM user
modules transparently. Lambda's bootstrap resolves `dist/handler.handler`
to handler.mjs once `.js` is absent, so removing the shim is sufficient
and matches the behavior the published layer has always had.
End state:
- Sole Lambda entry point everywhere: `handler.mjs`.
- The npm tarball and the layer ship the same `dist/` (one handler file).
- No detection heuristics, no shim, no `handler.cjs` (already gone).
Aligns with:
- Darcy Rayner's recommendation on the PR thread to "only bundle the
ESM handler" since all supported runtimes (Node 18+) support top-level
await in ESM.
- The customer's own suggested fix in
#782.
- SLES-2888's confirmed-working internal workaround
(`rm dist/handler.js` post-install).
- The intent of the earlier PR #697.
* refactor: drop orphaned sync loader chain; guard dist against stale artifacts
Two follow-ups from PR #784 review:
1. Remove `loadSync`, `_loadUserAppSync`, `_tryRequireSync`. Deleting
`src/handler.cjs` in the previous commit removed the only consumers of
the synchronous loader chain. `loadSync` was never re-exported from the
public `src/index.ts` entry point, no tests reference it, and the only
internal import (`src/handler.mjs`) uses the async `load()`. The three
functions and the `loadSync` re-export in `src/runtime/index.ts` are
now dead code, so drop them.
2. Defensive cleanup in `scripts/update_dist_version.sh`: explicitly
`rm -f dist/handler.js dist/handler.cjs` before copying handler.mjs.
`tsc` does not clean stale files in `dist/` between incremental
builds, so a leftover `handler.js` from a prior build of an earlier
ref could resurface and silently re-introduce the resolver bug this
PR is fixing. The publish scripts already do `rm -rf ./dist` before
`yarn build`, so this is purely a guard for local dev builds.
* test(integration): cover dist/handler.handler under AWS-stock RIC
Add container-image integration tests that exercise the npm-redirect
path through the AWS-published `public.ecr.aws/lambda/nodejs:N` base
image, with both CJS and ESM user handlers. Closes the gap from the
existing `esm_node*` tests, which only cover the layer path
(`/opt/nodejs/node_modules/datadog-lambda-js/handler.handler`) where
only `handler.mjs` has ever been shipped.
These tests would fail under any future regression that re-introduces
a CJS-shadowing artifact at `dist/handler.js`, including the original
#305 scenario for stock AWS RIC.
- integration_tests/container/{cjs,esm}/ — Dockerfiles + handlers
- serverless.yml — provider.ecr.images + container-{cjs,esm}_node funcs
- run_integration_tests.sh — pack local datadog-lambda-js before deploy,
thread NODE_MAJOR for ECR build args, add handlers to snapshot loop
* test(integration): make container tests pass + add baseline snapshots
Three fixes needed to get the container-image integration tests
deploying and invoking against real AWS-stock RIC:
- Container Dockerfiles previously installed only the
datadog-lambda-js tarball, which doesn't pull dd-trace because
it's a devDep of the package. Add dd-trace as a direct dependency
in each container test's package.json, pinned to 5.105.0 to
match the version yarn.lock resolves for the layer build.
- Run `npm install --omit=dev` before installing the tarball so
package.json deps are picked up.
- Export BUILDX_NO_DEFAULT_ATTESTATIONS=1 in run_integration_tests.sh.
Modern Docker buildx attaches provenance attestations by default,
which produce OCI index manifests that AWS Lambda rejects with
"image manifest ... media type ... not supported." Disabling the
default attestations makes buildx emit Docker v2 manifests Lambda
accepts.
Plus baseline snapshots for `container-{cjs,esm}_node{18,20,22,24}`
across 9 input events (72 return-value + 8 log files). Each
container handler returns `{"message":"hello, dog!"}` end-to-end
through `dist/handler.handler`, proving CJS and ESM user code load
correctly on AWS-stock RIC for every supported Node major.
* fix(integration): install devDeps before host-side yarn build
`yarn build` runs `tsc` which needs the root devDeps (@types/node,
@types/aws-lambda, typescript, etc.). In CI the script is invoked
without BUILD_LAYERS=true, so the layer-build Docker path that would
otherwise hide the host install isn't reached, and tsc blew up with
~150 missing-type errors before getting to npm pack.
Add `yarn install --frozen-lockfile` before the build so the host
node_modules is populated regardless of which path the caller took.
* ci(gitlab): route integration tests to docker-in-docker runner
The `arch:amd64` tag only provides the docker CLI; the daemon isn't
running, so the container-image integration tests blow up with
"Cannot connect to the Docker daemon at unix:///var/run/docker.sock"
when serverless tries to build the ECR images.
Switch to `docker-in-docker:amd64`, matching the pattern used by
serverless-self-monitoring's `.deploy-lod-base` job for Docker-
dependent steps. Other jobs (lint, unit test, layer build, etc.)
don't need a daemon and stay on the cheaper plain runner.
* test(integration): refresh container-cjs_node24 log snapshot
The container-cjs_node24 snapshot was originally captured during a
container init that timed out and recovered. That left two artifacts
that no other container snapshot shares:
- a leading `INIT_REPORT ... Status: timeout` line
- the cold-start `END Duration` line missing the `(init: XXXX ms)`
segment that AWS Lambda normally bundles when init completes
inside the budget
After routing integration tests to dind runners (faster image cache)
and shipping handler.mjs as the sole entrypoint (no shim probing), the
container init now reliably finishes inside the init budget. The new
log shape matches every sibling container snapshot
(container-cjs_node18/20/22 and container-esm_node18/20/22/24).
Refresh the snapshot to that shape. After this change,
container-cjs_node24.log is byte-aligned with container-cjs_node22.log
on the `END Duration` line positions (107, 213, 319, ...).
---------
Co-authored-by: Jordan Gonzalez <30836115+duncanista@users.noreply.github.com>1 parent 5b3377b commit 32b9736
96 files changed
Lines changed: 7924 additions & 159 deletions
File tree
- .gitlab
- input_files
- scripts
- integration_tests
- container
- cjs
- esm
- snapshots
- logs
- return_values
- scripts
- src
- runtime
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
81 | 81 | | |
82 | 82 | | |
83 | 83 | | |
84 | | - | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
85 | 89 | | |
86 | 90 | | |
87 | 91 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
33 | 32 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
18 | 33 | | |
19 | 34 | | |
20 | 35 | | |
| |||
81 | 96 | | |
82 | 97 | | |
83 | 98 | | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
84 | 120 | | |
85 | 121 | | |
86 | 122 | | |
| |||
0 commit comments