-
Notifications
You must be signed in to change notification settings - Fork 269
updated napi crate from v2 to v3 #2228
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
updated napi crate from v2 to v3 #2228
Conversation
@ethanlijin is attempting to deploy a commit to the Gloo Team on Vercel. A member of the Team first needs to authorize it. |
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.
Caution
Changes requested ❌
Reviewed everything up to f2bc26c in 1 minute and 41 seconds. Click for details.
- Reviewed
894
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
9
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_client_typescript/src/types/type_builder.rs:142
- Draft comment:
The union method locks each FieldType individually in the iteration. Consider acquiring the lock once per element (or batching the clones) to avoid repeated lock acquisitions if performance becomes a concern. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. engine/language_client_typescript/src/types/type_builder.rs:152
- Draft comment:
In the add_baml method, error mapping is done via from_anyhow_error. It would be beneficial to include additional context in the error message so that debugging failures (e.g., due to malformed BAML strings) is easier. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/language_client_typescript/src/types/type_builder.rs:128
- Draft comment:
Locking with unwrap() is used several times throughout the builder methods. Consider handling potential lock failures more gracefully (e.g., using expect with a descriptive message) to aid debugging if the lock is poisoned. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/language_client_typescript/src/types/type_builder.rs:185
- Draft comment:
Typographical issue: The block comment starting on this line is not closed. If this is intentional to comment out the code, please add a closing '*/' to properly close the comment block. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. engine/language_client_typescript/src/types/type_builder.rs:196
- Draft comment:
Typo: It appears that there's a stray block comment terminator (*/
) on this line. If this is not intended to be part of a comment, please remove it to prevent confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. engine/language_client_typescript/src/types/type_builder.rs:214
- Draft comment:
Typographical issue: The block comment starts with '/' on this line but does not appear to have a corresponding closing '/'. Please close the comment block or remove the opening if unintended. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. engine/language_client_typescript/src/types/type_builder.rs:225
- Draft comment:
There appears to be a stray closing block comment marker (*/) on this line. Please verify that this is intentional or remove it if it's a leftover from a previous comment block. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. engine/language_client_typescript/src/types/type_builder.rs:255
- Draft comment:
There's a stray closing comment marker (*/
) on line 255 with no matching opening comment. Please remove it or fix the comment block. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. engine/language_client_typescript/src/types/type_builder.rs:322
- Draft comment:
Typographical error: The stray '*/' appears to be an unused block comment terminator. Consider removing it if it's not intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is clearly incorrect. The '/' at line 322 is not stray - it's properly closing a block comment that starts at line 311. This is part of a pattern in the file where old implementations are commented out before being replaced with new ones. The comment is suggesting fixing something that isn't actually broken. Could there be some formatting or whitespace issue that's not visible in the plain text that's causing this to look like a stray comment to the tool? No - the code structure is very clear and consistent. The block comment is properly formatted and is part of a clear pattern used throughout the file for commenting out old implementations. The comment should be deleted as it is incorrect - there is no stray '/' and the code is properly formatted.
Workflow ID: wflow_BSP0nzSHxvU87hJ5
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -182,6 +182,7 @@ impl EnumBuilder { | |||
self.inner.lock().unwrap().value(&name).into() | |||
} | |||
|
|||
/* |
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.
The old alias implementation (using Option<&str>) is commented out and replaced with the new Option version. Consider removing these commented-out blocks to simplify maintenance and improve code clarity.
ty - we've got a big release pending so won't be able to look at this til friday |
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.
Nice, I think we should migrate rest of types to V3 Values
Since the V2 - V3 Migration Guide specifies that some JsValues
are unsafe. I'll push some changes to make this compile on canary
then I'll take a look at Values
.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
@sxlijin
Important
Update
napi
crate from v2 to v3, adjusting dependencies and code to accommodate API changes.napi
to version3.1.3
andnapi-derive
to3.1.1
inCargo.toml
.@napi-rs/cli
to3.0.4
inpackage.json
.JsUnknown
withUnknown
inparse_ts_types.rs
,log_collector.rs
, andrequest.rs
.Function
andFunctionRef
for callbacks inruntime.rs
andfunction_result_stream.rs
.serde_value_to_js
to returnUnknown
instead ofJsUnknown
inlog_collector.rs
.napi_derive::module_init
instead ofnapi::module_init
inlib.rs
.compat-mode
feature tonapi
inCargo.toml
.Cargo.lock
for updated dependencies.This description was created by
for f2bc26c. You can customize this summary. It will automatically update as commits are pushed.