From a46b799ad597d70e97da59af0ee09911510be642 Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sat, 22 Aug 2026 18:33:28 +0300 Subject: [PATCH 1/5] fix: keep the CommonJS interop shim out of the ESM build (#346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dual CJS/ESM build (#326, shipped in 1.5.0) compiles a single `src/` tree twice. Seven source files ended with an interop shim: if (typeof module !== 'undefined') { module.exports = createAPI; module.exports.default = createAPI; } which restores the historical CommonJS shape — but it was emitted into `dist/esm` as well. Bundlers that inline the ESM artifact into a generated CommonJS wrapper (esbuild `--format=cjs`, AWS CDK `NodejsFunction`, SST, Serverless Framework) leave exactly one `module` in scope: the consumer's. The shim then replaced the consumer's `module.exports`, deleting their `handler`. On Lambda this deployed cleanly and failed on every invocation with `Runtime.HandlerNotFound: index.handler is undefined or not exported`, with nothing in the error naming lambda-api. `src/` is now pure ESM. The interop footers are appended to `dist/cjs` only, by `scripts/cjs-interop.js`, which also loads each patched file back and fails the build if the CommonJS shape is wrong. Footers are per-file on purpose: the `.default` self-reference is correct on the package root but would make `mimemap['default']` resolve to the whole MIME map. `src/lib/s3-service.js` now exports its `service` object so the footer can reach it; that object must stay the module value so `response.js` and the unit suites (`sinon.stub`) share one set of mutable properties. Nothing about `dist/cjs` changes: `require('lambda-api')` is still callable, `.default` still self-references, and `require('lambda-api/lib/*')` still returns the value itself. Regression coverage: - `module-compat` fails if any `dist/esm` file references `module.exports`, `typeof module`, or `exports.` — it named all 7 files before the fix. - `module-compat` pins the `dist/cjs` interop shape the footers must preserve. - New e2e Layer 1 fixture bundles an ESM handler with esbuild `--format=cjs` against the packed tarball and asserts the consumer's `handler` export survives and returns 200. It reported `["default"] / handler undefined` before the fix. Closes #346 --- AGENTS.md | 15 +++ __tests__/esm-compat.mjs | 12 +++ __tests__/module-compat.unit.js | 71 ++++++++++++ e2e/fixtures/esm-to-cjs-bundle/handler.mjs | 9 ++ e2e/fixtures/esm-to-cjs-bundle/invoke.cjs | 28 +++++ e2e/fixtures/esm-to-cjs-bundle/package.json | 1 + e2e/run-layer1.mjs | 28 +++++ package.json | 2 +- scripts/cjs-interop.js | 113 ++++++++++++++++++++ src/index.js | 8 -- src/lib/mimemap.js | 4 - src/lib/prettyPrint.js | 4 - src/lib/request.js | 4 - src/lib/response.js | 4 - src/lib/s3-service.js | 12 +-- src/lib/statusCodes.js | 4 - 16 files changed, 284 insertions(+), 35 deletions(-) create mode 100644 e2e/fixtures/esm-to-cjs-bundle/handler.mjs create mode 100644 e2e/fixtures/esm-to-cjs-bundle/invoke.cjs create mode 100644 e2e/fixtures/esm-to-cjs-bundle/package.json create mode 100644 scripts/cjs-interop.js diff --git a/AGENTS.md b/AGENTS.md index 0956a3e..45ed324 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,6 +59,20 @@ api.use((err, req, res, next) => { }); ``` +## Build + +`src/` is **pure ESM** and is compiled twice by SWC — to `dist/cjs` and to `dist/esm`. The +CommonJS interop that makes `require('lambda-api')` callable is appended to the CJS artifact +_only_, by `scripts/cjs-interop.js`. + +Never put a `module.exports` / `typeof module` write in `src/`. It would also land in `dist/esm`, +and bundlers that inline the ESM artifact into a generated CommonJS wrapper (esbuild +`--format=cjs`, AWS CDK `NodejsFunction`, SST, Serverless) execute it against the _consumer's_ +`module`, wiping out their exports — on Lambda that reads as `Runtime.HandlerNotFound` +(issue #346). `__tests__/module-compat.unit.js` fails the build if one reappears. + +To change the CommonJS shape of a module, edit the footer table in `scripts/cjs-interop.js`. + ## Testing - Tests live in `__tests__/*.unit.js` @@ -72,6 +86,7 @@ api.use((err, req, res, next) => { - Add external npm dependencies (zero-dependency policy is non-negotiable) - Introduce breaking changes to the public API +- Write to `module.exports` or `exports` from `src/` — see Build below **Always do:** diff --git a/__tests__/esm-compat.mjs b/__tests__/esm-compat.mjs index 8cd8bc0..b8220bc 100644 --- a/__tests__/esm-compat.mjs +++ b/__tests__/esm-compat.mjs @@ -4,6 +4,7 @@ import createAPI from '../dist/esm/index.js'; import * as utils from '../dist/esm/lib/utils.js'; import prettyPrint from '../dist/esm/lib/prettyPrint.js'; import { ApiError } from '../dist/esm/lib/errors.js'; +import * as s3 from '../dist/esm/lib/s3-service.js'; const event = { httpMethod: 'GET', @@ -43,6 +44,17 @@ async function main() { if (JSON.parse(result.body).ok !== true) { throw new Error('Expected successful ESM route response'); } + + for (const name of ['setConfig', 'getObject', 'getSignedUrl']) { + if (typeof s3[name] !== 'function') { + throw new Error(`Expected s3-service to export ${name} as a function`); + } + } + + // The CJS artifact collapses to this object; under ESM it stays a plain named export. + if (typeof s3.service !== 'object' || typeof s3.service.getObject !== 'function') { + throw new Error('Expected s3-service to export the service object'); + } } main().catch((error) => { diff --git a/__tests__/module-compat.unit.js b/__tests__/module-compat.unit.js index a894e04..1ca4cd1 100644 --- a/__tests__/module-compat.unit.js +++ b/__tests__/module-compat.unit.js @@ -1,8 +1,11 @@ 'use strict'; const { execFileSync } = require('child_process'); +const fs = require('fs'); const path = require('path'); +const DIST = path.join(__dirname, '..', 'dist'); + const event = { httpMethod: 'GET', path: '/compat', @@ -18,6 +21,14 @@ const runRoute = async (api) => { return api.run(event, {}); }; +// Recursively collect every compiled .js file under a dist directory. +const jsFilesIn = (dir) => + fs.readdirSync(dir, { withFileTypes: true }).reduce((acc, entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return acc.concat(jsFilesIn(full)); + return entry.name.endsWith('.js') ? acc.concat(full) : acc; + }, []); + describe('Module Compatibility Tests:', function () { describe('CommonJS build output', function () { it('loads the package factory from dist/cjs', function () { @@ -52,6 +63,44 @@ describe('Module Compatibility Tests:', function () { }); }); + // The CommonJS interop shape below is what `scripts/cjs-interop.js` appends to dist/cjs. It is + // pinned here because the source is pure ESM: nothing in src/ produces this shape any more. + describe('CommonJS interop shape (dist/cjs only)', function () { + it('exports the default value itself for single-default modules', function () { + expect(typeof require('../dist/cjs/lib/request.js')).toBe('function'); + expect(typeof require('../dist/cjs/lib/response.js')).toBe('function'); + expect(typeof require('../dist/cjs/lib/prettyPrint.js')).toBe('function'); + expect(typeof require('../dist/cjs/lib/statusCodes.js')[404]).toBe( + 'string' + ); + expect(typeof require('../dist/cjs/lib/mimemap.js').json).toBe('string'); + }); + + it('does not graft a .default self-reference onto the lib modules', function () { + // `.default` is only correct on the package root. On mimemap in particular it would make + // a `default` file extension resolve to the whole map. + expect(require('../dist/cjs/lib/mimemap.js').default).toBeUndefined(); + expect(require('../dist/cjs/lib/statusCodes.js').default).toBeUndefined(); + }); + + it('exposes s3-service as a single mutable object that sinon can stub', function () { + // response.js reads S3 methods through this same object, and four unit suites do + // `sinon.stub(require('../lib/s3-service'), 'getSignedUrl')`. SWC's own `_export()` emits + // non-configurable getters, which would make stubbing throw. + const s3 = require('../dist/cjs/lib/s3-service.js'); + + expect(s3.__esModule).toBe(true); + expect('client' in s3).toBe(true); + + ['getObject', 'getSignedUrl', 'setConfig'].forEach((method) => { + const descriptor = Object.getOwnPropertyDescriptor(s3, method); + expect(typeof s3[method]).toBe('function'); + expect(descriptor.writable).toBe(true); + expect(descriptor.configurable).toBe(true); + }); + }); + }); + describe('ESM build output', function () { it('passes Node ESM compatibility checks', function () { execFileSync(process.execPath, ['__tests__/esm-compat.mjs'], { @@ -59,6 +108,28 @@ describe('Module Compatibility Tests:', function () { stdio: 'pipe', }); }); + + // Regression guard for issue #346. Bundlers that inline the ESM artifact into a generated + // CommonJS wrapper (esbuild --format=cjs, CDK NodejsFunction, SST, Serverless) leave exactly + // one `module` in scope: the CONSUMER's. Any write to it from here silently replaces the + // consumer's exports, which on Lambda surfaces as `Runtime.HandlerNotFound`. + it('never touches the CommonJS module system (issue #346)', function () { + const esm = path.join(DIST, 'esm'); + const patterns = [ + /\bmodule\s*\.\s*exports\b/, + /\btypeof\s+module\b/, + /(?:^|[^.\w$])exports\s*(?:\.|\[)/, + ]; + + const offenders = jsFilesIn(esm) + .filter((file) => { + const source = fs.readFileSync(file, 'utf8'); + return patterns.some((pattern) => pattern.test(source)); + }) + .map((file) => path.relative(esm, file)); + + expect(offenders).toEqual([]); + }); }); describe('Package exports resolution', function () { diff --git a/e2e/fixtures/esm-to-cjs-bundle/handler.mjs b/e2e/fixtures/esm-to-cjs-bundle/handler.mjs new file mode 100644 index 0000000..e2a6d12 --- /dev/null +++ b/e2e/fixtures/esm-to-cjs-bundle/handler.mjs @@ -0,0 +1,9 @@ +// Issue #346: an ESM handler bundled to a single CommonJS file — the shape produced by +// AWS CDK `NodejsFunction`, SST and Serverless Framework. esbuild resolves lambda-api through +// the `import` condition and inlines dist/esm into a generated CJS wrapper. +import createAPI from 'lambda-api'; + +const api = createAPI({ version: 'v1' }); +api.get('/', (req, res) => res.json({ hello: 'world', lang: 'esm-to-cjs' })); + +export const handler = async (event, context) => api.run(event, context); diff --git a/e2e/fixtures/esm-to-cjs-bundle/invoke.cjs b/e2e/fixtures/esm-to-cjs-bundle/invoke.cjs new file mode 100644 index 0000000..285c594 --- /dev/null +++ b/e2e/fixtures/esm-to-cjs-bundle/invoke.cjs @@ -0,0 +1,28 @@ +'use strict'; +// Loads the esbuild CJS bundle exactly the way the AWS Lambda Node runtime does — a plain +// `require()` followed by a lookup of the named export — and reports what actually survived. +const { readFileSync } = require('fs'); + +const bundle = require('./bundle.cjs'); +const event = JSON.parse(readFileSync(process.argv[2], 'utf8')); + +const out = { + keys: Object.keys(bundle), + handlerType: typeof bundle.handler, + response: null, +}; + +Promise.resolve() + .then(() => + out.handlerType === 'function' + ? bundle.handler(event, { getRemainingTimeInMillis: () => 3000 }) + : null + ) + .then((response) => { + out.response = response; + process.stdout.write(JSON.stringify(out)); + }) + .catch((e) => { + process.stderr.write(String((e && e.stack) || e)); + process.exit(1); + }); diff --git a/e2e/fixtures/esm-to-cjs-bundle/package.json b/e2e/fixtures/esm-to-cjs-bundle/package.json new file mode 100644 index 0000000..df2bf4d --- /dev/null +++ b/e2e/fixtures/esm-to-cjs-bundle/package.json @@ -0,0 +1 @@ +{ "name": "fixture-esm-to-cjs-bundle", "private": true, "type": "module" } diff --git a/e2e/run-layer1.mjs b/e2e/run-layer1.mjs index 6607254..30652f3 100644 --- a/e2e/run-layer1.mjs +++ b/e2e/run-layer1.mjs @@ -143,6 +143,34 @@ section('esbuild bundle (issue #295: no "Dynamic require")'); }); } +// 4b. issue #346 — an ESM entry bundled to CJS must not lose the consumer's own exports. +// esbuild resolves lambda-api through the `import` condition here and inlines dist/esm into +// a generated CommonJS wrapper, so any `module.exports = ...` in the ESM artifact lands on +// the BUNDLE's exports. On Lambda that shows up as `Runtime.HandlerNotFound`. +section('esbuild ESM entry -> cjs output (issue #346: consumer exports survive)'); +{ + const dir = stageFixture(base, 'esm-to-cjs-bundle'); + check('bundles, keeps `handler` export, and returns 200', () => { + try { + execFileSync( + esbuildBin, + ['handler.mjs', '--bundle', '--format=cjs', '--platform=node', '--external:@aws-sdk/*', '--outfile=bundle.cjs'], + { cwd: dir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] } + ); + } catch (e) { + throw new Error(`esbuild failed: ${(e.stderr || '') + (e.stdout || '')}`); + } + const r = parseResponse(runNode(dir, 'invoke.cjs', ev1), 'esm-to-cjs-bundle'); + assert( + r.handlerType === 'function', + `bundle exports ${JSON.stringify(r.keys)} — handler is ${r.handlerType} (issue #346 regression)` + ); + assert(r.keys.includes('handler'), `expected 'handler' in exports, got ${JSON.stringify(r.keys)}`); + assert(r.response && r.response.statusCode === 200, `status ${r.response && r.response.statusCode}`); + assert(JSON.parse(r.response.body).hello === 'world', 'body.hello'); + }); +} + // 5. exports map subpath resolution (root, lib/*, lib/*.js, package.json) section('exports map subpath resolution'); { diff --git a/package.json b/package.json index 772bbc3..254b11a 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ }, "scripts": { "clean": "rm -rf dist", - "build:cjs": "swc src -d dist/cjs --config-file .swcrc.cjs.json --strip-leading-paths", + "build:cjs": "swc src -d dist/cjs --config-file .swcrc.cjs.json --strip-leading-paths && node scripts/cjs-interop.js", "build:esm": "swc src -d dist/esm --config-file .swcrc.esm.json --copy-files --strip-leading-paths", "build:types": "tsc -p tsconfig.types.json && cp dist/types/lib/*.d.ts dist/cjs/lib/ && cp dist/types/lib/*.d.ts dist/esm/lib/ && rm -rf dist/types", "build": "npm run clean && npm run build:cjs && npm run build:esm && npm run build:types", diff --git a/scripts/cjs-interop.js b/scripts/cjs-interop.js new file mode 100644 index 0000000..3b8f61d --- /dev/null +++ b/scripts/cjs-interop.js @@ -0,0 +1,113 @@ +'use strict'; +/** + * Appends the CommonJS interop footers to the compiled CJS artifact. + * @author Jeremy Daly + * @license MIT + * + * `src/` is pure ESM and is compiled twice by SWC — to `dist/cjs` and to `dist/esm`. The + * historical CommonJS shape (`require('lambda-api')` is callable, `require('lambda-api/lib/*')` + * returns the value itself) has to be restored on top of SWC's `exports.default` output, but it + * MUST NOT live in `src/`: anything written there also lands in `dist/esm`, and bundlers that + * inline the ESM artifact into a generated CommonJS wrapper (esbuild `--format=cjs`, AWS CDK + * `NodejsFunction`, SST, Serverless Framework) would then execute that write against the + * CONSUMER's `module`, wiping out their own exports — issue #346, which surfaced on Lambda as + * `Runtime.HandlerNotFound: index.handler is undefined or not exported`. + * + * So the footers are injected here, into `dist/cjs` only. Each one is written out per file rather + * than generated from a single template: `.default` is correct on the package root but would, for + * example, make `mimemap['default']` resolve to the whole MIME map. + */ + +const fs = require('fs'); +const path = require('path'); + +const CJS_DIR = path.join(__dirname, '..', 'dist', 'cjs'); +const MARKER = '/* CommonJS interop — injected by scripts/cjs-interop.js */'; + +// Modules with a single `export default`: `require()` returns that value directly. +const DEFAULT_FOOTER = 'module.exports = exports.default;\n'; + +const FOOTERS = { + 'index.js': + 'var _createAPI = exports.default;\n' + + 'module.exports = _createAPI;\n' + + '// Preserve the `.default` self-reference the CommonJS build shipped before the dual-package\n' + + "// refactor, so `require('lambda-api').default` keeps working for TypeScript consumers\n" + + '// compiled to CommonJS with esModuleInterop:false (they emit `require(...).default(...)`).\n' + + 'module.exports.default = _createAPI;\n', + 'lib/mimemap.js': DEFAULT_FOOTER, + 'lib/prettyPrint.js': DEFAULT_FOOTER, + 'lib/request.js': DEFAULT_FOOTER, + 'lib/response.js': DEFAULT_FOOTER, + 'lib/statusCodes.js': DEFAULT_FOOTER, + // s3-service has no default export. It must resolve to the single mutable `service` object so + // response.js and the unit suites (sinon.stub) operate on the same properties — SWC's own + // `_export()` emits non-configurable getters, which stubbing cannot replace. `__esModule` keeps + // SWC's `_interop_require_wildcard` returning the object untouched. + 'lib/s3-service.js': + "Object.defineProperty(exports.service, '__esModule', { value: true });\n" + + 'module.exports = exports.service;\n', +}; + +// Loaded back after patching so a toolchain change that breaks the contract fails the BUILD, +// not just the test suite (`npm test` and `prepublishOnly` both go through `npm run build`). +const CONTRACT = { + 'index.js': (m) => typeof m === 'function' && m.default === m, + 'lib/mimemap.js': (m) => + typeof m.json === 'string' && m.default === undefined, + 'lib/prettyPrint.js': (m) => typeof m === 'function', + 'lib/request.js': (m) => typeof m === 'function', + 'lib/response.js': (m) => typeof m === 'function', + 'lib/statusCodes.js': (m) => + typeof m[404] === 'string' && m.default === undefined, + 'lib/s3-service.js': (m) => + m.__esModule === true && + 'client' in m && + ['getObject', 'getSignedUrl', 'setConfig'].every(function (name) { + const descriptor = Object.getOwnPropertyDescriptor(m, name); + return ( + typeof m[name] === 'function' && + descriptor.writable && + descriptor.configurable + ); + }), +}; + +const fail = (message) => { + console.error('cjs-interop: ' + message); // eslint-disable-line no-console + process.exit(1); +}; + +Object.keys(FOOTERS).forEach((relative) => { + const file = path.join(CJS_DIR, relative); + + if (!fs.existsSync(file)) { + fail('expected ' + relative + ' in dist/cjs — did build:cjs run?'); + } + + const source = fs.readFileSync(file, 'utf8'); + + if (source.indexOf(MARKER) !== -1) { + fail(relative + ' is already patched — run `npm run clean` first'); + } + + fs.writeFileSync( + file, + source.replace(/\s*$/, '\n') + '\n' + MARKER + '\n' + FOOTERS[relative] + ); +}); + +Object.keys(CONTRACT).forEach((relative) => { + const file = path.join(CJS_DIR, relative); + let loaded; + + try { + loaded = require(file); + } catch (e) { + fail('patched ' + relative + ' failed to load: ' + e.message); + } + + if (!CONTRACT[relative](loaded)) { + fail(relative + ' did not end up with the expected CommonJS shape'); + } +}); diff --git a/src/index.js b/src/index.js index 2d7604a..8beb073 100644 --- a/src/index.js +++ b/src/index.js @@ -564,11 +564,3 @@ class API { const createAPI = (opts) => new API(opts); export default createAPI; - -if (typeof module !== 'undefined') { - module.exports = createAPI; - // Preserve the `.default` self-reference the CommonJS build shipped before the dual-package - // refactor, so `require('lambda-api').default` keeps working for TypeScript consumers compiled - // to CommonJS with esModuleInterop:false (they emit `require('lambda-api').default(...)`). - module.exports.default = createAPI; -} diff --git a/src/lib/mimemap.js b/src/lib/mimemap.js index 7c92222..1fa6f0d 100644 --- a/src/lib/mimemap.js +++ b/src/lib/mimemap.js @@ -73,7 +73,3 @@ const mimemap = { }; export default mimemap; - -if (typeof module !== 'undefined') { - module.exports = mimemap; -} diff --git a/src/lib/prettyPrint.js b/src/lib/prettyPrint.js index e2ad5b7..7ac0586 100644 --- a/src/lib/prettyPrint.js +++ b/src/lib/prettyPrint.js @@ -83,7 +83,3 @@ const prettyPrint = (routes) => { }; export default prettyPrint; - -if (typeof module !== 'undefined') { - module.exports = prettyPrint; -} diff --git a/src/lib/request.js b/src/lib/request.js index 431e28f..c6eaadc 100644 --- a/src/lib/request.js +++ b/src/lib/request.js @@ -356,7 +356,3 @@ class REQUEST { } // end REQUEST class export default REQUEST; - -if (typeof module !== 'undefined') { - module.exports = REQUEST; -} diff --git a/src/lib/response.js b/src/lib/response.js index 81b1d0d..7a7c2b7 100644 --- a/src/lib/response.js +++ b/src/lib/response.js @@ -611,7 +611,3 @@ class RESPONSE { } // end Response class export default RESPONSE; - -if (typeof module !== 'undefined') { - module.exports = RESPONSE; -} diff --git a/src/lib/s3-service.js b/src/lib/s3-service.js index 091a1f5..73d505f 100644 --- a/src/lib/s3-service.js +++ b/src/lib/s3-service.js @@ -73,7 +73,12 @@ export const getSignedUrl = async ( } }; -const service = { +// The CommonJS artifact re-exports this object as its whole module value (see +// scripts/cjs-interop.js) so `require('lambda-api/lib/s3-service')` keeps returning ONE mutable +// service object: response.js reads the S3 methods through it and the unit suites stub them on +// it. It is exported here only so the build step can reach it — the `client` getter closes over +// the module-local client, so the footer cannot rebuild the object itself. +export const service = { get client() { return _client; }, @@ -81,8 +86,3 @@ const service = { getObject, getSignedUrl, }; - -if (typeof module !== 'undefined') { - Object.defineProperty(service, '__esModule', { value: true }); - module.exports = service; -} diff --git a/src/lib/statusCodes.js b/src/lib/statusCodes.js index 4e059d1..33df292 100644 --- a/src/lib/statusCodes.js +++ b/src/lib/statusCodes.js @@ -83,7 +83,3 @@ const statusCodes = { }; export default statusCodes; - -if (typeof module !== 'undefined') { - module.exports = statusCodes; -} From c2be2174ef3f1b2fdef2afcc0561489f49481bff Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sat, 22 Aug 2026 18:48:47 +0300 Subject: [PATCH 2/5] refactor: derive the CJS footer per module instead of enumerating files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The footer table listed all seven modules by name, so adding a new `src/lib/*.js` with a default export would silently get no footer and its CommonJS shape would drift from its siblings — a failure nothing would catch. The footer is now derived from the module's own exports: - only `default` -> collapse to that value - named exports -> leave SWC's output alone - both -> fail the build, ambiguous, needs an explicit entry That leaves two real exceptions rather than a seven-entry table: the package root (callable + `.default` self-reference) and lib/s3-service.js (no default export; must resolve to the mutable service object). Also drops the build-time contract check. It duplicated __tests__/module-compat.unit.js, which already runs on the publish path — `prepublishOnly` -> `npm test` -> `jest unit`, and `unit` matches module-compat.unit.js. dist/cjs is byte-for-byte identical to the previous implementation. --- AGENTS.md | 5 +- scripts/cjs-interop.js | 115 ++++++++++++++++++----------------------- 2 files changed, 55 insertions(+), 65 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 45ed324..0278231 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,10 @@ and bundlers that inline the ESM artifact into a generated CommonJS wrapper (esb `module`, wiping out their exports — on Lambda that reads as `Runtime.HandlerNotFound` (issue #346). `__tests__/module-compat.unit.js` fails the build if one reappears. -To change the CommonJS shape of a module, edit the footer table in `scripts/cjs-interop.js`. +The footer is derived per file, not enumerated: a module whose only export is `default` collapses +to that value, a module with named exports is left as SWC emitted it, and a module with both fails +the build until you add an explicit entry to `EXCEPTIONS` in `scripts/cjs-interop.js`. New files +are covered automatically. ## Testing diff --git a/scripts/cjs-interop.js b/scripts/cjs-interop.js index 3b8f61d..f993dde 100644 --- a/scripts/cjs-interop.js +++ b/scripts/cjs-interop.js @@ -13,21 +13,27 @@ * CONSUMER's `module`, wiping out their own exports — issue #346, which surfaced on Lambda as * `Runtime.HandlerNotFound: index.handler is undefined or not exported`. * - * So the footers are injected here, into `dist/cjs` only. Each one is written out per file rather - * than generated from a single template: `.default` is correct on the package root but would, for - * example, make `mimemap['default']` resolve to the whole MIME map. + * The rule, applied to every `src/**\/*.js` in turn: + * + * - a module whose only export is `default` collapses to that value + * - a module with named exports is left as SWC emitted it + * - a module with BOTH is ambiguous and fails the build (see EXCEPTIONS) */ const fs = require('fs'); const path = require('path'); -const CJS_DIR = path.join(__dirname, '..', 'dist', 'cjs'); +const ROOT = path.join(__dirname, '..'); +const SRC_DIR = path.join(ROOT, 'src'); +const CJS_DIR = path.join(ROOT, 'dist', 'cjs'); const MARKER = '/* CommonJS interop — injected by scripts/cjs-interop.js */'; -// Modules with a single `export default`: `require()` returns that value directly. -const DEFAULT_FOOTER = 'module.exports = exports.default;\n'; +const HAS_DEFAULT = /^export default /m; +const HAS_NAMED = /^export (const|let|var|function|class|\{)/m; -const FOOTERS = { +// Modules whose CommonJS shape is not "collapse to the default export". Anything listed here +// wins over the rule, so a module with both a default and named exports needs an entry. +const EXCEPTIONS = { 'index.js': 'var _createAPI = exports.default;\n' + 'module.exports = _createAPI;\n' + @@ -35,79 +41,60 @@ const FOOTERS = { "// refactor, so `require('lambda-api').default` keeps working for TypeScript consumers\n" + '// compiled to CommonJS with esModuleInterop:false (they emit `require(...).default(...)`).\n' + 'module.exports.default = _createAPI;\n', - 'lib/mimemap.js': DEFAULT_FOOTER, - 'lib/prettyPrint.js': DEFAULT_FOOTER, - 'lib/request.js': DEFAULT_FOOTER, - 'lib/response.js': DEFAULT_FOOTER, - 'lib/statusCodes.js': DEFAULT_FOOTER, - // s3-service has no default export. It must resolve to the single mutable `service` object so - // response.js and the unit suites (sinon.stub) operate on the same properties — SWC's own - // `_export()` emits non-configurable getters, which stubbing cannot replace. `__esModule` keeps - // SWC's `_interop_require_wildcard` returning the object untouched. - 'lib/s3-service.js': + // No default export. It must resolve to the single mutable `service` object so response.js and + // the unit suites (sinon.stub) operate on the same properties — SWC's own `_export()` emits + // non-configurable getters, which stubbing cannot replace. `__esModule` keeps SWC's + // `_interop_require_wildcard` returning the object untouched. + [path.join('lib', 's3-service.js')]: "Object.defineProperty(exports.service, '__esModule', { value: true });\n" + 'module.exports = exports.service;\n', }; -// Loaded back after patching so a toolchain change that breaks the contract fails the BUILD, -// not just the test suite (`npm test` and `prepublishOnly` both go through `npm run build`). -const CONTRACT = { - 'index.js': (m) => typeof m === 'function' && m.default === m, - 'lib/mimemap.js': (m) => - typeof m.json === 'string' && m.default === undefined, - 'lib/prettyPrint.js': (m) => typeof m === 'function', - 'lib/request.js': (m) => typeof m === 'function', - 'lib/response.js': (m) => typeof m === 'function', - 'lib/statusCodes.js': (m) => - typeof m[404] === 'string' && m.default === undefined, - 'lib/s3-service.js': (m) => - m.__esModule === true && - 'client' in m && - ['getObject', 'getSignedUrl', 'setConfig'].every(function (name) { - const descriptor = Object.getOwnPropertyDescriptor(m, name); - return ( - typeof m[name] === 'function' && - descriptor.writable && - descriptor.configurable - ); - }), -}; +const COLLAPSE_TO_DEFAULT = 'module.exports = exports.default;\n'; const fail = (message) => { console.error('cjs-interop: ' + message); // eslint-disable-line no-console process.exit(1); }; -Object.keys(FOOTERS).forEach((relative) => { - const file = path.join(CJS_DIR, relative); +const jsFilesIn = (dir) => + fs.readdirSync(dir, { withFileTypes: true }).reduce((acc, entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return acc.concat(jsFilesIn(full)); + return entry.name.endsWith('.js') ? acc.concat(full) : acc; + }, []); - if (!fs.existsSync(file)) { - fail('expected ' + relative + ' in dist/cjs — did build:cjs run?'); - } +jsFilesIn(SRC_DIR).forEach((sourceFile) => { + const relative = path.relative(SRC_DIR, sourceFile); + const source = fs.readFileSync(sourceFile, 'utf8'); - const source = fs.readFileSync(file, 'utf8'); + let footer = EXCEPTIONS[relative]; - if (source.indexOf(MARKER) !== -1) { - fail(relative + ' is already patched — run `npm run clean` first'); + if (!footer) { + if (!HAS_DEFAULT.test(source)) return; // named exports only — SWC's output is already right + if (HAS_NAMED.test(source)) { + fail( + 'src/' + + relative + + ' has BOTH a default and named exports, so its CommonJS shape is ambiguous — ' + + 'add an explicit entry to EXCEPTIONS in this file.' + ); + } + footer = COLLAPSE_TO_DEFAULT; } - fs.writeFileSync( - file, - source.replace(/\s*$/, '\n') + '\n' + MARKER + '\n' + FOOTERS[relative] - ); -}); - -Object.keys(CONTRACT).forEach((relative) => { - const file = path.join(CJS_DIR, relative); - let loaded; + const target = path.join(CJS_DIR, relative); - try { - loaded = require(file); - } catch (e) { - fail('patched ' + relative + ' failed to load: ' + e.message); + if (!fs.existsSync(target)) { + fail('expected ' + relative + ' in dist/cjs — did build:cjs run?'); } - if (!CONTRACT[relative](loaded)) { - fail(relative + ' did not end up with the expected CommonJS shape'); - } + fs.writeFileSync( + target, + fs.readFileSync(target, 'utf8').replace(/\s*$/, '\n') + + '\n' + + MARKER + + '\n' + + footer + ); }); From f8103171cd575731feadf0605149fa255b4c4313 Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sat, 22 Aug 2026 18:55:41 +0300 Subject: [PATCH 3/5] fix: classify export shapes totally in the CJS interop step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The named-export check listed keywords (`const|let|var|function|class|{`), which only covered the forms that happen to exist in src/ today. Three forms slipped through, and two of them were silent: export async function a(){}; export default b -> footer applied, `a` DROPPED export * from './x.js'; export default b -> footer applied, re-exports DROPPED export { x as default } -> no footer, require() gave {default} Stated as a negative lookahead instead — any `export` line that is not `export default` is a named export — so every form classifies correctly and mixed modules fail the build rather than losing exports. `as default` now counts as a default export, so `export { x as default }` is caught too. Verified across all five shapes: the three above now fail the build loudly, default-only still collapses, named-only is still left alone. dist/cjs is byte-for-byte unchanged. Also marks the s3-service `service` export @internal — it exists so the build step can reach the object, not as public API. --- scripts/cjs-interop.js | 18 +++++++++--------- src/lib/s3-service.js | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/scripts/cjs-interop.js b/scripts/cjs-interop.js index f993dde..451fe45 100644 --- a/scripts/cjs-interop.js +++ b/scripts/cjs-interop.js @@ -28,8 +28,11 @@ const SRC_DIR = path.join(ROOT, 'src'); const CJS_DIR = path.join(ROOT, 'dist', 'cjs'); const MARKER = '/* CommonJS interop — injected by scripts/cjs-interop.js */'; -const HAS_DEFAULT = /^export default /m; -const HAS_NAMED = /^export (const|let|var|function|class|\{)/m; +// Any `export` line that is not `export default` is a named export. Stated as a negative +// lookahead rather than a list of keywords so forms nobody uses today — `export async function`, +// `export * from`, `export { x as default }` — classify correctly instead of slipping through. +const HAS_DEFAULT = /^export default\b|\bas default\b/m; +const HAS_NAMED = /^export (?!default\b)/m; // Modules whose CommonJS shape is not "collapse to the default export". Anything listed here // wins over the rule, so a module with both a default and named exports needs an entry. @@ -76,8 +79,9 @@ jsFilesIn(SRC_DIR).forEach((sourceFile) => { fail( 'src/' + relative + - ' has BOTH a default and named exports, so its CommonJS shape is ambiguous — ' + - 'add an explicit entry to EXCEPTIONS in this file.' + ' mixes a default export with named exports, so collapsing it to the default would ' + + 'silently drop the rest — add an explicit entry to EXCEPTIONS in this file. ' + + '(`export { x as default }` counts; prefer `export default x`.)' ); } footer = COLLAPSE_TO_DEFAULT; @@ -91,10 +95,6 @@ jsFilesIn(SRC_DIR).forEach((sourceFile) => { fs.writeFileSync( target, - fs.readFileSync(target, 'utf8').replace(/\s*$/, '\n') + - '\n' + - MARKER + - '\n' + - footer + fs.readFileSync(target, 'utf8').trimEnd() + '\n\n' + MARKER + '\n' + footer ); }); diff --git a/src/lib/s3-service.js b/src/lib/s3-service.js index 73d505f..09a80be 100644 --- a/src/lib/s3-service.js +++ b/src/lib/s3-service.js @@ -73,6 +73,7 @@ export const getSignedUrl = async ( } }; +// @internal — not part of the public API. // The CommonJS artifact re-exports this object as its whole module value (see // scripts/cjs-interop.js) so `require('lambda-api/lib/s3-service')` keeps returning ONE mutable // service object: response.js reads the S3 methods through it and the unit suites stub them on From fad9b00584cee23b30938320e87b6927f0e70ba4 Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sat, 22 Aug 2026 19:04:32 +0300 Subject: [PATCH 4/5] test: pin the CJS footer classification for every export form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The classification is the part of the build step that fails silently — a module mixing a default with named exports would collapse to the default and drop the rest — but it had no automated coverage; it was only ever checked by hand. scripts/cjs-interop.js now exposes `footerFor(relative, source)` and runs its side effects under `require.main === module`, so the decision is testable without touching the filesystem. __tests__/cjs-interop.unit.js pins 17 cases across all three outcomes plus the exceptions, and runs under `jest unit`, so it gates PRs on the Node matrix. Mutation-checked: restoring the enumerated named-export regex fails `default + async function` and `default + star`; dropping the `as default` alternation fails `export { x as default }`. dist/cjs is byte-for-byte unchanged. --- __tests__/cjs-interop.unit.js | 88 ++++++++++++++++++++++++++++++++++ scripts/cjs-interop.js | 89 +++++++++++++++++++++++------------ 2 files changed, 146 insertions(+), 31 deletions(-) create mode 100644 __tests__/cjs-interop.unit.js diff --git a/__tests__/cjs-interop.unit.js b/__tests__/cjs-interop.unit.js new file mode 100644 index 0000000..171e84c --- /dev/null +++ b/__tests__/cjs-interop.unit.js @@ -0,0 +1,88 @@ +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +const { + footerFor, + COLLAPSE_TO_DEFAULT, + EXCEPTIONS, +} = require('../scripts/cjs-interop.js'); + +const SRC = path.join(__dirname, '..', 'src'); + +// The build step decides each module's CommonJS shape from its ESM source. Getting that +// classification wrong is silent in the worst direction — a module that mixes a default with +// named exports would collapse to the default and DROP the rest — so every export form is +// pinned here rather than only the ones src/ happens to use today. +describe('cjs-interop footer classification:', function () { + describe('collapses to the default export', function () { + const cases = { + 'export default': 'const t = () => 1;\nexport default t;\n', + 'export default class': 'export default class T {}\n', + 'export default function': 'export default function t() {}\n', + 'export default object': 'export default { a: 1 };\n', + }; + + Object.keys(cases).forEach((name) => { + it(name, function () { + expect(footerFor('lib/probe.js', cases[name])).toBe( + COLLAPSE_TO_DEFAULT + ); + }); + }); + }); + + describe('leaves SWC output alone', function () { + const cases = { + 'export const': 'export const a = 1;\n', + 'export async function': 'export async function a() {}\n', + 'export list': 'const a = 1;\nexport { a };\n', + 'export star': "export * from './x.js';\n", + 'no exports at all': 'const a = 1;\n', + }; + + Object.keys(cases).forEach((name) => { + it(name, function () { + expect(footerFor('lib/probe.js', cases[name])).toBeNull(); + }); + }); + }); + + describe('refuses ambiguous shapes rather than dropping exports', function () { + const cases = { + 'default + const': 'export const a = 1;\nexport default a;\n', + 'default + async function': + 'export async function a() {}\nconst t = 1;\nexport default t;\n', + 'default + star': "export * from './x.js';\nexport default 1;\n", + 'default + named list': + 'const a = 1;\nconst t = 2;\nexport { a };\nexport default t;\n', + 'export { x as default }': 'const t = 1;\nexport { t as default };\n', + }; + + Object.keys(cases).forEach((name) => { + it(name, function () { + expect(() => footerFor('lib/probe.js', cases[name])).toThrow( + /ambiguous|silently drop/ + ); + }); + }); + }); + + describe('exceptions win over the rule', function () { + Object.keys(EXCEPTIONS).forEach((relative) => { + it(`${relative} uses its explicit footer`, function () { + // Passing source that the rule would classify differently proves the override applies. + expect(footerFor(relative, 'export const a = 1;\n')).toBe( + EXCEPTIONS[relative] + ); + }); + }); + + it('every exception names a file that still exists in src/', function () { + Object.keys(EXCEPTIONS).forEach((relative) => { + expect(fs.existsSync(path.join(SRC, relative))).toBe(true); + }); + }); + }); +}); diff --git a/scripts/cjs-interop.js b/scripts/cjs-interop.js index 451fe45..ef7ffd5 100644 --- a/scripts/cjs-interop.js +++ b/scripts/cjs-interop.js @@ -55,11 +55,6 @@ const EXCEPTIONS = { const COLLAPSE_TO_DEFAULT = 'module.exports = exports.default;\n'; -const fail = (message) => { - console.error('cjs-interop: ' + message); // eslint-disable-line no-console - process.exit(1); -}; - const jsFilesIn = (dir) => fs.readdirSync(dir, { withFileTypes: true }).reduce((acc, entry) => { const full = path.join(dir, entry.name); @@ -67,34 +62,66 @@ const jsFilesIn = (dir) => return entry.name.endsWith('.js') ? acc.concat(full) : acc; }, []); -jsFilesIn(SRC_DIR).forEach((sourceFile) => { - const relative = path.relative(SRC_DIR, sourceFile); - const source = fs.readFileSync(sourceFile, 'utf8'); - - let footer = EXCEPTIONS[relative]; - - if (!footer) { - if (!HAS_DEFAULT.test(source)) return; // named exports only — SWC's output is already right - if (HAS_NAMED.test(source)) { - fail( - 'src/' + - relative + - ' mixes a default export with named exports, so collapsing it to the default would ' + - 'silently drop the rest — add an explicit entry to EXCEPTIONS in this file. ' + - '(`export { x as default }` counts; prefer `export default x`.)' +/** + * Decide which CommonJS footer a module needs, from its ESM source. + * + * @param {string} relative path of the module inside `src/` + * @param {string} source its ESM source + * @returns {string|null} the footer to append, or null when SWC's output is already correct + * @throws {Error} when the export shape is ambiguous and needs an explicit EXCEPTIONS entry + */ +const footerFor = (relative, source) => { + if (EXCEPTIONS[relative]) return EXCEPTIONS[relative]; + + // Named exports only — SWC's output is already the right shape. + if (!HAS_DEFAULT.test(source)) return null; + + if (HAS_NAMED.test(source)) { + throw new Error( + 'src/' + + relative + + ' mixes a default export with named exports, so collapsing it to the default would ' + + 'silently drop the rest — add an explicit entry to EXCEPTIONS in scripts/cjs-interop.js. ' + + '(`export { x as default }` counts; prefer `export default x`.)' + ); + } + + return COLLAPSE_TO_DEFAULT; +}; + +const run = () => { + jsFilesIn(SRC_DIR).forEach((sourceFile) => { + const relative = path.relative(SRC_DIR, sourceFile); + const footer = footerFor(relative, fs.readFileSync(sourceFile, 'utf8')); + + if (!footer) return; + + const target = path.join(CJS_DIR, relative); + + if (!fs.existsSync(target)) { + throw new Error( + 'expected ' + relative + ' in dist/cjs — did build:cjs run?' ); } - footer = COLLAPSE_TO_DEFAULT; - } - const target = path.join(CJS_DIR, relative); + fs.writeFileSync( + target, + fs.readFileSync(target, 'utf8').trimEnd() + + '\n\n' + + MARKER + + '\n' + + footer + ); + }); +}; + +module.exports = { footerFor, run, COLLAPSE_TO_DEFAULT, EXCEPTIONS, MARKER }; - if (!fs.existsSync(target)) { - fail('expected ' + relative + ' in dist/cjs — did build:cjs run?'); +if (require.main === module) { + try { + run(); + } catch (e) { + console.error('cjs-interop: ' + e.message); // eslint-disable-line no-console + process.exit(1); } - - fs.writeFileSync( - target, - fs.readFileSync(target, 'utf8').trimEnd() + '\n\n' + MARKER + '\n' + footer - ); -}); +} From f827ec1fb439f01a93dc155322a5d0d766d9ead9 Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sat, 22 Aug 2026 19:06:48 +0300 Subject: [PATCH 5/5] refactor: key the CJS footer exceptions by plain path, drop generated comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The s3-service key was a computed `path.join('lib','s3-service.js')` so it would match `path.relative` on Windows. The relative path is now normalized to forward slashes at the call site instead, so the table reads as plain strings. The explanatory comments moved out of the emitted footers and into the table itself — dist/ is generated output, and the MARKER already points readers at this script. The root footer drops from six lines to three. --- scripts/cjs-interop.js | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/scripts/cjs-interop.js b/scripts/cjs-interop.js index ef7ffd5..f427fa7 100644 --- a/scripts/cjs-interop.js +++ b/scripts/cjs-interop.js @@ -34,21 +34,24 @@ const MARKER = '/* CommonJS interop — injected by scripts/cjs-interop.js */'; const HAS_DEFAULT = /^export default\b|\bas default\b/m; const HAS_NAMED = /^export (?!default\b)/m; -// Modules whose CommonJS shape is not "collapse to the default export". Anything listed here -// wins over the rule, so a module with both a default and named exports needs an entry. +// Modules whose CommonJS shape is not "collapse to the default export". Keyed by path relative +// to src/, always with forward slashes. An entry wins over the rule, so a module with both a +// default and named exports needs one. const EXCEPTIONS = { + // The package root stays callable AND keeps the `.default` self-reference the CommonJS build + // shipped before the dual-package refactor, so TypeScript consumers compiled to CommonJS with + // esModuleInterop:false (they emit `require('lambda-api').default(...)`) keep working. + // The local is `_createAPI`, not `_default`: SWC already declares `const _default` here. 'index.js': 'var _createAPI = exports.default;\n' + 'module.exports = _createAPI;\n' + - '// Preserve the `.default` self-reference the CommonJS build shipped before the dual-package\n' + - "// refactor, so `require('lambda-api').default` keeps working for TypeScript consumers\n" + - '// compiled to CommonJS with esModuleInterop:false (they emit `require(...).default(...)`).\n' + 'module.exports.default = _createAPI;\n', - // No default export. It must resolve to the single mutable `service` object so response.js and - // the unit suites (sinon.stub) operate on the same properties — SWC's own `_export()` emits + + // No default export. Must resolve to the single mutable `service` object so response.js and the + // unit suites (sinon.stub) share one set of properties — SWC's `_export()` emits // non-configurable getters, which stubbing cannot replace. `__esModule` keeps SWC's // `_interop_require_wildcard` returning the object untouched. - [path.join('lib', 's3-service.js')]: + 'lib/s3-service.js': "Object.defineProperty(exports.service, '__esModule', { value: true });\n" + 'module.exports = exports.service;\n', }; @@ -91,7 +94,10 @@ const footerFor = (relative, source) => { const run = () => { jsFilesIn(SRC_DIR).forEach((sourceFile) => { - const relative = path.relative(SRC_DIR, sourceFile); + const relative = path + .relative(SRC_DIR, sourceFile) + .split(path.sep) + .join('/'); const footer = footerFor(relative, fs.readFileSync(sourceFile, 'utf8')); if (!footer) return;