Jens-G commented on PR #3747:
URL: https://github.com/apache/thrift/pull/3747#issuecomment-5749377326
### Ran this locally
I built the PR head (0fc68b1d2) and master (6b1d360e7) side by side in the
same container —
Ubuntu 22.04 with ant/Java 17/unzip, Node v22.23.2, compiler built from each
tree — to see what
holds up. Summary: **everything in this PR that can be exercised today
works, and the parts that
cannot be exercised cannot be exercised on master either.**
**`npm install` and the ant target are fine.** Install on Node 22.23.2 exits
0, `extract-zip` is
gone from the tree, and `ant download_jslibs` fetches QUnit 2.26.0 over
HTTPS with
`BUILD SUCCESSFUL`. `npm audit` goes from 13 findings on master (4 low, 9
high) to 9 here
(4 low, 5 high).
**The new QUnit runner works.** Against a minimal QUnit page served over
HTTP — the same
mechanism `Gruntfile.js` uses — `grunt-contrib-qunit` 10.2 + Puppeteer 25.8
+ QUnit 2.26.0 gives
`2 tests completed, 0 failed`, exit 0; with a deliberately failing assertion
it gives `1 failed`
and exit 6. The old stack behaves the same. So the plugin/QUnit event API
pairing this PR is
about holds up. What I could *not* confirm is Thrift's own `test.html` under
QUnit 2.26 — see
below for why.
**`npx grunt` does not complete, on this branch or on master, for two
reasons that predate this
PR.** Both reproduce identically in both trees:
1. `shell:BuildTS` fails with `test.ts(58,34)` and `test.ts(72,34)` `TS2345:
Argument of type
'string' is not assignable to parameter of type
'Buffer<ArrayBufferLike>'`. `test.ts` is
byte-identical in both trees, and `typescript` (5.7.3) and `@types/node`
(22.10.5) are the
same in both lockfiles.
2. With that cast away locally, `shell:BrowserifyCompiledTS` then fails —
`browserify` 16.2.3
cannot parse the current `uuid` that the root `npm install` pulls in.
Filed as THRIFT-6317 and THRIFT-6318, plus THRIFT-6319 for the underlying
reason nobody noticed:
`lib/ts` is in no `SUBDIRS` block and `configure.ac` has no
`AX_THRIFT_LIB(ts, …)`, so
`check-local` never runs, and no CI job runs it either.
**One verified behaviour change worth a line in the PR.** Copilot's note
about `yauzl` is
correct, and it is a real difference rather than a theoretical one:
| | without system `unzip` | with `unzip` |
|---|---|---|
| this branch (`@puppeteer/browsers` 3.2.1) | **exit 1** — extraction fails,
see below | exit 0 |
| master (`extract-zip`, pure JS) | exit 0, Chromium extracted | — |
```
Error: All providers failed for chrome-headless-shell 152.0.7977.42:
- DefaultProvider: Extraction failed: no zip archiver is available.
Install `unzip`
(or `tar.exe`/Powershell on Windows), or add the optional `yauzl`
dependency.
```
So the unzip dependency moves from npm into the operating system. Not a
blocker — every CI image
here has `unzip` — but either adding `yauzl` as a devDependency or naming
`unzip` as a
prerequisite would keep this path self-contained.
**Two small things while you are in here:** `package.json` still has no
`engines.node`, though
puppeteer, puppeteer-core and `@puppeteer/browsers` all declare `>=22.12.0`;
and
`build.xml` line 102 still fetches jQuery over plain HTTP while the four
QUnit URLs right below
it now use HTTPS. I am fixing the same pattern in `lib/js/test/build.xml`,
`test/index.html`,
`test/features/index.html` and `tutorial/js/tutorial.html` under THRIFT-6316
(#3899), and deliberately
leaving that one line to you so the two changes do not collide.
🤖 Generated with [Claude Code](https://claude.com/claude-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]
