You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "jimexist (via GitHub)" <gi...@apache.org> on 2026/05/25 00:28:53 UTC
[PR] Switch JS/Node generator and runtime from Q to native Promise [thrift]
jimexist opened a new pull request, #3528:
URL: https://github.com/apache/thrift/pull/3528
## Summary
Removes the runtime dependency on the [`q`](https://github.com/kriskowal/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`](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/index.js), [`lib/nodejs/lib/thrift/browser.js`](lib/nodejs/lib/thrift/browser.js))
- Removed `exports.Q = require("q")`.
**Dependencies** ([`package.json`](package.json), [`package-lock.json`](package-lock.json), [`lib/nodejs/test/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_driver.ts), [`lib/nodets/test/test_handler.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`](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](https://issues.apache.org/jira/browse/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](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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist commented on code in PR #3528:
URL: https://github.com/apache/thrift/pull/3528#discussion_r3295711932
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
Review Comment:
Fixed in 7c3f37992: replaced `fail("testException: TException")` (which only built a callback and never invoked it) with `assert.fail(...)` directly in the unexpected-resolve branch.
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
- .fail(function (err) {
+ .catch(function (err) {
assert.ok(err instanceof TException);
});
- Q.resolve(client.testException("Xception"))
+ Promise.resolve(client.testException("Xception"))
.then(function (response) {
fail("testException: Xception");
})
Review Comment:
Fixed in 7c3f37992: replaced `fail("testException: Xception")` with `assert.fail(...)` directly so the test now actually fails if the promise unexpectedly resolves.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #3528: THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise
URL: https://github.com/apache/thrift/pull/3528
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist commented on code in PR #3528:
URL: https://github.com/apache/thrift/pull/3528#discussion_r3295711442
##########
compiler/cpp/src/thrift/generate/t_js_generator.cc:
##########
@@ -1570,7 +1590,7 @@ void t_js_generator::generate_process_function(t_service* tservice, t_function*
<< ".length === " << fields.size() << ") {" << '\n';
indent_up();
- if (gen_es6_) {
+ if (gen_es6_ || gen_native_promise_) {
indent(f_service_) << "new Promise((resolve) => resolve(this._handler." << tfunction->get_name() << ".bind(this._handler)(" << '\n';
} else {
Review Comment:
Fixed in 7c3f37992: non-ES6 + native_promise now emits `new Promise(function(resolve) { resolve(...); }.bind(this))` instead of an arrow function, keeping the output ES5-compatible while binding `this` explicitly.
##########
lib/nodets/test/test_driver.ts:
##########
@@ -149,7 +148,7 @@ export function ThriftTestDriverPromise(
.then(function (actual: any) {
assertionFn(actual, expected, fnName);
})
- .fail(fail("fnName"));
+ .catch(fail("fnName"));
Review Comment:
Fixed in 7c3f37992: changed `fail("fnName")` to `fail(fnName)` so the failure message reports the actual RPC name.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6040: Switch JS/Node generator and runtime from Q to native Promise [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3528:
URL: https://github.com/apache/thrift/pull/3528#discussion_r3295707462
##########
lib/nodets/test/test_driver.ts:
##########
@@ -149,7 +148,7 @@ export function ThriftTestDriverPromise(
.then(function (actual: any) {
assertionFn(actual, expected, fnName);
})
- .fail(fail("fnName"));
+ .catch(fail("fnName"));
Review Comment:
This `.catch` uses the literal string "fnName" instead of the actual function name variable, so failures won’t report which RPC failed.
##########
compiler/cpp/src/thrift/generate/t_js_generator.cc:
##########
@@ -1570,7 +1590,7 @@ void t_js_generator::generate_process_function(t_service* tservice, t_function*
<< ".length === " << fields.size() << ") {" << '\n';
indent_up();
- if (gen_es6_) {
+ if (gen_es6_ || gen_native_promise_) {
indent(f_service_) << "new Promise((resolve) => resolve(this._handler." << tfunction->get_name() << ".bind(this._handler)(" << '\n';
} else {
Review Comment:
The non-ES6 generator path (gen_es6_ == false) currently emits an arrow function inside the Promise constructor (`(resolve) => ...`). That’s ES6 syntax and breaks the intent of the non-ES6/ES5 output mode. If native Promises are enabled in non-ES6 mode, the emitted JS should use `function(resolve) { ... }` and avoid relying on arrow-function lexical `this` (capture `this` into a `self` variable or bind explicitly).
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
Review Comment:
These `.then` branches call `fail(...)` but never invoke the returned function, so the test will *not* fail if `testException(...)` unexpectedly resolves. Use `assert.fail(...)` directly in the success path.
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
- .fail(function (err) {
+ .catch(function (err) {
assert.ok(err instanceof TException);
});
- Q.resolve(client.testException("Xception"))
+ Promise.resolve(client.testException("Xception"))
.then(function (response) {
fail("testException: Xception");
})
Review Comment:
These `.then` branches call `fail(...)` but never invoke the returned function, so the test will *not* fail if `testException(...)` unexpectedly resolves. Use `assert.fail(...)` directly in the success path.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org