Skip to content

THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise#3528

Open
jimexist wants to merge 3 commits into
masterfrom
claude/zen-black-39b255
Open

THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise#3528
jimexist wants to merge 3 commits into
masterfrom
claude/zen-black-39b255

Conversation

@jimexist
Copy link
Copy Markdown
Member

Summary

Removes the runtime dependency on the q Promise library and switches the JS/TS code generator to emit native Promise by default. Adds a js:native_promise=[true|false] compiler flag (default true) so users who still need the old output can opt back in.

What changed

Compiler (compiler/cpp/src/thrift/generate/t_js_generator.cc)

  • In non-ES6 mode the generated processor now emits new Promise((resolve) => resolve(handler.fn(args))) instead of Q.fcall(...).
  • The non-ES6 Node client now emits new Promise((resolve, reject) => { ... }) instead of Q.defer() / _defer.promise.
  • The non-ES6 JS/TS _includes no longer pull in Q by default.
  • New flag js:native_promise=[true|false] (default true). When false, the legacy Q.fcall / Q.defer output is restored and Q is imported directly via require('q') (no longer routed through thrift.Q), so legacy users only need q in their own node_modules.

Runtime (lib/nodejs/lib/thrift/index.js, lib/nodejs/lib/thrift/browser.js)

  • Removed exports.Q = require("q").

Dependencies (package.json, package-lock.json, lib/nodejs/test/package-lock.json)

  • Dropped q from dependencies and @types/q from devDependencies; pruned the matching lockfile entries.

