Add external#42
Merged
Merged
Conversation
zig-napi ArkVM benchmark
|
There was a problem hiding this comment.
Pull request overview
Adds first-class support for N-API “external” values to the zig-napi wrapper layer, plus TS type generation and coverage in both the Node test addon and the ArkTS test suite.
Changes:
- Introduce
napi.External(T)wrapper with type-tagging, optional size hint, and finalizer-based cleanup. - Integrate externals into runtime conversion (
from_napi_value/to_napi_value) and TS type generation (ExternalObject<T>). - Add external-focused tests and example exports to validate roundtrips, mutation, and type-mismatch errors.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/external.spec.ts | Adds ArkTS test coverage for creating/reading/mutating externals and mismatch errors. |
| test/basic.ts | Registers the new external test in the basic test suite. |
| src/napi/wrapper/external.zig | Implements External(T) wrapper, tagging, finalizer cleanup, and size-hint support. |
| src/napi/util/napi.zig | Enables conversions/type checks to recognize External(T) as a supported wrapper type. |
| src/napi/util/helper.zig | Adds isExternal() helper used by conversion and fast-path logic. |
| src/napi.zig | Exposes External from the top-level napi module. |
| src/build/napi-tsgen.zig | Generates TS surface types for External(T) as ExternalObject<T>. |
| node-test/napi/src/values.zig | Adds Node-test addon functions exercising External(u32) and External(ExternalPoint). |
| node-test/napi/src/lib.zig | Re-exports the new node-test external functions. |
| node-test/napi/tests/values.spec.js | Adds JS tests validating external behavior and error cases. |
| examples/basic/src/hello.zig | Exposes external example functions from the basic example module. |
| examples/basic/src/external.zig | Implements basic example external functions mirroring the node-test API. |
| examples/basic/index.d.ts | Updates generated TS declarations to include ExternalObject<T> and external APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+112
| pub fn to_napi_value(self: Self, env: napi.napi_env) !napi.napi_value { | ||
| if (self.raw != null) { | ||
| return self.raw; | ||
| } | ||
|
|
||
| const size_hint_i64 = std.math.cast(i64, self.header.size_hint) orelse { | ||
| self.destroyDetached(); | ||
| return NapiError.Error.fromStatus(NapiError.Status.InvalidArg); | ||
| }; | ||
|
|
||
| var result: napi.napi_value = undefined; | ||
| const status = napi.napi_create_external(env, self.header, externalFinalizer, null, &result); | ||
| if (status != napi.napi_ok) { | ||
| self.destroyDetached(); | ||
| return NapiError.Error.fromStatus(NapiError.Status.New(status)); | ||
| } |
Comment on lines
+82
to
+88
| } | ||
|
|
||
| const header: *TaggedHeader = @ptrCast(@alignCast(data.?)); | ||
| if (header.magic != external_magic or !std.mem.eql(u8, header.typeName(), @typeName(T))) { | ||
| NapiError.last_error = NapiError.Error.withCodeAndMessage("InvalidArg", "External value type does not match expected type"); | ||
| return invalid(env, raw); | ||
| } |
Comment on lines
+121
to
+125
| if (adjust_status != napi.napi_ok) { | ||
| return NapiError.Error.fromStatus(NapiError.Status.New(adjust_status)); | ||
| } | ||
| self.header.memory_adjusted = true; | ||
| self.header.adjusted_size = adjusted_size; |
Comment on lines
+1158
to
+1159
| try append(&state.declarations, "export interface ExternalObject<T> {\n"); | ||
| try append(&state.declarations, " readonly __napiExternal?: T\n"); |
Comment on lines
+228
to
+230
| if (env != null and header.memory_adjusted and header.size_hint > 0) { | ||
| var adjusted_size: i64 = 0; | ||
| _ = napi.napi_adjust_external_memory(env, -@as(i64, @intCast(header.size_hint)), &adjusted_size); |
| .header = header, | ||
| }; | ||
| } | ||
|
|
Comment on lines
173
to
177
| if (comptime helper.isTypedArray(T)) break :blk isTypedArrayValue(env, raw); | ||
| if (comptime helper.isDataView(T)) break :blk isDataViewValue(env, raw); | ||
| if (comptime helper.isReference(T)) break :blk true; | ||
| if (comptime helper.isExternal(T)) break :blk napiTypeOf(env, raw) == napi.napi_external; | ||
| if (comptime helper.isTuple(T)) break :blk isArrayValue(env, raw); |
This file contains hidden or 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
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.
No description provided.