diff --git a/AGENTS.md b/AGENTS.md index 0956a3e..0278231 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,6 +59,23 @@ 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. + +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 - Tests live in `__tests__/*.unit.js` @@ -72,6 +89,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__/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/__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..f427fa7 --- /dev/null +++ b/scripts/cjs-interop.js @@ -0,0 +1,133 @@ +'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`. + * + * 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 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 */'; + +// 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". 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' + + 'module.exports.default = _createAPI;\n', + + // 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. + 'lib/s3-service.js': + "Object.defineProperty(exports.service, '__esModule', { value: true });\n" + + 'module.exports = exports.service;\n', +}; + +const COLLAPSE_TO_DEFAULT = 'module.exports = exports.default;\n'; + +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; + }, []); + +/** + * 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) + .split(path.sep) + .join('/'); + 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?' + ); + } + + fs.writeFileSync( + target, + fs.readFileSync(target, 'utf8').trimEnd() + + '\n\n' + + MARKER + + '\n' + + footer + ); + }); +}; + +module.exports = { footerFor, run, COLLAPSE_TO_DEFAULT, EXCEPTIONS, MARKER }; + +if (require.main === module) { + try { + run(); + } catch (e) { + console.error('cjs-interop: ' + e.message); // eslint-disable-line no-console + process.exit(1); + } +} 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..09a80be 100644 --- a/src/lib/s3-service.js +++ b/src/lib/s3-service.js @@ -73,7 +73,13 @@ export const getSignedUrl = async ( } }; -const service = { +// @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 +// 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 +87,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; -}