napi: report Node-API version 10 and accept zero-length external strings

[robobun]

Problem

napi_get_version() returns 9, but Bun already exports and implements the complete Node-API v10 surface (node_api_create_external_string_latin1/utf16, node_api_create_property_key_latin1/utf8/utf16), and process.versions.napi already reads "10". Spec-following addons that feature-detect with napi_get_version() >= 10 take the degraded fallback path even though the v10 APIs work.

uint32_t ver; napi_get_version(env, &ver);
// node v26: ver=10
// bun     : ver=9
// ...but dlsym finds the full v10 surface on bun, and they work.

While auditing the v10 surface, two actual gaps:

• node_api_create_external_string_latin1 / _utf16 reject length == 0 with napi_invalid_arg (WTF::ExternalStringImpl cannot represent empty strings). Node.js returns an empty string, writes *copied = false, and invokes the finalizer immediately.
• node_api_create_external_string_utf16 never wrote *copied on success (the latin1 variant did).

Fix

• src/runtime/napi/napi_body.rs: bump the hardcoded version from 9 to 10, noting it must track process.versions.napi.
• src/jsc/bindings/napi.cpp: for both external-string creators, when length == 0 return jsEmptyString, set *copied = false, and call the finalizer synchronously. The utf16 variant now also writes *copied = false on the non-empty success path. Both now reject with napi_pending_exception before adopting str when a napi exception is already stashed, matching napi_create_external_buffer / _arraybuffer, so callers cleanly retain ownership on non-ok.

Tests

• New test_napi_v10_surface in test/napi/napi.test.ts compares Bun against Node via checkSameOutput: asserts napi_get_version() >= 10, empty external strings return status=0 copied=0 finalized=1 length=0 for both encodings, and copied=0 for a non-empty utf16 external string.
• Removed the Bun-only if (str.length > 0) guards in the vendored test_string/test.js so empty strings now round-trip through TestLatin1External / TestUtf16External.
• Bumped the vendored test_general/test.js expected version to 10.

Verification

$ bun bd test test/napi/napi.test.ts -t "reports Node-API v10"
(pass) napi > napi_get_version / node_api_create_external_string_* > reports Node-API v10 and accepts zero-length external strings

$ bun bd test test/napi/node-napi-tests/test/js-native-api/test_string/do.test.ts
3 pass, 0 fail

---

no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts

[coderabbitai]

Walkthrough

Changes

N-API v10 external string support

Layer / File(s)	Summary
N-API v10 contract and external string creation src/runtime/napi/napi_body.rs, src/jsc/bindings/napi.cpp	N-API version reporting changes to 10, while external Latin-1 and UTF-16 string creation handles pending exceptions, empty inputs, finalizers, and copied state.
Standalone v10 surface validation test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts	Standalone coverage validates empty external strings, finalization, copied results, and UTF-8-visible lengths.
Node-API compatibility coverage test/napi/node-napi-tests/test/js-native-api/test_general/test.js, test/napi/node-napi-tests/test/js-native-api/test_string/test.js	Compatibility tests expect N-API version 10 and validate empty and non-empty external Latin-1 and UTF-16 strings.

Possibly related PRs

• oven-sh/bun#34137: Updates related pending-exception gating in the N-API entry-point handling.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately summarizes the main change: N-API v10 reporting and zero-length external string handling.
Description check	✅ Passed	The description covers the change, rationale, tests, and verification, with only non-template section headings differing.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Reproduced with USE_SYSTEM_BUN=1 bun test/napi/napi-app/main.js test_napi_v10_surface '[]' (prints version>=10 = false, status=1 copied=1 finalized=0). Fixed build passes bun bd test test/napi/napi.test.ts -t 'reports Node-API v10' with output matching Node byte-for-byte.

Rebased on cc0c1e8; the get_all_property_names test-addon dedup was already fixed on main and dropped from this diff (6 files now).

CI status: the napi changes are green on every lane across builds 72876, 72885, and 72943. Remaining red on 72943 is unrelated: test-net-connect-memleak.js (pre-existing on main, being fixed separately) plus known flakes (es-module-lexer, spawn, no-orphans, webview-chrome, fetch.stream, issue 20144/30205). Ready for a maintainer to merge.

[robobun]

^{Updated 10:53 AM PT - Jul 14th, 2026}

❌ @robobun, your commit 08b5f64 has 1 failures in Build #72943 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 34146

