Skip to content

Conversation

@minggangw
Copy link
Member

@minggangw minggangw commented Oct 20, 2025

This PR removes the json-bigint dependency by implementing native JSON parsing with big integer support. The change consolidates JSON parsing logic into a reusable utility function that uses Node.js 21+ native capabilities when available, and falls back to regex preprocessing for older versions.

Key Changes:

  • Implemented parseJSONWithBigInt utility function to replace json-bigint package
  • Removed json-bigint dependency from package.json
  • Refactored rosidl_parser.js to use the new utility function

Fix: #1304

Copilot AI review requested due to automatic review settings October 20, 2025 06:49
Copy link

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 json-bigint dependency by implementing native JSON parsing with big integer support. The change consolidates JSON parsing logic into a reusable utility function that uses Node.js 21+ native capabilities when available, and falls back to regex preprocessing for older versions.

Key Changes:

  • Implemented parseJSONWithBigInt utility function to replace json-bigint package
  • Removed json-bigint dependency from package.json
  • Refactored rosidl_parser.js to use the new utility function

Reviewed Changes

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

File Description
lib/utils.js Added parseJSONWithBigInt function with comprehensive documentation to handle large integers in JSON parsing
rosidl_parser/rosidl_parser.js Removed local _parseJSONObject method and version checking logic, replaced with imported parseJSONWithBigInt utility
package.json Removed json-bigint dependency from dependencies list

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

lib/utils.js Outdated
Comment on lines 320 to 327
function parseJSONWithBigInt(str) {
const contextSupportedVersion = '21.0.0.0';
const currentVersion = process.version;
const isContextSupported = compareVersions(
currentVersion.substring(1, currentVersion.length),
contextSupportedVersion,
'>='
);
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

The version comparison is executed on every call to parseJSONWithBigInt. Consider caching isContextSupported as a module-level constant to avoid repeated comparisons, similar to how it was implemented in the original code.

Copilot uses AI. Check for mistakes.
Comment on lines 347 to 359
const processed = str.replace(
/:\s*(-?\d+)(?=\s*[,}\]])/g,
(match, number) => {
const num = Number(number);
// If the number is not safe, keep it as a string
if (!Number.isSafeInteger(num)) {
return `: "${number}"`;
}
return match;
}
);

return JSON.parse(processed);
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

The regex pattern may incorrectly match numbers in JSON string values. For example, in {\"key\": \"value: 123\"}, the number inside the string value could be mistakenly converted. The pattern should account for proper JSON structure to avoid matching numbers within string literals.

Suggested change
const processed = str.replace(
/:\s*(-?\d+)(?=\s*[,}\]])/g,
(match, number) => {
const num = Number(number);
// If the number is not safe, keep it as a string
if (!Number.isSafeInteger(num)) {
return `: "${number}"`;
}
return match;
}
);
return JSON.parse(processed);
// Use a reviver to convert unsafe integers to strings
return JSON.parse(str, (key, value) => {
if (typeof value === 'number' && Number.isInteger(value) && !Number.isSafeInteger(value)) {
return value.toString();
}
return value;
});

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Oct 20, 2025

Coverage Status

coverage: 82.208% (-0.4%) from 82.578%
when pulling 6412a0b on minggangw:fix-1304
into c8f0ae6 on RobotWebTools:develop.

@minggangw minggangw requested a review from Copilot October 22, 2025 07:43
Copy link

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

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 359 to 368
/(?:[:,[])\s*(-?\d+)(?=\s*[,}\]])/g,
(match, number) => {
const num = Number(number);
// If the number is not safe, convert it to a string
if (!Number.isSafeInteger(num)) {
// Preserve the prefix character (: or , or [)
const prefix = match.charAt(0);
return `${prefix} "${number}"`;
}
return match;
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

The regex pattern requires a preceding character (:, ,, or [) which means it won't match integers at the start of the JSON string or after an opening brace {. For example, {\"value\": 9007199254740992} would match, but [9007199254740992] or {9007199254740992} (if this were valid JSON) would not match the integer at the start. Consider using a lookbehind assertion or adjust the logic to handle integers that appear immediately after {.

Suggested change
/(?:[:,[])\s*(-?\d+)(?=\s*[,}\]])/g,
(match, number) => {
const num = Number(number);
// If the number is not safe, convert it to a string
if (!Number.isSafeInteger(num)) {
// Preserve the prefix character (: or , or [)
const prefix = match.charAt(0);
return `${prefix} "${number}"`;
}
return match;
/(?<=^|[:,\[{])\s*(-?\d+)(?=\s*[,}\]])/g,
(numberMatch, number, offset, string) => {
const num = Number(number);
// If the number is not safe, convert it to a string
if (!Number.isSafeInteger(num)) {
// Find the prefix character (if any)
const prefixMatch = string.slice(Math.max(0, offset - 1), offset);
const prefix = /[:,\[{]/.test(prefixMatch) ? prefixMatch : '';
return `${prefix} "${number}"`;
}
return numberMatch;

Copilot uses AI. Check for mistakes.
lib/utils.js Outdated
Comment on lines 361 to 363
const num = Number(number);
// If the number is not safe, convert it to a string
if (!Number.isSafeInteger(num)) {
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

Converting the string to Number before checking if it's safe defeats the purpose of the check. For integers beyond Number.MAX_SAFE_INTEGER, Number(number) will lose precision during conversion (e.g., Number('9007199254740993') becomes 9007199254740992), making the subsequent safety check unreliable. Instead, compare the absolute value of the integer string against Number.MAX_SAFE_INTEGER without conversion, or use BigInt for comparison.

Suggested change
const num = Number(number);
// If the number is not safe, convert it to a string
if (!Number.isSafeInteger(num)) {
// Use BigInt to check if the integer is outside the safe range
const absBigInt = BigInt(number.startsWith('-') ? number.slice(1) : number);
if (absBigInt > BigInt(Number.MAX_SAFE_INTEGER)) {

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove json-bigint dependency

2 participants