Jens-G commented on PR #3747:
URL: https://github.com/apache/thrift/pull/3747#issuecomment-5635254720
### Code review
No blocking issues found. Checked for bugs and CLAUDE.md compliance.
Two suggestions, below the bar for an issue but verified:
- This change needs a JIRA ticket. CONTRIBUTING.md: "All significant changes
require an Apache Jira THRIFT Issue ticket. Trivial changes such as fixing a
typo or a compiler warning do not." Moving grunt-contrib-qunit from 3.1 to 10.2
and Puppeteer from 1.20 to 25.8 changes about 55 package entries in the
lockfile and the Node.js requirement of this test path. With a ticket, the
title and the commit subject also get their `THRIFT-NNNN:` prefix; right now
they don't match ("Remove extract-zip from TypeScript test dependencies" vs.
"Update TypeScript QUnit dependencies").
https://github.com/apache/thrift/blob/5acac82d3bdba57f06cc3543f7919e95dd8fc98e/CONTRIBUTING.md#L15-L17
- The Puppeteer override makes Node.js 22.12 the minimum for the lib/ts
browser tests: puppeteer, puppeteer-core and @puppeteer/browsers all declare
`"node": ">=22.12.0"` in the lockfile. #3724 refreshed the same lockfile and
stated that it kept Node.js 16 compatibility. The PR description mentions the
new floor, but nothing in the tree records it. Copilot's inline comment on this
line is still unanswered.
https://github.com/apache/thrift/blob/5acac82d3bdba57f06cc3543f7919e95dd8fc98e/lib/ts/package.json#L39-L43
🤖 Generated with [Claude Code](https://claude.ai/code)
--
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]