That installs a local version of the PR into your bun-34146 executable, so you can run:

bun-34146 --bun

[robobun]

Independently reproduced and fixed the same two gaps (zero-length rejection + utf16 *copied never written) on current main at c71f98b; branch farm/c8150e60/napi-external-string-empty-copied has a templated napi_create_external_string_impl shared between latin1/utf16 plus a checkSameOutput test that also asserts the finalizer receives the caller's original buffer pointer and that NAPI_AUTO_LENGTH on an empty string works. Happy to fold any of it in here if it helps the rebase; this PR is currently conflicting against main.

[claude]

LGTM — the follow-up commits (2ff5f94, f9e7fe5) address all three of my earlier inline concerns, and the (NULL, 0) question was resolved empirically against Node/V8.

What was reviewed:

• length == 0 branches: doFinalizer null-checks the callback (napi.h:417), NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION handles a throwing finalizer, and the new hasPendingException() guard matches napi_create_external_buffer/_arraybuffer so callers cleanly retain ownership on pre-existing exceptions.
• utf16 non-empty path now writes *copied = false; latin1's existing write was hoisted to the same position.
• napi_get_version → 10 matches process.versions.napi in BunProcess.cpp:254; vendored test_general/test_string guards updated in lockstep.
• get_all_property_names dedup: the surviving {status, keys} variant is still registered and the one call site expecting a bare array was updated.

Extended reasoning...

Overview

This PR bumps napi_get_version() from 9 to 10 (aligning with process.versions.napi), fixes node_api_create_external_string_latin1/_utf16 to accept zero-length input (previously rejected because WTF::ExternalStringImpl cannot represent empty strings), and fixes the utf16 variant to write *copied on success. It also deduplicates a get_all_property_names test helper that was defined twice on main after two PRs merged. Files touched: src/jsc/bindings/napi.cpp, src/runtime/napi/napi_body.rs, and five test files.

Review history

I left three inline comments across two prior runs, all now resolved:

1. NAPI_RETURN_SUCCESS after synchronous doFinalizer would trip assertNoException() if the finalizer threw — fixed by switching to NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION (2ff5f94).
2. The switch to _UNLESS_EXCEPTION opened a path where a pre-stashed napi exception could cause napi_pending_exception to be returned after the finalizer freed str, risking double-free in callers that free-on-non-ok — fixed by adding NAPI_RETURN_EARLY_IF_FALSE(env, !env->hasPendingException(), napi_pending_exception) before adoption (f9e7fe5), matching the sibling napi_create_external_buffer/_arraybuffer pattern.
3. (str=NULL, length=0) — the author tested empirically and Node/V8 aborts on this via CHECK(resource && resource->data()), so Bun returning napi_invalid_arg is strictly better than matching Node's crash. No change needed.

The remaining path-2 case (finalizer itself throws → returns napi_pending_exception after freeing) was consciously left as-is with a clear rationale: returning napi_ok with a live VM exception would trip the assert, and a throwing external-string finalizer is an addon bug in both runtimes.

Security risks

None. No user-controlled parsing, no auth/crypto, no path handling. The ownership-contract concern (double-free on non-ok after finalizer ran) was the closest thing to a memory-safety issue and has been addressed for the reachable case.

Level of scrutiny

NAPI C++ bindings warrant careful review for exception-scope and ownership correctness, which is why this went through three rounds. The final shape now matches established sibling code (napi_create_external_buffer), doFinalizer correctly handles a null callback, and the new test uses checkSameOutput to diff Bun against Node byte-for-byte. The vendored Node test_string suite now exercises empty external strings unconditionally.

Other factors

• The get_all_property_names dedup fixes a real build break on main; I verified the surviving definition is still registered and the one call site that expected a bare array (module.js) was updated to destructure {keys}.
• robobun mentioned a parallel branch with a templated shared helper — that's a nice-to-have refactor, not a correctness concern, and can be a follow-up.
• Version bump is consistent: process.versions.napi was already "10" in BunProcess.cpp.

[claude]

LGTM — all three prior concerns addressed (2ff5f94, f9e7fe5, and the empirical (NULL, 0) check against V8).

What was reviewed:

• length == 0 branches now guard hasPendingException() before adopting, matching napi_create_external_buffer/_arraybuffer; doFinalizer null-checks finalize_cb so a null finalizer is a no-op.
• napi_get_version() bump to 10 agrees with process.versions.napi in BunProcess.cpp:254.
• utf16 *copied = false now written on the non-empty success path; checkSameOutput test pins all three behaviors against Node.
• get_all_property_names dedup keeps the {status, keys} variant and updates the one bare-array caller in module.js.

