-
-
Notifications
You must be signed in to change notification settings - Fork 435
chore: replace bigint-buffer #8789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @rnfairchild, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically replaces the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully replaces the bigint-buffer package with @vekexasia/bigint-buffer2 across the codebase. The changes are consistent in package.json files, import statements, and the pnpm-lock.yaml file. A positive side effect of this replacement is the removal of several transitive dependencies (bindings, file-uri-to-path), which simplifies the dependency tree. The changes appear correct and well-contained to the stated motivation.
Performance Report✔️ no performance regression detected Full benchmark results
|
This reverts commit 62c43f3.
browser tests return Uint8Array, unit tests return Buffer due to new library implementation
e4d0743 to
58192b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WAY TO GO ROBBY!!!! 🎉 🎉
This is epic work dude! I'm really proud of you!
Just want to give some others on the team time to digest the rust code. I glanced through and it looks to be well structured and has unit tests. I am a bit hesitant to approve though as there is very low adoption on the package. This is the package that @nflaig suggested that we use so I will look to him for final approval. I think it also good to get @wemeetagain to give his blessing as well as he is a Rustacion. In particular I think we may want to look at the test cases in the lib to make sure they are covering the edge cases.
Overall this is great and I LOVE the initiative! Keep it up dude, you 🪨 thoroughly!!
|
FYI @nflaig and @wemeetagain I have asked Robby to use Gemini to audit the rust code and tests. He will post his results as a first pass of the lib. We can go from there |
what you mean by "buffer" here, browsers don't support |
I am a bit hesitant to switch to this library just yet, I would like this see more adoption first, we also want to have this running on a feat group for a while to make sure it's stable and performant the current library has it's own issue but the known security vulnerabilities do not affect us from last time I checked |
|
Looked into the upstream library. It looks solid. Only critique I could find was how it selects native vs browser implementations. It dynamically loads native vs browser, instead of using conditional exports via package.json. |
Motivation
Description
bigint-bufferwith@vekexasia/bigint-buffer2Closes #8771