Tests (lib/nodets/test/test_driver.ts, lib/nodets/test/test_handler.ts)

  • Q.IPromise<T>Promise<T>; Q.resolve(x)Promise.resolve(x); .fail(...).catch(...).
  • Removed import Q = require("q") / import Q = thrift.Q.
  • AsyncThriftTestHandler method return types changed to Promise<void> since the callback delivers the value and the body returns Promise.resolve() (native Promise<void> is not assignable to Promise<string> etc. under TS strict mode, unlike Q's loose typings).
  • Renamed "Q Promise Client Tests""Promise Client Tests".

Why

q has been in maintenance mode for years; native Promise is available in every supported runtime (node >= 10.18.0 per package.json). Removing it shrinks the dependency footprint and brings the non-ES6 / Node output in line with what the ES6 generator already emitted. The native_promise flag is an escape hatch for downstream code that still depends on the Q-shaped output.

Heads-up for reviewers

  • A JIRA ticket should be filed under THRIFT — this is non-trivial and the PR title / commit should be prefixed with the ticket ID before merge.
  • I was unable to run the JS/TS toolchain or build the C++ compiler in my environment. Please verify locally: npm run lint, npm run test-ts, and a thrift compiler build that regenerates an example service in both default and native_promise=false modes.
  • The runtime export thrift.Q is gone; downstream code that imported it will break. Users who need to keep it can use the new native_promise=false generator flag and install q themselves.

Test plan

  • make style passes
  • npm run lint and npm run test-ts pass
  • Thrift compiler builds; generated code for a sample .thrift service is valid JS/TS under both native_promise defaults
  • native_promise=false output still works against a project that has q installed
  • CI green

🤖 Generated with Claude Code

Removes the runtime dependency on the `q` Promise library and switches
the JS/TS code generator to emit native Promise by default.

- Compiler (t_js_generator.cc): in non-ES6 mode the processor emits
  `new Promise((resolve) => resolve(handler.fn(args)))` instead of
  `Q.fcall(...)`, and the Node client emits `new Promise((resolve, reject)
  => ...)` instead of `Q.defer()` / `_defer.promise`. The non-ES6 JS/TS
  imports no longer pull in `Q` by default.
- New compiler flag `js:native_promise=[true|false]` (default `true`).
  Setting it to `false` restores the legacy `Q.fcall` / `Q.defer` output
  and emits `const Q = require('q');` / `import Q = require('q');`
  directly (no longer routed through `thrift.Q`), so legacy users only
  need `q` in their own `node_modules`.
- Runtime: removed `exports.Q = require("q")` from
  lib/nodejs/lib/thrift/index.js and browser.js, dropped `q` from
  dependencies and `@types/q` from devDependencies, and pruned the
  matching package-lock.json entries.
- Tests: lib/nodets/test/test_driver.ts and test_handler.ts now use
  native `Promise` / `.catch()` instead of `Q.IPromise` / `Q.resolve` /
  `.fail()`. Renamed the test case "Q Promise Client Tests" to
  "Promise Client Tests". `AsyncThriftTestHandler` return types changed
  to `Promise<void>` (the callback delivers the value and the body
  returns `Promise.resolve()`, which native Promise types strictly).

Client: js,nodejs

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimexist jimexist requested review from Jens-G and emmenlau as code owners May 25, 2026 00:28
@mergeable mergeable Bot added javascript Pull requests that update Javascript code nodejs typescript compiler labels May 25, 2026
Native Promise's `.catch(fn)` strictly expects a one-arg callback;
Q's `.fail(fn)` was loose about extra args, so `function (error, response)`
worked before. TS errors out with TS2345 — drop the unused `response` arg.

Client: nodets

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimexist jimexist changed the title Switch JS/Node generator and runtime from Q to native Promise THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise May 25, 2026
@jimexist jimexist requested a review from Copilot May 25, 2026 01:29
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the Node.js runtime dependency on the q Promise library and updates the JS/TS code generator to emit native Promise-based code by default, with an opt-out js:native_promise=false flag to restore legacy Q-shaped output.

Changes:

  • Dropped q (and @types/q) from npm dependencies and removed the thrift.Q runtime export.
  • Updated the JS/TS generator to default to native Promise code paths and added a native_promise generator option to toggle legacy Q output/imports.
  • Updated Node TS tests to use native Promise APIs (Promise.resolve, .catch) and removed Q typing/usages.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
compiler/cpp/src/thrift/generate/t_js_generator.cc Adds native_promise option and switches non-ES6 generated code paths from Q to native Promises by default.
lib/nodejs/lib/thrift/index.js Removes exports.Q from the Node runtime entrypoint.
lib/nodejs/lib/thrift/browser.js Removes exports.Q from the browser runtime entrypoint.
package.json Removes q from dependencies and @types/q from devDependencies.
package-lock.json Prunes q / @types/q entries from the lockfile.
lib/nodejs/test/package-lock.json Prunes q / @types/q entries from the test lockfile.
lib/nodets/test/test_handler.ts Migrates test handlers from Q promises to native Promise return types and helpers.
lib/nodets/test/test_driver.ts Migrates promise-client tests from .fail/Q patterns to .catch/native Promise patterns.
Files not reviewed (1)
  • lib/nodejs/test/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread compiler/cpp/src/thrift/generate/t_js_generator.cc Outdated
Comment thread lib/nodets/test/test_driver.ts Outdated
Comment thread lib/nodets/test/test_driver.ts
Comment thread lib/nodets/test/test_driver.ts
- t_js_generator.cc: stop emitting an arrow function inside the Promise
  constructor for non-ES6 native_promise mode. Use
  `new Promise(function(resolve) { resolve(...); }.bind(this))` so the
  output stays ES5-compatible and `this` is bound explicitly.
- test_driver.ts: pass the actual `fnName` variable to `.catch(fail(...))`
  in `makeAsserter` (was the literal string "fnName"), so failures report
  which RPC actually failed.
- test_driver.ts: in the `testException("TException")` / "Xception"
  unexpected-resolve branches, call `assert.fail(...)` directly instead
  of `fail(...)` which only returned a function and was never invoked.

Client: js,nodets

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler javascript Pull requests that update Javascript code nodejs typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants