-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
Add more utility traits and funtions to boa_interop #3773
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3773 +/- ##
==========================================
+ Coverage 47.24% 50.30% +3.05%
==========================================
Files 476 458 -18
Lines 46892 44905 -1987
==========================================
+ Hits 22154 22589 +435
+ Misses 24738 22316 -2422 ☔ View full report in Codecov by Sentry. |
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.
Just have two small suggestions. Everything else looks great!
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.
Really nice API! I'm preemptively approving considering that we'll work on this incrementally, but left some additional comments for discussion.
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.
This is looking great! I had one small question.
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.
LGTM :)
This PR adds support for
Fn(...)
closures, assuming the closure isCopy
. This should satisfy theunsafe
requirement established in #3772 for that kind of function.This also add implementations for both
IntoJsFunction
andIntoJsFunctionUnsafe
for up to 12 arguments, including aJsRest
argument, aJsThis
argument and aContext
argument, all of whom can be placed at the end of the list of required arguments.This also adds support for return value from the functions, both
JsValue
-convertible types as well asJsResult<...>
, as long as the inner value is convertible to aJsValue
.