Skip to content
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

Merged
merged 1 commit into from
Jun 10, 2024
Merged

feat: Rewrite Node-API #24101

merged 1 commit into from
Jun 10, 2024

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 4, 2024

Phase 1 node-api rewrite

ext/node/polyfills/process.ts Outdated Show resolved Hide resolved
ext/napi/lib.rs Show resolved Hide resolved
Comment on lines 375 to 363
unsafe impl Send for Env {}
unsafe impl Sync for Env {}

impl Env {
Copy link
Member

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?

ext/napi/lib.rs Outdated Show resolved Hide resolved
pub fn create_function_template<'a>(
/// # Safety
/// env_ptr must be valid
pub unsafe fn create_function_template<'a>(
env_ptr: *mut Env,
Copy link
Member

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?

Copy link
Member Author

@devsnek devsnek Jun 6, 2024

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

cli/napi/util.rs Outdated Show resolved Hide resolved
cli/napi/threadsafe_functions.rs Outdated Show resolved Hide resolved
cli/napi/node_api.rs Show resolved Hide resolved
@devsnek devsnek added ci-draft Run the CI on draft PRs. labels Jun 5, 2024
@devsnek devsnek force-pushed the x/napi-rework branch 3 times, most recently from a052391 to 2975d64 Compare June 5, 2024 04:35
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

cli/napi/node_api.rs Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that #23281 or #24010 have been fixed?

ext/napi/value.rs Outdated Show resolved Hide resolved
ext/napi/lib.rs Show resolved Hide resolved
cli/napi/node_api.rs Outdated Show resolved Hide resolved
@devsnek devsnek force-pushed the x/napi-rework branch 2 times, most recently from 65a65a3 to 9887033 Compare June 6, 2024 20:57
@devsnek devsnek marked this pull request as ready for review June 6, 2024 23:25
@devsnek
Copy link
Member Author

devsnek commented Jun 6, 2024

Have you tested that #23281 or #24010 have been fixed?

yep. they do not appear to be blocked on n-api issues anymore.

@bartlomieju
Copy link
Member

Have you tested that #23281 or #24010 have been fixed?

yep. they do not appear to be blocked on n-api issues anymore.

After landing this PR, let's update npm_smoke_tests repo with repros for these issues 👍

bartlomieju added a commit that referenced this pull request Jun 7, 2024
Copy link
Member

@nathanwhit nathanwhit left a 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

cli/napi/js_native_api.rs Outdated Show resolved Hide resolved
cli/napi/js_native_api.rs Outdated Show resolved Hide resolved
cli/napi/js_native_api.rs Show resolved Hide resolved
cli/napi/js_native_api.rs Show resolved Hide resolved
cli/napi/util.rs Show resolved Hide resolved
Comment on lines +1203 to +1229
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 {
&[]
};
Copy link
Member

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

cli/napi/js_native_api.rs Outdated Show resolved Hide resolved
cli/napi/node_api.rs Outdated Show resolved Hide resolved
@devsnek devsnek enabled auto-merge (squash) June 10, 2024 15:53
@devsnek devsnek merged commit e3b2ee1 into main Jun 10, 2024
17 checks passed
@devsnek devsnek deleted the x/napi-rework branch June 10, 2024 16:20
@janpio
Copy link

janpio commented Jun 10, 2024

🥳

nathanwhit pushed a commit that referenced this pull request Jun 13, 2024
nathanwhit pushed a commit that referenced this pull request Jun 13, 2024
Phase 1 node-api rewrite
eilvelia added a commit to eilvelia/tdl that referenced this pull request Jun 18, 2024
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-draft Run the CI on draft PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants