Copilot commented on code in PR #3529:
URL: https://github.com/apache/thrift/pull/3529#discussion_r3295734257
##########
lib/nodejs/lib/thrift/index.js:
##########
@@ -58,6 +58,34 @@ exports.createWebServer = web_server.createWebServer;
exports.Int64 = require("node-int64");
exports.Q = require("q");
+/**
+ * Convert a `node-int64` Int64 (as returned by `TBinaryProtocol.readI64`,
+ * `TCompactProtocol.readI64`, etc.) to a native `bigint`. Used by code
+ * generated with `js:bigint=true` to surface int64 values as BigInt without
+ * a protocol-layer toggle.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return i64.buffer.readBigInt64BE(i64.offset || 0);
+};
+
+/**
+ * Convert a native `bigint` to a `node-int64` Int64 suitable for passing to
+ * `writeI64`. Values outside the signed 64-bit range are wrapped to fit
+ * (`BigInt.asIntN(64, ...)`).
+ *
+ * @param {bigint} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ buf.writeBigInt64BE(BigInt.asIntN(64, value), 0);
+ return new Int64(buf);
+};
Review Comment:
`toBigInt`/`fromBigInt` rely on
`Buffer#readBigInt64BE`/`Buffer#writeBigInt64BE`. Those APIs are not available
in older Node versions (including parts of the currently-declared support range
`>= 10.18.0`), which would cause a runtime TypeError when bigint-mode generated
code calls these helpers. Consider adding a feature-detected fallback (manual
byte/shift conversion) or bumping the minimum supported Node version to one
that guarantees these Buffer methods.
##########
lib/nodejs/lib/thrift/browser.js:
##########
@@ -38,6 +38,33 @@ exports.createClient = require("./create_client");
exports.Int64 = require("node-int64");
exports.Q = require("q");
+/**
+ * Convert a `node-int64` Int64 to a native `bigint`. Used by code generated
+ * with `js:bigint=true`. Browser builds need `Buffer#readBigInt64BE`, which
+ * is shimmed by webpack's buffer polyfill / available natively in modern
+ * runtimes.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return i64.buffer.readBigInt64BE(i64.offset || 0);
+};
+
+/**
+ * Convert a native `bigint` to a `node-int64` Int64 for `writeI64`. Values
+ * outside the signed 64-bit range are wrapped (`BigInt.asIntN(64, ...)`).
+ *
+ * @param {bigint} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ buf.writeBigInt64BE(BigInt.asIntN(64, value), 0);
+ return new Int64(buf);
+};
Review Comment:
Same issue as the node entrypoint: these helpers assume
`Buffer#readBigInt64BE`/`writeBigInt64BE` exist. In browser bundles (and in
older Node runtimes) Buffer polyfills may not implement these methods, so
calling `thrift.toBigInt`/`fromBigInt` can fail at runtime. Add a
feature-detected fallback implementation or gate the helpers behind
availability checks.
--
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]
