Skip to content

THRIFT-6429: Update TypeScript browser-test dependencies - #3747

Merged
kpumuk merged 1 commit into
apache:masterfrom
kpumuk:ts-extract-zip-dependencies
Oct 16, 2026
Merged

kpumuk merged 1 commit into
apache:masterfrom
kpumuk:ts-extract-zip-dependencies

Conversation

@kpumuk

@kpumuk kpumuk commented Aug 24, 2026 •

Copy link
Copy Markdown
Member

The TypeScript browser tests currently pull extract-zip 1.7.0 through grunt-contrib-qunit 3.1.0 and Puppeteer 1.20.0.

This updates grunt-contrib-qunit to 11 and uses its declared Puppeteer 25 dependency without an override. The lockfile selects Puppeteer 25.8.0 and no longer includes extract-zip. The downloaded QUnit assets move to 2.26.0 to match the plugin's event API, and all browser-test library downloads use HTTPS. Adding yauzl preserves browser ZIP installation when system unzip is unavailable.

The TypeScript test tools now require Node.js 22.12.0 or newer. This is declared in lib/ts/package.json, documented under the README's 0.26.0 breaking changes, and supported by selecting Node.js 22 in the TypeScript CI job. This does not change Node.js support for the Thrift runtime.

  • Did you create an Apache Jira ticket? THRIFT-6429 (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@kpumuk
kpumuk requested a review from emmenlau as a code owner August 24, 2026 13:02
Copilot AI lite review requested due to automatic review settings August 24, 2026 13:02
@mergeable mergeable Bot added the typescript label Aug 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the TypeScript browser test toolchain in lib/ts to remove the extract-zip dependency by upgrading the Grunt QUnit runner stack and aligning the downloaded QUnit assets with the plugin’s expected event API.

Changes:

  • Bump grunt-contrib-qunit to ^10.2.0 and pin its Puppeteer runner to 25.8.0 via overrides.
  • Update QUnit assets downloaded for TS tests to 2.26.0 and switch those downloads to HTTPS.
  • Regenerate lib/ts/package-lock.json to reflect the new dependency tree (removing extract-zip and older Puppeteer-era deps).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/ts/test/build.xml Updates downloaded QUnit asset versions/URLs used by the TS test build.
lib/ts/package.json Upgrades grunt-contrib-qunit and adds an override to select Puppeteer 25.8.0.
lib/ts/package-lock.json Updates the resolved dependency graph to match the new dev dependency set (removing extract-zip).
Files not reviewed (1)
  • lib/ts/package-lock.json: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/ts/test/build.xml Outdated
Comment thread lib/ts/package.json Outdated
Comment on lines +40 to +42
"grunt-contrib-qunit": {
"puppeteer": "25.8.0"
},
@Jens-G Jens-G assigned Jens-G and kpumuk and unassigned Jens-G Aug 28, 2026
@Jens-G

Jens-G commented Sep 11, 2026

Copy link
Copy Markdown
Member

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").

thrift/CONTRIBUTING.md

Lines 15 to 17 in 5acac82

1. All significant changes require an [Apache Jira THRIFT Issue](http://issues.apache.org/jira/browse/THRIFT) ticket. Trivial changes such as fixing a typo or a compiler warning do not.

  • 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. Update JavaScript js-yaml dependencies #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.

"grunt-legacy-util": "2.0.2",
"grunt-contrib-qunit": {
"puppeteer": "25.8.0"
},
"jshint": {

🤖 Generated with Claude Code

@kpumuk
kpumuk force-pushed the ts-extract-zip-dependencies branch from 5acac82 to 0fc68b1 Compare September 14, 2026 17:57
Copilot AI review requested due to automatic review settings September 14, 2026 17:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved dependency-installation and CI-validation issues remain.

Review details

Files not reviewed (1)

  • lib/ts/package-lock.json: Generated file

Suppressed comments (2)

lib/ts/package-lock.json:58

  • The new @puppeteer/browsers dependency declares yauzl as an optional ZIP extractor, but this lockfile leaves that peer uninstalled. Its browser download path therefore falls back to a system unzip executable; make -C lib/ts check can fail during Puppeteer's install/download step on minimal environments without unzip. Add yauzl as a development dependency (and regenerate the lockfile), or make unzip an explicit prerequisite for this test path.
    lib/ts/package.json:22
  • This major tooling update is not exercised by the repository's automated tests: the build workflow's TypeScript step runs make -C lib/nodets check, while lib/ts is not included in lib/Makefile.am. Please add a CI invocation for this Grunt/Puppeteer path (with Node >=22.12) or an equivalent validation before relying on the new runner and QUnit assets.
    "grunt-contrib-qunit": "^10.2.0",
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Jens-G

Jens-G commented Sep 20, 2026

Copy link
Copy Markdown
Member

Ran this locally

I built the PR head (0fc68b1) and master (6b1d360) 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

@Jens-G
Jens-G marked this pull request as draft September 26, 2026 19:50
@kpumuk
kpumuk force-pushed the ts-extract-zip-dependencies branch from 0fc68b1 to 33af63b Compare October 16, 2026 01:40
Client: js,ts

Co-Authored-By: OpenAI Codex (gpt-6.1-sol) <codex@openai.com>
@kpumuk
kpumuk force-pushed the ts-extract-zip-dependencies branch from 33af63b to 16b7857 Compare October 16, 2026 02:03
@mergeable mergeable Bot added the javascript Pull requests that update Javascript code label Oct 16, 2026
@kpumuk kpumuk changed the title Remove extract-zip from TypeScript test dependencies THRIFT-6429: Remove extract-zip from TypeScript test dependencies Oct 16, 2026
@mergeable mergeable Bot added the github_actions Pull requests that update GitHub Actions code label Oct 16, 2026
@kpumuk kpumuk changed the title THRIFT-6429: Remove extract-zip from TypeScript test dependencies THRIFT-6429: Update TypeScript browser-test dependencies Oct 16, 2026
@kpumuk

kpumuk commented Oct 16, 2026 •

Copy link
Copy Markdown
Member Author

Thanks for the investigation, @Jens-G. I’ve rebased onto master to pick up the separate TypeScript compilation, Browserify, and CI fixes, and updated this PR:

  • Created THRIFT-6429 and aligned the PR title and commit message.
  • Upgraded grunt-contrib-qunit to 11, which declares Puppeteer 25 directly, removing the override. The regenerated lockfile retains Puppeteer 25.8.0 without extract-zip; QUnit assets are aligned to 2.26.0.
  • Added yauzl as a development dependency so ZIP extraction does not require system unzip.
  • Declared engines.node >=22.12.0, selected Node 22 in the existing lib-ts CI job, and documented the requirement under the README’s 0.26.0 breaking changes. This applies to the TypeScript test tools, not the Thrift runtime.
  • Switched the remaining jQuery download to HTTPS, making all five browser-test asset downloads consistent.

Compilation, lint, and all 26 browser tests passed on Node 22.12.0. I also reproduced the missing-extractor failure and verified that fresh ZIP-fixture installation succeeds with yauzl when unzip is unavailable.

@kpumuk
kpumuk marked this pull request as ready for review October 16, 2026 02:08
Copilot AI lite review requested due to automatic review settings October 16, 2026 02:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The package-level Node.js engine constraint may reject runtime consumers and should be limited to test tooling or CI.

1 open finding
Files not reviewed (1)
  • lib/ts/package-lock.json: Generated file

🧠 Review effort: Lite

@kpumuk
kpumuk merged commit 2aafe4f into apache:master Oct 16, 2026
112 checks passed
@kpumuk
kpumuk deleted the ts-extract-zip-dependencies branch October 16, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code javascript Pull requests that update Javascript code typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants