Remove jstype=string from resources.proto
#1040
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
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: 0
🧹 Outside diff range and nitpick comments (1)
api/yorkie/v1/resources.proto (1)
60-61: Summary: Successfully removed jstype=JS_STRING from all relevant fieldsThe changes in this file consistently remove the
jstype = JS_STRINGoption from int64 fields in the ChangeID, Checkpoint, and TimeTicket messages. These modifications align with the PR objectives and leverage JavaScript's native BigInt support introduced in ES2020.To ensure a smooth transition and maintain the quality of the codebase:
- Coordinate with the js-sdk team to ensure that the corresponding changes (replacing
LongwithBigInt) are implemented in parallel.- Update any relevant documentation, especially API references or client guidelines, to reflect the expectation of BigInt usage for these fields.
- Consider adding inline comments in the protobuf file to explain the rationale behind removing
jstype = JS_STRING. This can be helpful for future maintainers and contributors.- Plan for a comprehensive testing phase, particularly focusing on JavaScript clients, to ensure that the transition to BigInt doesn't introduce any unexpected behaviors or performance issues.
- If not already in place, consider implementing a migration guide or deprecation notice for any clients that may be affected by this change, especially if they are using older JavaScript environments that don't support BigInt.
These steps will help ensure a successful transition and maintain the robustness of the Yorkie project across its ecosystem.
Also applies to: 335-335, 346-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/yorkie/v1/resources.proto (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
api/yorkie/v1/resources.proto (3)
60-61: LGTM. Verify js-sdk compatibility.The removal of
jstype = JS_STRINGfrom theserver_seqandlamportfields aligns with the PR objectives. This change reflects the ability to use BigInt in JavaScript since ES2020.To ensure compatibility, please verify that the corresponding changes in the js-sdk repository (replacing
LongwithBigInt) have been implemented or are planned. This will maintain consistency between the protobuf definitions and the JavaScript implementation.
335-335: LGTM. Consistent with previous changes.The removal of
jstype = JS_STRINGfrom theserver_seqfield in the Checkpoint message is consistent with the changes made to the ChangeID message. This maintains uniformity across the protobuf definitions.
346-346: LGTM. Completes the removal of jstype=JS_STRING.The removal of
jstype = JS_STRINGfrom thelamportfield in the TimeTicket message is consistent with the previous changes and completes the task of updating all relevant int64 fields in this file.These changes collectively update the protobuf definitions to leverage JavaScript's native BigInt support, which should simplify the generated JavaScript code and improve consistency with modern JavaScript practices.
To ensure a smooth transition:
- Confirm that all consumers of this protobuf file (especially JavaScript clients) are prepared to handle int64 values as BigInt.
- Update any documentation or client guidelines to reflect the expectation of BigInt usage for these fields.
- Consider adding a comment in the protobuf file to explain why
jstype = JS_STRINGis no longer used, which could be helpful for future maintainers.
|
@Aswinr24 Thanks for your contribution. Could you sign the CLA? |
Done ! |
hackerwins
changed the title
jstype=string from resources.proto
What this PR does / why we need it:
Removes
jstype=stringnotation from resources.proto file since javascript supports Bigint from ES2020One of the two PRs addressing this issue, the other PR is for the js-sdk repository replacing Long with BigInt.
Which issue(s) this PR fixes:
One of the two PRs that should Fix #1023
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
No
Additional documentation:
Checklist:
Summary by CodeRabbit
ChangeID,Checkpoint, andTimeTicketmessages by reverting certain fields to numeric types, enhancing compatibility and performance.These changes improve the overall efficiency of data handling within the application without altering existing functionalities.