THRIFT-6130: Restore CommonJS UUID compatibility

[vikrantpuppala]

Summary

• constrain uuid to ^11.1.1, its maintained legacy-11 floor with both CommonJS and ESM conditional exports
• preserve the existing UUID parse/stringify and generated-code v4 APIs
• add a behavioral CJS-only subprocess regression plus binary/compact UUID round trips

Motivation

The Node.js runtime and generated CommonJS code load uuid with require(), but uuid 12 and newer are ESM-only. Consequently, require("thrift") fails in CommonJS tooling such as Jest before application tests execute.

The 11.1.1 floor is deliberate: it is the maintained legacy-11 release, while the caret range remains within v11 and cannot resolve an ESM-only major. It should not be relaxed to an earlier 11.x release.

Downstream reproduction: databricks/databricks-sql-nodejs#461

Test plan

• npm run lint-tests
• npx prettier --check lib/nodejs/test/uuid.test.js
• node lib/nodejs/test/binary.test.js
• node lib/nodejs/test/header.test.js
• node lib/nodejs/test/uuid.test.js on Node 16 and Node 24; modern Node runs the load check with require(esm) explicitly disabled
• packed-tarball Jest smoke test on Node 20 and Node 24
• packed consumer production audit: 0 vulnerabilities

The broad npm run lint command remains red on existing legacy files on master; the changed test file and the complete Node.js test directory pass ESLint.

Generated-by: OpenAI Codex (GPT-5)

[Jens-G]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

One suggestion, not a blocker:

The regression guard is weaker than it looks on modern Node. Re-installing the broken dependency (uuid@14.0.0) and re-running the new test on Node v20.19.2, only the two static exports-shape assertions fail:

thrift/lib/nodejs/test/uuid.test.js

Lines 33 to 44 in e398e41

	const browserExport = uuidPackage.exports["."].browser;
	assert.equal(
	typeof nodeExport === "object" ? typeof nodeExport.require : "undefined",
	"string",
	);
	assert.equal(
	typeof browserExport === "object"
	? typeof browserExport.require
	: "undefined",
	"string",
	);
	let uuid;

Every other assertion passes against the broken dependency — including the one named "CommonJS runtime loads without an ESM transform", and both protocol round-trips — because Node >= 20.19 satisfies require(esm) natively, so require("uuid") of an ESM-only package silently succeeds:

thrift/lib/nodejs/test/uuid.test.js

Lines 51 to 58 in e398e41

	test("CommonJS runtime loads without an ESM transform", function (assert) {
	assert.doesNotThrow(function () {
	require("../lib/thrift");
	});
	assert.end();
	});

No workflow pins a Node version (the lib-nodejs job in .github/workflows/build.yml runs on bare ubuntu-24.04), so CI takes the masking path, and the Node 16 run from the test plan is the only thing that exercised the real throw path. The file as a whole still goes red on a recurrence via the two shape assertions, so the guard does work — but the behavioural tests carry none of that weight, and the shape assertions depend on uuid's internal exports layout, which is not part of its semver contract.

Two ways to re-arm it, if you think it's worth it: pin an older Node on one lib-nodejs matrix entry so require(esm) is not available, or assert through a resolver that does not implement require(esm) — which is what the downstream report (databricks/databricks-sql-nodejs#461) actually hits, since Jest resolves CJS itself rather than deferring to Node.

Separately, worth stating in the commit message or PR body: ^11.1.1 is not merely "the last dual-export line". 11.1.1 is the designated patched release for the 11.x maintenance line and carries npm's legacy-11 dist-tag, published alongside 12.0.1 and 13.0.1. So the floor is a deliberate, non-obvious choice, not an arbitrary downgrade — worth recording so a future reader does not relax it to ^11.0.0. (Thrift's own code paths were never affected either way: the runtime uses only parse/stringify and generated code uses v4.)

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[vikrantpuppala]

Thanks — addressed in 152b8bd.

• Replaced the exports-shape assertions and in-process load check with a subprocess that disables require(esm) when the runtime supports that flag; older Node versions naturally use their CJS-only loader.
• The subprocess loads Thrift, requires uuid, and executes the generated-code v4 API, so uuid@14 now fails behaviorally even on modern CI Node.
• Kept the binary/compact UUID round-trip coverage.
• Documented that 11.1.1 is the deliberate legacy-11 maintenance floor in both the commit and PR body.

Validated on Node 16 and Node 24; formatting and npm run lint-tests pass.