fix(1557): fix for big int serialization #1581
Conversation
- changed serialization of bigint - cleaned up tests and added new one
WalkthroughThe changes introduce a modification to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/errors/tests/helpers/helpers.unit.test.ts (1)
76-87: Consider adding more BigInt test cases.While the current test case verifies basic BigInt handling, consider adding tests for:
- Maximum safe integer boundary cases (Number.MAX_SAFE_INTEGER + 1n)
- Negative BigInt values
- Zero as BigInt (0n)
Example addition:
test('Should be able to build an error message - bigint', () => { + const testCases = [ + { input: 10n, expected: '10n' }, + { input: -10n, expected: '-10n' }, + { input: 0n, expected: '0n' }, + { input: BigInt(Number.MAX_SAFE_INTEGER) + 1n, expected: '9007199254740992n' } + ]; + + for (const { input, expected } of testCases) { const errorMessage = createErrorMessage( 'simpleMethod', 'Error message', { - data: 10n + data: input } ); expect(errorMessage).toBeDefined(); - expect(errorMessage).toContain('10n'); + expect(errorMessage).toContain(expected); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/errors/src/helpers/helpers.ts(1 hunks)packages/errors/tests/helpers/helpers.unit.test.ts(1 hunks)
🔇 Additional comments (2)
packages/errors/src/helpers/helpers.ts (1)
25-27: LGTM! Proper handling of BigInt serialization.
The implementation correctly handles BigInt serialization by converting to string with 'n' suffix, matching JavaScript's standard BigInt literal notation.
packages/errors/tests/helpers/helpers.unit.test.ts (1)
Line range hint 25-27: Verify BigInt handling across the codebase.
Let's ensure consistent BigInt handling across the codebase.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get more specific information about BigInt handling.
The first scan revealed extensive BigInt usage across the codebase, particularly in core packages and network-related functionality. However, we need to verify if there are proper error handling patterns and test coverage specifically for BigInt serialization, which these additional checks will help us determine.
Let me generate one final verification script to check the specific error handling implementation in the helpers file.
This will help us verify if the error handling implementation properly handles BigInt serialization, which is crucial for the final assessment.
BigInt handling is properly implemented across the codebase
The error handling implementation in packages/errors/src/helpers/helpers.ts already includes proper BigInt serialization by converting BigInt values to strings with an 'n' suffix during JSON stringification. This is the correct approach for handling BigInt values in error messages and logging.
Key findings:
- The
stringifyDatafunction includes specific handling for BigInt values - BigInt test coverage exists in the codebase
- The error handling implementation properly handles BigInt serialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BigInt usage in the codebase
echo "Searching for BigInt usage patterns..."
rg -l "bigint|BigInt" --type ts
# Search for potential JSON.stringify usage that might need BigInt handling
echo -e "\nChecking for JSON.stringify usage that might need BigInt handling..."
rg "JSON\.stringify" --type ts -C 2
# Look for potential test files that might need BigInt test coverage
echo -e "\nIdentifying test files that might need BigInt test coverage..."
fd ".*\.test\.ts$" | xargs rg "test\(" -l
Length of output: 29796
Script:
#!/bin/bash
# Check for BigInt-related error handling patterns
echo "Checking for BigInt error handling patterns..."
rg "BigInt.*error|error.*BigInt" --type ts -C 2
# Look for JSON.stringify with BigInt handling
echo -e "\nChecking for BigInt-aware JSON.stringify usage..."
rg "JSON\.stringify.*BigInt|BigInt.*JSON\.stringify" --type ts -C 2
# Check for specific BigInt test cases
echo -e "\nLooking for BigInt-specific test cases..."
rg "test.*BigInt|describe.*BigInt" --type ts -C 2
# Check for BigInt type definitions and interfaces
echo -e "\nChecking for BigInt type definitions..."
ast-grep --pattern 'type $_ = bigint'
Length of output: 2721
Script:
#!/bin/bash
# Check the error handling implementation in helpers.ts
echo "Checking error handling implementation in helpers.ts..."
rg -C 10 "stringify" packages/errors/src/helpers/helpers.ts
# Check for any BigInt-related error handling in the errors package
echo -e "\nChecking for BigInt handling in errors package..."
rg "bigint|BigInt" packages/errors/src/ -C 5
Length of output: 3534
Test CoverageSummary
|
Description
Fixes # (#1557 )
Type of change
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
bigintvalues in the stringification process.Bug Fixes
bigintdata types.Tests
bigintin error messages.