-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat: Rewrite Node-API #24101
feat: Rewrite Node-API #24101
Conversation
unsafe impl Send for Env {} | ||
unsafe impl Sync for Env {} | ||
|
||
impl Env { |
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.
We're doing something wrong here 😅 we define these are Send
and Sync
and then we accept Rc<RefCell<...>>
for `cleanup_hooks. That's trouble. Maybe open an issue about that for a follow up cleanup?
pub fn create_function_template<'a>( | ||
/// # Safety | ||
/// env_ptr must be valid | ||
pub unsafe fn create_function_template<'a>( | ||
env_ptr: *mut Env, |
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.
I think this can now be &mut Env
?
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.
&mut Env
is annoying with the lifetime of the created js object. i think this could be cleaned up in a followup pr.
pub fn create_function<'a>( | ||
/// # Safety | ||
/// env_ptr must be valid | ||
pub unsafe fn create_function<'a>( | ||
env_ptr: *mut Env, |
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.
Ditto
a052391
to
2975d64
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.
Nice
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.
65a65a3
to
9887033
Compare
After landing this PR, let's update |
Includes denoland/deno_core#770 necessary for #24101. Also includes denoland/deno_core#769 that fixes #24098 and #24069 and #24089.
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, only a few small comments/questions
if length > 0 { | ||
check_arg!(env, string); | ||
} | ||
crate::return_status_if_false!( | ||
env, | ||
(length == NAPI_AUTO_LENGTH) || length <= INT_MAX as _, | ||
napi_invalid_arg | ||
); | ||
|
||
let buffer = if length > 0 { | ||
unsafe { | ||
std::slice::from_raw_parts( | ||
string, | ||
if length == NAPI_AUTO_LENGTH { | ||
let mut length = 0; | ||
while *(string.add(length)) != 0 { | ||
length += 1; | ||
} | ||
length | ||
} else { | ||
length | ||
}, | ||
) | ||
} | ||
_ => { | ||
return napi_invalid_arg; | ||
} else { | ||
&[] | ||
}; |
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.
Could probably extract this snippet into a function, it's identical to the code in napi_create_string_utf16
🥳 |
Includes denoland/deno_core#770 necessary for #24101. Also includes denoland/deno_core#769 that fixes #24098 and #24069 and #24089.
The Node-API implementation in deno has been rewritten recently: denoland/deno#24101 Running a basic example, deno v1.44.2 seems to work fine (and doesn't have bun's issues that are fixed in the previous commit).
Phase 1 node-api rewrite