jimexist commented on code in PR #3529:
URL: https://github.com/apache/thrift/pull/3529#discussion_r3366041260
##########
lib/nodejs/lib/thrift/index.js:
##########
@@ -57,6 +57,50 @@ exports.createWebServer = web_server.createWebServer;
exports.Int64 = require("node-int64");
+const bigIntCompat = require("./bigint_compat");
+
+/**
+ * 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.
+ *
+ * Uses `Buffer#readBigInt64BE` when available (Node >= 12), and a
+ * `readInt32BE` / `readUInt32BE` + BigInt fallback otherwise — so the
+ * helpers work across the full `engines: >= 10.18.0` support range.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return bigIntCompat.readBigInt64BE(i64.buffer, 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, ...)`).
+ *
+ * Also accepts a decimal-string or `number` for callers that don't hold the
+ * value as a `bigint` — notably generated code serializing `map<i64, …>`,
+ * where the map key is iterated as the JS object-key string (`for (k in
obj)`).
+ * Strings are parsed via `BigInt(string)` (decimal); numbers must be in the
+ * safe integer range.
+ *
+ * Uses `Buffer#writeBigInt64BE` when available, with a 32-bit-pair fallback
+ * for older runtimes (see `bigint_compat.js`).
+ *
+ * @param {bigint | string | number} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ const big = typeof value === "bigint" ? value : BigInt(value);
+ bigIntCompat.writeBigInt64BE(buf, big, 0);
+ return new Int64(buf);
+};
Review Comment:
Fixed in abaf00950 — `fromBigInt` now rejects non-safe-integer `number`
inputs with a `RangeError`. `bigint` and decimal-string inputs still cover the
full signed 64-bit range. New tests in `bigint_helpers.test.js` cover the
rejection (MAX_SAFE_INTEGER+1, MIN_SAFE_INTEGER-1, 1.5, NaN) and confirm the
decimal-string path still works for the same magnitudes.
##########
lib/nodejs/README.md:
##########
@@ -65,6 +66,48 @@ client.get_slice("Keyspace", "key", new
ttypes.ColumnParent({column_family: "Exa
Since JavaScript represents all numbers as doubles, int64 values cannot be
accurately represented naturally. To solve this, int64 values in responses will
be wrapped with Thrift.Int64 objects. The Int64 implementation used is
[broofa/node-int64](https://github.com/broofa/node-int64).
Review Comment:
Reworked the Int64 section in abaf00950 to be explicit that both the
protocol layer and generated struct fields default to `Thrift.Int64`, with a
link to the new BigInt section. (Also relevant: the bigint flag is now opt-in /
default-off per your other comment, so default behavior matches the original
Int64 description.)
##########
lib/nodejs/lib/thrift/browser.js:
##########
@@ -37,6 +37,39 @@ exports.createClient = require("./create_client");
exports.Int64 = require("node-int64");
+const bigIntCompat = require("./bigint_compat");
+
+/**
+ * Convert a `node-int64` Int64 to a native `bigint`. Used by code generated
+ * with `js:bigint=true`. Feature-detects `Buffer#readBigInt64BE` (Node 12+
+ * and modern buffer polyfills) and falls back to a `readInt32BE` /
+ * `readUInt32BE` + BigInt composition when the native method is absent.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return bigIntCompat.readBigInt64BE(i64.buffer, 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, ...)`).
+ * Also accepts a decimal-string or `number` so generated `map<i64, …>`
+ * serialization works (map keys reach this function as object-key strings).
+ * Uses the same feature-detected fallback as `toBigInt`.
+ *
+ * @param {bigint | string | number} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ const big = typeof value === "bigint" ? value : BigInt(value);
+ bigIntCompat.writeBigInt64BE(buf, big, 0);
+ return new Int64(buf);
+};
Review Comment:
Same fix as for the index.js thread — `browser.js` `fromBigInt` also rejects
non-safe-integer numbers. Tests cover both entrypoints through the same export
path.
##########
compiler/cpp/src/thrift/generate/t_js_generator.cc:
##########
@@ -551,10 +578,14 @@ string t_js_generator::js_includes() {
}
}
if (gen_esm_) {
- result += "import Int64 from 'node-int64';\n";
+ if (!gen_bigint_) {
+ result += "import Int64 from 'node-int64';\n";
+ }
result += "import { v4 as uuid } from 'uuid';";
Review Comment:
Good catch — fixed in abaf00950. The ESM include path now emits `import * as
thrift from 'thrift'; const { Thrift } = thrift;` when `bigint` is on, so the
generated `thrift.toBigInt(...)` / `thrift.fromBigInt(...)` calls resolve
correctly under `--gen js:node,es6,esm,bigint`. The CJS path was already fine
(it already binds `thrift = require('thrift')`).
--
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]
