feat: Rewrite Node-API #24101
Merged
feat: Rewrite Node-API #24101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartlomieju
reviewed
Jun 4, 2024
Comment on lines
375
to
363
| unsafe impl Send for Env {} | ||
| unsafe impl Sync for Env {} | ||
|
|
||
| impl Env { |
There was a problem hiding this comment.
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.
I think this can now be &mut Env?
There was a problem hiding this comment.
&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, |
devsnek
force-pushed
the
x/napi-rework
branch
3 times, most recently
from
a052391
to
2975d64
Compare
devsnek
force-pushed
the
x/napi-rework
branch
2 times, most recently
from
65a65a3
to
9887033
Compare
After landing this PR, let's update |
bartlomieju
added a commit
that referenced
this pull request
Jun 7, 2024
Includes denoland/deno_core#770 necessary for #24101. Also includes denoland/deno_core#769 that fixes #24098 and #24069 and #24089.
nathanwhit
approved these changes
Jun 7, 2024
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 { | ||
| &[] | ||
| }; |
There was a problem hiding this comment.
Could probably extract this snippet into a function, it's identical to the code in napi_create_string_utf16
littledivy
reviewed
Jun 8, 2024
|
🥳 |
This was referenced Jun 10, 2024
This was referenced Jun 10, 2024
This was referenced Jun 11, 2024
nathanwhit
pushed a commit
that referenced
this pull request
Jun 13, 2024
Includes denoland/deno_core#770 necessary for #24101. Also includes denoland/deno_core#769 that fixes #24098 and #24069 and #24089.
Bannerets
added a commit
to Bannerets/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).
This was referenced Jul 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Phase 1 node-api rewrite