Update target to ES2020 and replace Long with bigint #912
Conversation
WalkthroughThe pull request introduces significant modifications across various files in the SDK, primarily focusing on the conversion functions in Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (19)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
🧰 Additional context used🔇 Additional comments (9)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
packages/sdk/src/document/change/checkpoint.ts (2)
92-92: Consider using strict equality and LGTM for InitialCheckpointIn the
equalsmethod:
- Consider using
===instead of==for comparingbigintvalues. While==works,===is preferred for strict equality checks in TypeScript/JavaScript.The
InitialCheckpointconstant initialization with0nis correct for abigintliteral.Suggested change for line 92:
- this.clientSeq === other.clientSeq && this.serverSeq == other.serverSeq + this.clientSeq === other.clientSeq && this.serverSeq === other.serverSeqAlso applies to: 108-108
Migration Incomplete: Remaining
LongUsages FoundThe
Checkpointclass has been successfully migrated fromLongtobigint. However, the following files still import and useLong:
packages/sdk/test/unit/document/crdt/counter_test.tspackages/sdk/test/unit/document/crdt/primitive_test.tspackages/sdk/test/integration/primitive_test.tspackages/sdk/test/integration/counter_test.tspackages/sdk/src/document/crdt/primitive.tspackages/sdk/src/document/crdt/counter.tspackages/sdk/src/document/operation/increase_operation.tspackages/sdk/src/document/json/counter.tsAction Items:
- Remove or Replace
LongImports: Ensure that allLongimports are either removed or replaced withbigintin the listed files.- Update Type Usages: Replace all instances of
Longtypes withbigintto maintain consistency.- Review Test Cases: Update test cases to reflect the changes from
Longtobigint.This complete migration is necessary to prevent type mismatches and enhance code maintainability.
🔗 Analysis chain
Line range hint
1-108: Overall assessment: Consistent migration from Long to bigintThe changes in this file successfully migrate the
Checkpointclass from usingLongtobigint. This includes updates to:
- Property types
- Constructor and method signatures
- Comparison logic
- Equality checks
- Constant initialization
These changes align with the PR objectives and improve the code by using native JavaScript
biginttype. This should lead to better performance and easier maintenance.To ensure the changes are applied consistently across the codebase, it's important to verify that all usages of this class have been updated accordingly.
To verify the consistent usage of
bigintinstead ofLongforCheckpoint:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of Long type with Checkpoint # Test 1: Search for Long type usage with Checkpoint echo "Searching for Long type usage with Checkpoint:" rg -i 'checkpoint.*long' --type ts # Test 2: Verify bigint usage with Checkpoint echo "Verifying bigint usage with Checkpoint:" rg -i 'checkpoint.*bigint' --type ts # Test 3: Check for any remaining Long imports echo "Checking for any remaining Long imports:" rg -i "import.*long.*from" --type tsLength of output: 985
Script:
#!/bin/bash # Description: Comprehensive search for bigint usage with Checkpoint # Test 1: Search for bigint type usage in Checkpoint class echo "Searching for 'bigint' type usage within the Checkpoint class:" rg -i 'Checkpoint.*bigint' --type ts # Test 2: Search for bigint literals or operations related to Checkpoint echo "Searching for bigint literals or operations within the Checkpoint class:" rg -i 'Checkpoint.*\b0n\b' --type ts # Test 3: Verify all method parameters and returns use bigint where appropriate echo "Verifying method signatures in Checkpoint use bigint:" rg -i 'Checkpoint.*\(.*bigint.*\)' --type tsLength of output: 640
Script:
#!/bin/bash # Description: Enhanced search for bigint usage within the Checkpoint class and related contexts # Test 1: Search for 'bigint' declarations within checkpoint.ts echo "Searching for 'bigint' declarations in checkpoint.ts:" rg -i 'bigint' packages/sdk/src/document/change/checkpoint.ts # Test 2: Search for 'bigint' in constructor parameters within Checkpoint class echo "Searching for 'bigint' in Checkpoint constructor parameters:" rg -i 'constructor\(.*bigint.*\)' packages/sdk/src/document/change/checkpoint.ts # Test 3: Search for 'bigint' in method return types within Checkpoint class echo "Searching for 'bigint' return types in Checkpoint methods:" rg -i 'public .*(): bigint' packages/sdk/src/document/change/checkpoint.ts # Test 4: Ensure no 'Long' types remain in checkpoint.ts echo "Ensuring no 'Long' types are present in checkpoint.ts:" rg -i 'Long' packages/sdk/src/document/change/checkpoint.tsLength of output: 1146
Script:
#!/bin/bash # Description: Search for any remaining 'Long' imports and usages across the codebase # Test 1: Search for 'Long' imports in all TypeScript files echo "Searching for 'Long' imports across the codebase:" rg -i "import.*Long.*from" --type ts # Test 2: Search for any usage of 'Long' type outside of imports echo "Searching for 'Long' type usages outside of imports:" rg -i '\bLong\b' --type ts # Test 3: Verify if 'bigint' is used consistently in places where 'Long' was previously used echo "Verifying 'bigint' usage in place of 'Long':" rg -i '\bbigint\b' --type tsLength of output: 9097
packages/sdk/src/api/yorkie/v1/resources.proto (1)
Line range hint
1-380: Summary of changes and suggestions for next stepsThe changes in this file align well with the PR objectives:
- Removal of
jstype = JS_STRINGoptions improves type consistency and supports the transition from Long to bigint.- Addition of the
ArraySetoperation expands the SDK's functionality.Suggestions for next steps:
- Provide documentation or comments explaining the use case and behavior of the new
ArraySetoperation.- Ensure that corresponding changes have been made in the TypeScript implementation to handle the new bigint types correctly.
- Update any relevant tests to cover the new
ArraySetoperation and the changed field types.- Consider updating the SDK's documentation to reflect these protocol changes, especially regarding the new operation type and the change from Long to bigint.
packages/sdk/src/document/document.ts (1)
1384-1385: LGTM! Consider updating the method documentation.The change from
Longtobigintfor theserverSeqparameter is correct and aligns with the PR objectives. This improves type safety by using a native JavaScript type.Consider updating the method's JSDoc comment to reflect the new
biginttype forserverSeq.packages/sdk/test/integration/object_test.ts (1)
Line range hint
735-788: Duplicated test function: 'Should not propagate changes when there is no applied undo operation'It appears that the test function
Should not propagate changes when there is no applied undo operationis duplicated starting at line 735. There are two identical test cases with the same name and code. This duplication may lead to confusion and unnecessary redundancy.Apply the following diff to remove the duplicated test:
@@ -735,54 +735,0 @@ -it(`Should not propagate changes when there is no applied undo operation`, async function ({ - task, -}) { - interface TestDoc { - shape?: { color: string }; - } - const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); - const doc1 = new Document<TestDoc>(docKey); - const doc2 = new Document<TestDoc>(docKey); - - const client1 = new Client(testRPCAddr); - const client2 = new Client(testRPCAddr); - await client1.activate(); - await client2.activate(); - - await client1.attach(doc1, { syncMode: SyncMode.Manual }); - let doc1ChangeID = doc1.getChangeID(); - let doc1Checkpoint = doc1.getCheckpoint(); - assert.equal(doc1ChangeID.getClientSeq(), 1); - assert.equal(doc1Checkpoint.getClientSeq(), 1); - assert.equal(doc1Checkpoint.getServerSeq(), 1n); - - doc1.update((root) => { - root.shape = { color: 'black' }; - }, 'init doc'); - await client1.sync(); - assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"black"}}'); - doc1ChangeID = doc1.getChangeID(); - doc1Checkpoint = doc1.getCheckpoint(); - assert.equal(doc1ChangeID.getClientSeq(), 2); - assert.equal(doc1Checkpoint.getClientSeq(), 2); - assert.equal(doc1Checkpoint.getServerSeq(), 2n); - - await client2.attach(doc2, { syncMode: SyncMode.Manual }); - assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"black"}}'); - - doc2.update((root) => { - delete root.shape; - }, 'delete shape'); - await client2.sync(); - await client1.sync(); - assert.equal(doc1.toSortedJSON(), '{}'); - assert.equal(doc2.toSortedJSON(), '{}'); - doc1ChangeID = doc1.getChangeID(); - doc1Checkpoint = doc1.getCheckpoint(); - assert.equal(doc1ChangeID.getClientSeq(), 2); - assert.equal(doc1Checkpoint.getClientSeq(), 2); - assert.equal(doc1Checkpoint.getServerSeq(), 4n); - - // c2 deleted the shape, so the reverse operation cannot be applied - doc1.history.undo(); - assert.equal(doc1.toSortedJSON(), '{}'); - assert.equal(doc1.getRedoStackForTest().length, 0); - assert.equal(doc1.history.canRedo(), false); -});packages/sdk/src/api/yorkie/v1/resources_pb.ts (1)
2578-2589: Ensure Correct Handling of Actor IDs asUint8ArrayThe
TimeTicketclass usesUint8ArrayforactorId. While this is acceptable, ensure that the rest of the application correctly handlesUint8Arrayfor IDs and that there are utility methods for converting to and from string representations if necessary.Consider adding helper functions to serialize and deserialize
actorIdfor ease of use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- packages/sdk/src/api/converter.ts (6 hunks)
- packages/sdk/src/api/yorkie/v1/resources.proto (5 hunks)
- packages/sdk/src/api/yorkie/v1/resources_pb.ts (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie.proto (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie_connect.ts (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie_pb.ts (1 hunks)
- packages/sdk/src/document/change/change_id.ts (5 hunks)
- packages/sdk/src/document/change/checkpoint.ts (5 hunks)
- packages/sdk/src/document/document.ts (2 hunks)
- packages/sdk/src/document/time/ticket.ts (6 hunks)
- packages/sdk/test/integration/client_test.ts (3 hunks)
- packages/sdk/test/integration/object_test.ts (10 hunks)
- packages/sdk/tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/src/api/yorkie/v1/yorkie.proto
🧰 Additional context used
🔇 Additional comments (48)
packages/sdk/tsconfig.json (3)
3-3: LGTM: Target update aligns with PR objectives.Updating the
targetto "ES2020" is in line with the PR's goal of updating the target JavaScript version. This change enables the use of ES2020 features, including BigInt, which supports the transition from Long to bigint types.
6-6: LGTM: Library update consistent with target change.Updating the
libarray to include "ES2020" is consistent with the target update and ensures that TypeScript recognizes ES2020 library features during compilation. Retaining "dom" maintains support for DOM types.
3-6: Verify compatibility with project dependencies and target environments.The updates to both
targetandlibare appropriate for the PR objectives and should enable the use of ES2020 features, including BigInt. However, it's important to ensure these changes are compatible with the project's dependencies and target environments.Please run the following script to check for any potential compatibility issues:
Review the output of this script to ensure there are no compatibility concerns with the move to ES2020.
✅ Verification successful
Compatibility Verified with Project Dependencies and Target Environments
The updates to
targetandlibintsconfig.jsonhave been verified and no compatibility issues were found with project dependencies or target environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues with ES2020 # Test: Check if any dependencies specify ES2019 or lower as their target echo "Checking package.json for dependencies with potential compatibility issues:" jq '.dependencies + .devDependencies' package.json | grep -i "es201[0-9]" # Test: Look for any explicit ES2019 or lower usage in the codebase echo "Checking for explicit ES2019 or lower usage in the codebase:" rg -i "es201[0-9]" --type ts --type js # Test: Check if there are any browser targets specified that might not support ES2020 echo "Checking for browser targets in browserslist config (if exists):" if [ -f ".browserslistrc" ]; then cat .browserslistrc elif [ -f "package.json" ]; then jq '.browserslist' package.json fiLength of output: 639
packages/sdk/src/document/change/checkpoint.ts (5)
23-23: LGTM: Property type updated to bigintThe change from
Longtobigintfor theserverSeqproperty is consistent with the PR objectives and aligns with similar changes across the codebase.
26-26: LGTM: Constructor signature updatedThe constructor parameter type change for
serverSeqfromLongtobigintis consistent with the property type change and ensures type safety.
34-34: LGTM: Static method signature updatedThe static
ofmethod parameter type change forserverSeqfromLongtobigintis consistent with the constructor and property type changes, maintaining type consistency across the class.
58-59: LGTM: Comparison logic updated for bigintThe comparison logic in the
forwardmethod has been correctly updated to use the>operator, which is appropriate forbigintvalues. This change maintains the original logic while adapting to the new data type.
82-82: LGTM: Method return type updatedThe
getServerSeqmethod return type change fromLongtobigintis consistent with the property type change, ensuring type consistency throughout the class.packages/sdk/src/document/change/change_id.ts (8)
30-32: LGTM: Type changes align with PR objectivesThe changes to
serverSeqandlamportproperties fromLongtobigintare consistent with the PR objectives and the changes in other files. This update improves type consistency and leverages native JavaScript features.
37-39: LGTM: Constructor parameter types updated correctlyThe constructor parameters for
lamportandserverSeqhave been correctly updated tobigint, maintaining consistency with the property type changes. This ensures type safety throughout the class.
52-54: LGTM: Static method parameter types updated correctlyThe static
ofmethod parameters forlamportandserverSeqhave been correctly updated tobigint, maintaining consistency with the constructor parameter changes. This ensures type safety and proper object creation.
63-63: LGTM: Correct bigint operation in next methodThe
nextmethod has been updated to use the correctbigintaddition (+ 1n) instead of the previousLong.add(1). This change is consistent with the transition tobigintand uses the correct literal notation forbigintvalues.
71-72: LGTM: syncLamport method correctly updated for bigintThe
syncLamportmethod has been properly updated to work withbigint:
- The parameter type is now
bigint.- The comparison uses the correct
>operator forbigintvalues.- The
bigintaddition (+ 1n) is used correctly.These changes ensure that the Lamport clock synchronization works correctly with the new
biginttype.Also applies to: 76-76
113-113: LGTM: getLamport return type updated correctlyThe
getLamportmethod's return type has been correctly updated tobigint, maintaining consistency with thelamportproperty type change. This ensures type safety for consumers of this method.
145-145: LGTM: InitialChangeID correctly initialized with bigintThe
InitialChangeIDconstant has been correctly updated to use thebigintliteral0ninstead ofLong.fromInt(0, true). This change is consistent with the transition tobigintand ensures that the initial change ID is properly typed.
Line range hint
1-145: Overall assessment: Successful transition to bigintThe changes in this file successfully implement the transition from
Longtobigintas per the PR objectives. All relevant properties, method parameters, and return types have been updated consistently. The use ofbigintliterals (e.g.,0n,1n) is correct throughout the file.These changes offer several benefits:
- Improved type consistency by using native JavaScript
biginttype.- Simplified code by removing the dependency on the
longlibrary.- Potentially improved performance by using native
bigintoperations.The modifications maintain the existing functionality while enhancing the code's clarity and efficiency. Great job on implementing these changes consistently throughout the file!
packages/sdk/src/api/yorkie/v1/resources.proto (4)
60-61: LGTM: Improved type consistency forChangeIDfieldsThe removal of
jstype = JS_STRINGfromserver_seqandlamportfields in theChangeIDmessage aligns with the PR objective of replacing Long with bigint. This change improves type consistency and potentially enhances performance by using native numeric types in JavaScript.
139-144: LGTM: NewArraySetoperation addedThe addition of the
ArraySetmessage to theOperationmessage expands the SDK's functionality, likely allowing for direct manipulation of array elements. The structure is consistent with other operation types.Could you provide some context on the specific use cases for this new operation type and how it differs from existing array manipulation operations?
157-157: LGTM:ArraySetadded toOperationtypesThe
ArraySetoperation has been correctly added to thebodyoneof field of theOperationmessage. This addition is consistent with the introduction of the new operation type and follows the correct numbering sequence.
335-335: LGTM: Consistent type updates inCheckpointandTimeTicketThe removal of
jstype = JS_STRINGfrom theserver_seqfield inCheckpointand thelamportfield inTimeTicketis consistent with earlier changes and aligns with the PR objective of replacing Long with bigint. This improves type consistency across the protocol.To ensure these changes are reflected correctly in the SDK implementation:
Also applies to: 346-346
packages/sdk/src/api/converter.ts (6)
151-153: LGTM: SimplifiedtoCheckpointfunctionThe change to directly use
serverSeqwithout conversion aligns with the PR objective of replacing Long with bigint. This simplification potentially improves performance by avoiding unnecessary conversions.
162-164: LGTM: SimplifiedtoChangeIDfunctionThe direct use of
lamportwithout conversion is consistent with the PR objective and the previous changes. This modification maintains consistency across the codebase and potentially improves performance.
176-178: LGTM: SimplifiedtoTimeTicketfunctionThe modification to directly use
lamportwithout conversion is in line with the PR objective and the previous changes. This consistent approach simplifies the code and potentially improves performance across the codebase.
809-811: LGTM: Completed transition from Long to bigintThe modification in
fromChangeIDto directly uselamportandserverSeqwithout conversion completes the transition from Long to bigint. This change, along with the previous modifications, successfully implements the PR objective across both directions of conversion (to and from Protobuf format).These changes collectively:
- Simplify the code by removing unnecessary conversions
- Potentially improve performance by using native bigint types
- Maintain consistency across the codebase
Great job on implementing this transition smoothly and consistently!
1311-1311: LGTM: SimplifiedfromCheckpointfunctionThe modification in
fromCheckpointto directly useserverSeqwithout conversion is consistent with the previous changes and completes the transition from Long to bigint for Checkpoint conversion. This change maintains the consistency of the approach throughout the file.
Line range hint
1-1585: Overall assessment: Successful implementation of ES2020 target and Long to bigint transitionThis PR successfully achieves its objective of updating the target to ES2020 and replacing Long with bigint in the
converter.tsfile. The changes are consistent across all affected functions (toCheckpoint,toChangeID,toTimeTicket,fromChangeID, andfromCheckpoint).Key improvements:
- Simplified code by removing unnecessary conversions
- Potential performance improvements by using native bigint types
- Consistent implementation across both directions of conversion (to and from Protobuf format)
- Maintained code clarity and readability
The changes align well with modern JavaScript practices and should result in more efficient and maintainable code. Great job on this update!
packages/sdk/src/document/document.ts (1)
1656-1657: LGTM! Correct conversion to bigint.The update to use
BigInt(serverSeq)is correct and consistent with the changes made to theapplySnapshotmethod signature. This ensures that theserverSeqis properly converted from a string to abigintbefore being passed to the method.packages/sdk/src/api/yorkie/v1/yorkie_connect.ts (1)
1-106: LGTM!The
YorkieServiceand its methods are correctly defined according to the Protobuf service specifications. The generated code aligns with the project's architecture and coding standards.packages/sdk/src/document/time/ticket.ts (7)
48-48: Ensurebigintcompatibility across target environmentsThe use of
bigintrequires environments that support ES2020 or newer. Please verify that all target environments where this SDK is used are compatible withbigint. If not, consider providing a fallback or polyfill.
52-54: Update constructor to acceptlamportasbigintThe constructor now accepts
lamportas abigint, aligning with the updated property type. Ensure that all instantiations ofTimeTicketare updated accordingly.
62-65: Updateofmethod to acceptlamportasbigintThe static
ofmethod now acceptslamportas abigint. Ensure that any calls to this method pass abigintvalue forlamport.
125-127: UpdategetLamportmethod to returnbigintThe
getLamportmethod now returns abigint, consistent with the updatedlamporttype.
163-167: Modify comparison logic to usebigintoperatorsIn the
comparemethod, the comparison oflamportvalues now uses standard comparison operators (>,<) suitable forbiginttypes.
186-186: DefineMaxLamportas abigintliteral
MaxLamportis now defined using thebigintliteral9223372036854775807n, representing the maximumInt64value.
189-191: InitializeInitialTimeTicketwithbigintliteralIn
InitialTimeTicket, thelamportis now initialized with0n, abigintliteral.packages/sdk/src/api/yorkie/v1/yorkie_pb.ts (1)
23-23: Ensure correct import paths and exportsVerify that
ChangePackandDocEventare correctly imported from"./resources_pb.js". Incorrect import paths or missing exports can lead to runtime errors.Run the following script to confirm that
ChangePackandDocEventare exported fromresources_pb.js:packages/sdk/test/integration/object_test.ts (10)
659-659: Appropriate use of BigInt literal in assertionThe assertion at line 659 correctly compares
doc1Checkpoint.getServerSeq()with1n. SincegetServerSeq()returns abigint, using the1nBigInt literal ensures accurate comparison.
670-670: Consistent use of BigInt literals in assertionsAt line 670, comparing
doc1Checkpoint.getServerSeq()with2nmaintains consistency withbiginttypes across assertions.
686-686: Consistent use of BigInt literals in assertionsThe assertion at line 686 appropriately uses
4nfor comparingbigintvalues.
701-701: Consistent use of BigInt literals in assertionsAt line 701, the comparison with
4ncontinues the correct usage ofbigintliterals in assertions.
712-712: Consistent use of BigInt literals in assertionsLine 712 correctly compares
doc1Checkpoint.getServerSeq()with5n, ensuring accuratebigintcomparisons.
735-735: Consistent use of BigInt literals in assertionsAt line 735, the assertion uses
1n, which is appropriate forbigintcomparisons.
746-746: Consistent use of BigInt literals in assertionsLine 746 correctly compares with
2n, maintaining consistency.
762-762: Consistent use of BigInt literals in assertionsAt line 762, the use of
4nin the assertion is appropriate.
777-777: Consistent use of BigInt literals in assertionsLine 777 continues the correct pattern by comparing with
4n.
788-788: Consistent use of BigInt literals in assertionsAt line 788, comparing with
5nis appropriate forbiginttypes.packages/sdk/src/api/yorkie/v1/resources_pb.ts (2)
2667-2710: Incomplete Implementation ofDocEventHandlingThe
DocEventclass defines events likeDOCUMENT_CHANGED,DOCUMENT_WATCHED, etc., but there is no implementation detail or methods associated with handling these events. Ensure that event handling logic is implemented elsewhere and that this class integrates properly with it.Please confirm that the event handling for
DocEventis properly implemented in other parts of the codebase.You can run the following script to search for usages of
DocEvent:#!/bin/bash # Description: Search for implementations or usages of DocEvent. # Find all references to DocEvent in the codebase rg 'DocEvent' packages/sdk/src/
1159-1195: Clarify the Use ofUint8Arrayfor ValuesIn the
JSONElementSimpleclass, thevaluefield is aUint8Array, which may not be suitable for representing all value types, especially when usingbigint. This could lead to serialization/deserialization issues.Please confirm that using
Uint8Arrayis appropriate for allValueTypecases, especially with the shift tobigint. Consider whether a different representation is needed.You can run the following script to check for usages of
Uint8Arrayin value fields:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/sdk/test/integration/object_test.ts (1)
Line range hint
659-789: Overall assessment: Successful transition to BigInt with a minor issueThe changes in this file successfully implement the transition from Long to bigint, which aligns with the PR objective and the move to ES2020. The use of BigInt literals (e.g.,
1n,2n,4n,5n) is consistent throughout the file and correctly implements the required changes.However, there is a duplicate test case starting from line 735 that should be addressed to maintain a clean and efficient test suite.
Action items:
- Remove the duplicate test case (either the one starting at line 659 or the one at line 735).
- After removing the duplicate, please verify that all unique test scenarios are still covered.
packages/sdk/src/api/converter.ts (5)
809-809: LGTM with suggestion: BigInt conversion forlamportThe conversion of
lamportto BigInt aligns with the PR objective. However, consider adding a safety check to ensure the input is valid for BigInt conversion.BigInt(Number.isSafeInteger(pbChangeID.lamport) ? pbChangeID.lamport : 0)This prevents potential runtime errors if the input is not a valid integer.
811-811: LGTM with suggestion: BigInt conversion forserverSeqThe conversion of
serverSeqto BigInt aligns with the PR objective. However, consider adding a safety check to ensure the input is valid for BigInt conversion, similar to thelamportconversion:BigInt(Number.isSafeInteger(pbChangeID.serverSeq) ? pbChangeID.serverSeq : 0)This prevents potential runtime errors if the input is not a valid integer.
824-824: LGTM with suggestion: BigInt conversion forlamportinfromTimeTicketThe conversion of
lamportto BigInt in thefromTimeTicketfunction aligns with the PR objective. However, consider adding a safety check to ensure the input is valid for BigInt conversion:BigInt(Number.isSafeInteger(pbTimeTicket.lamport) ? pbTimeTicket.lamport : 0)This prevents potential runtime errors if the input is not a valid integer and maintains consistency with other BigInt conversions in this file.
1311-1311: LGTM with suggestion: BigInt conversion forserverSeqinfromCheckpointThe conversion of
serverSeqto BigInt in thefromCheckpointfunction aligns with the PR objective. However, consider adding a safety check to ensure the input is valid for BigInt conversion:BigInt(Number.isSafeInteger(pbCheckpoint.serverSeq) ? pbCheckpoint.serverSeq : 0)This prevents potential runtime errors if the input is not a valid integer and maintains consistency with other BigInt conversions in this file.
Line range hint
1-1511: Overall assessment: Consistent BigInt implementation with room for improvementThe changes in this file consistently implement the replacement of Long with BigInt, aligning well with the PR objectives. The code is simplified and more consistent. However, there's a recurring need for input validation in BigInt conversions.
Consider implementing a utility function for safe BigInt conversion:
function safeBigInt(value: number | string): bigint { return BigInt(Number.isSafeInteger(Number(value)) ? value : 0); }Then use this function throughout the file for all BigInt conversions. This would improve code safety and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- packages/sdk/src/api/converter.ts (6 hunks)
- packages/sdk/src/api/yorkie/v1/resources.proto (5 hunks)
- packages/sdk/src/api/yorkie/v1/resources_pb.ts (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie.proto (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie_connect.ts (1 hunks)
- packages/sdk/src/api/yorkie/v1/yorkie_pb.ts (1 hunks)
- packages/sdk/src/document/change/change_id.ts (5 hunks)
- packages/sdk/src/document/change/checkpoint.ts (5 hunks)
- packages/sdk/src/document/document.ts (2 hunks)
- packages/sdk/src/document/time/ticket.ts (6 hunks)
- packages/sdk/test/integration/client_test.ts (3 hunks)
- packages/sdk/test/integration/object_test.ts (10 hunks)
- packages/sdk/tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/src/api/yorkie/v1/yorkie_pb.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/sdk/src/api/yorkie/v1/resources.proto
- packages/sdk/src/api/yorkie/v1/resources_pb.ts
- packages/sdk/src/api/yorkie/v1/yorkie.proto
- packages/sdk/src/api/yorkie/v1/yorkie_connect.ts
- packages/sdk/src/document/change/change_id.ts
- packages/sdk/src/document/change/checkpoint.ts
- packages/sdk/src/document/document.ts
- packages/sdk/src/document/time/ticket.ts
- packages/sdk/tsconfig.json
🧰 Additional context used
🔇 Additional comments (12)
packages/sdk/test/integration/object_test.ts (5)
659-660: LGTM: Correct use of BigInt literalThe change from Long to bigint is correctly implemented here, using the BigInt literal
1n. This aligns with the PR objective and the move to ES2020.
670-671: LGTM: Consistent use of BigInt literalThe change to use the BigInt literal
2nis consistent with the previous change and correctly implements the transition from Long to bigint.
686-687: LGTM: Consistent BigInt usageThe use of BigInt literal
4nfor asserting theserverSeqis consistent with the previous changes and correctly implements the transition to bigint.
701-702: LGTM: Consistent BigInt usage in assertionsThe assertion using BigInt literal
4nforserverSeqis consistent with the previous changes and correctly implements the transition to bigint.
712-713: LGTM: Consistent BigInt usage maintainedThe assertion using BigInt literal
5nforserverSeqmaintains consistency with the previous changes and correctly implements the transition to bigint.packages/sdk/test/integration/client_test.ts (4)
530-530: Useassert.strictEqualfor BigInt comparisonsThis comment is identical to the previous one and applies here as well.
547-547: Useassert.strictEqualfor BigInt comparisonsThis comment is identical to the first one and applies to both of these lines as well.
Also applies to: 564-564
Line range hint
525-564: Overall, the changes look good and align with the PR objectivesThe updates to use BigInt literals in the test assertions are consistent with the goal of replacing Long with BigInt. The tests cover various scenarios and seem to be comprehensive. The only suggestion is to use
assert.strictEqualfor more precise BigInt comparisons, as mentioned in the previous comments.
525-525: 🛠️ Refactor suggestionUse
assert.strictEqualfor BigInt comparisonsFor more precise BigInt comparisons, consider using
assert.strictEqualinstead ofassert.equal. This ensures both type and value equality.- assert.equal(checkpoint.getServerSeq(), 1n); + assert.strictEqual(checkpoint.getServerSeq(), 1n);packages/sdk/src/api/converter.ts (3)
151-151: LGTM: SimplifiedserverSeqassignmentThe direct assignment of
serverSeqfromcheckpoint.getServerSeq()aligns with the PR objective of replacing Long with bigint. This change simplifies the code by removing unnecessary type conversion.
162-162: LGTM: SimplifiedlamportassignmentThe direct assignment of
lamportfromchangeID.getLamport()is consistent with the PR objective of replacing Long with bigint. This change simplifies the code by removing unnecessary type conversion.
176-176: LGTM: Simplifiedlamportassignment intoTimeTicketThe direct assignment of
lamportfromticket.getLamport()in thetoTimeTicketfunction is consistent with the PR objective of replacing Long with bigint. This change simplifies the code by removing unnecessary type conversion.
hackerwins
force-pushed
the
bigint
branch
2 times, most recently
from
e1f94f1
to
a8c8a0a
Compare
This commit Streamlines data type handling by replacing Long with native. It also updates target to ECMAScript 2020. Despite the introduction of bigint, but Long still remains in Counter. We need to remove Long.
What this PR does / why we need it?
Update target to ES2020 and replace Long with bigint
Any background context you want to provide?
What are the relevant tickets?
Fixes yorkie-team/yorkie#1023
Checklist
Summary by CodeRabbit
Release Notes
New Features
ArraySetin the protocol.Improvements
Longwith nativebigintin various classes.Bug Fixes
Tests
Documentclass to validate nested updates, undo/redo functionality, and concurrent operations.