Jens-G commented on PR #3685: URL: https://github.com/apache/thrift/pull/3685#issuecomment-5124161232
### 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 (`[email protected]`) and re-running the new test on Node v20.19.2, only the two static `exports`-shape assertions fail: https://github.com/apache/thrift/blob/e398e416e0423e63051f5cd58c477c582fc01dff/lib/nodejs/test/uuid.test.js#L33-L44 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: https://github.com/apache/thrift/blob/e398e416e0423e63051f5cd58c477c582fc01dff/lib/nodejs/test/uuid.test.js#L51-L58 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](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