Extended reasoning...

Overview

Three focused changes: (1) bump napi_get_version() from 9 to 10 in napi_body.rs to match the already-shipped v10 surface and process.versions.napi; (2) make node_api_create_external_string_latin1/_utf16 accept length == 0 by returning jsEmptyString, writing *copied = false, and invoking the finalizer synchronously (WTF::ExternalStringImpl cannot represent empty strings); (3) fix the utf16 variant to write *copied on the non-empty success path. Also deduplicates get_all_property_names in the test addon (main was broken by two merged PRs each adding a definition), removes Bun-only if (str.length > 0) guards from the vendored test_string/test.js, and bumps the vendored test_general/test.js expected version.

Security risks

None. No user-controlled parsing, no auth/crypto/permissions. The memory-ownership contract (finalizer invocation vs. caller-retains-on-error) was the risk here; three review rounds hardened it — the hasPendingException() guard now rejects before adopting so callers cleanly retain ownership on non-ok, and the remaining path-2 case (a finalizer that itself throws after freeing) is an addon bug in both runtimes as the author noted.

Level of scrutiny

Medium. N-API is a compatibility-critical surface, but the change is narrow (~15 lines × 2 nearly-identical functions), follows the exact pattern of the sibling napi_create_external_buffer/_arraybuffer in the same file, and is pinned by a checkSameOutput test that byte-diffs against Node. The vendored Node test_string suite now exercises the empty-string path directly.

Other factors

This is my third pass. All three prior inline comments are resolved: the assertNoException debug assert (fixed via NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION), the double-free-on-pending-exception path (fixed via the hasPendingException() early return), and the (NULL, 0) question (author verified empirically that V8's String::NewExternalOneByte aborts on null data, so returning napi_invalid_arg is strictly better than Node). doFinalizer null-checks finalize_cb (napi.h:417), so the length == 0 branch is safe with a null finalizer. A separate branch independently arrived at the same fix, and the bug hunting system found nothing on this revision.

[claude]

LGTM — all prior review threads addressed; the final shape matches the napi_create_external_buffer/_arraybuffer sibling pattern exactly.

Checked: doFinalizer null-checks finalize_cb so the length==0 branch is safe with a null finalizer; *copied is written only after the pending-exception guard so out-params stay untouched on non-ok; the (NULL, 0) case staying napi_invalid_arg is correct given V8 aborts there. Tests pin behavior via checkSameOutput against Node and un-gate the vendored test_string empty-string cases.

Extended reasoning...

Overview

Bumps napi_get_version() from 9 → 10 (aligning with process.versions.napi), and fixes two gaps in node_api_create_external_string_latin1/_utf16: accept length == 0 by returning jsEmptyString + running the finalizer immediately (WTF::ExternalStringImpl can't represent empty), and write *copied = false on the utf16 success path. Adds a hasPendingException() guard before adopting str so callers cleanly retain ownership on napi_pending_exception. ~30 lines of native change plus a standalone C++ test, a checkSameOutput test, and un-gating of vendored Node test_string empty-string assertions.

Security risks

None. No user-controlled parsing, no auth/crypto. The ownership contract ("non-ok ⇒ caller still owns str") was the one memory-safety concern; it's now enforced by rejecting pending exceptions before the finalizer runs, matching the sibling external-buffer/arraybuffer adopters line-for-line.

Level of scrutiny

Medium — native N-API code with finalizer/ownership semantics. This went through three rounds of review here (exception-scope assert, pending-exception-before-adoption, (NULL, 0) edge case), each addressed with either a fix or an empirical Node.js counter-test. The final code is small, mechanical, and copies the exact guard pattern already shipping in napi_create_external_buffer/_arraybuffer.

Other factors

The new test uses checkSameOutput so it asserts byte-for-byte parity with Node for version, status, copied, finalizer-fired, and length across both encodings plus a non-empty utf16 control. The vendored test_string/test.js guards are removed so empty strings now round-trip through the external creators, and test_general/test.js pins version 10. doFinalizer (napi.h:417) early-returns on null finalize_cb, so passing nullptr in the length==0 path is safe. The remaining "throwing finalizer returns napi_pending_exception after freeing" edge is an addon bug in both runtimes and was reasonably left as-is.