Consider a `String` object an `object` instead of `string` in node-api #25353

kjvalencik · 2025-12-05T16:00:08Z

What does this PR do?

How did you verify your code works?

See the issue for repro steps.

coderabbitai · 2025-12-05T16:05:40Z

Walkthrough

The napi_typeof function is updated to correctly classify string objects (created via new String()) as objects rather than strings. StringObjectType and DerivedStringObjectType are now handled through the ObjectType case with a dynamic cast check, returning napi_external or napi_object as appropriate instead of napi_string.

Changes

Cohort / File(s)	Change Summary
Node-API type classification fix `src/bun.js/bindings/napi.cpp`	Modified `napi_typeof` to treat `StringObjectType` and `DerivedStringObjectType` as object types via dynamic cast check for `Bun::NapiExternal`, returning `napi_external` or `napi_object` instead of `napi_string`; removed explicit fallthrough handling from `StringType` branch

Pre-merge checks

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: treating String objects as objects instead of strings in node-api.
Description check	✅ Passed	The description includes both required sections: 'What does this PR do?' with issue reference and 'How did you verify your code works?' with repro steps.
Linked Issues check	✅ Passed	The code changes correctly address the linked issue #25351 by fixing napi_typeof to return napi_object for String object instances instead of napi_string.
Out of Scope Changes check	✅ Passed	All changes in napi.cpp are directly scoped to fixing the String object type handling in napi_typeof, with no unrelated modifications.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d60b6f and cf61344.

📒 Files selected for processing (1)

src/bun.js/bindings/napi.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

src/bun.js/bindings/napi.cpp

src/bun.js/bindings/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

src/bun.js/bindings/**/*.cpp: JavaScript class implementations in C++ should create three classes if there's a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties in C++ JavaScript classes using HashTableValue arrays
Add iso subspaces for C++ JavaScript classes with C++ fields
Cache structures in ZigGlobalObject for C++ JavaScript classes

Files:

src/bun.js/bindings/napi.cpp

🧠 Learnings (15)

📓 Common learnings

Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

📚 Learning: 2025-11-24T18:37:47.899Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-12-02T05:59:51.485Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : JavaScript class implementations in C++ should create three classes if there's a public constructor: `Foo` (JSDestructibleObject), `FooPrototype` (JSNonFinalObject), and `FooConstructor` (InternalFunction)

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Create V8 class implementations with .cpp extension following the pattern V8ClassName.cpp that include the header, v8_compatibility_assertions.h, use ASSERT_V8_TYPE_LAYOUT_MATCHES macro, and implement methods using isolate->currentHandleScope()->createLocal<T>() for handle creation

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-10-01T21:59:54.571Z

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-12-02T05:59:51.485Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for C++ JavaScript classes with C++ fields

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : For objects that need internal fields, extend InternalFieldObject class instead of directly extending Data

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Create V8 class headers with .h extension following the pattern V8ClassName.h that include pragma once, v8.h, V8Local.h, V8Isolate.h, and declare classes extending from Data with BUN_EXPORT static methods

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:35:25.883Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.cpp : Implement getter functions using `JSC_DEFINE_CUSTOM_GETTER` macro, performing type checking with `jsDynamicCast`, throwing errors for type mismatches, and returning encoded JSValue

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:35:25.883Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.cpp : Implement function definitions using `JSC_DEFINE_HOST_FUNCTION` macro, performing type checking with `jsDynamicCast`, throwing this-type errors when type checking fails, and returning encoded JSValue

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations

Applied to files:

src/bun.js/bindings/napi.cpp

📚 Learning: 2025-11-24T18:35:25.883Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.cpp : To create JavaScript objects from Zig, implement C++ functions following the `Bun__ClassName__toJS(Zig::GlobalObject*, NativeType*)` convention that construct and return the JavaScript object as an encoded JSValue

Applied to files:

src/bun.js/bindings/napi.cpp

🔇 Additional comments (1)

src/bun.js/bindings/napi.cpp (1)

2393-2402: Correctly classify String wrapper objects as napi_object

Including StringObjectType and DerivedStringObjectType in the ObjectType case so they return napi_object (and never napi_string) matches JS typeof semantics and Node’s napi_typeof behavior for new String() / derived String subclasses, while still correctly detecting Bun::NapiExternal as napi_external. This looks semantically right and well-scoped.

Please just confirm against the repro from issue #25351 and any existing Node-API napi_typeof tests to ensure parity with Node.js in edge cases (e.g., subclasses of String).

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Consider a String object an object instead of string in node-api

cf61344

Fixes oven-sh#25351

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Consider a `String` object an `object` instead of `string` in node-api #25353

Consider a `String` object an `object` instead of `string` in node-api #25353

kjvalencik commented Dec 5, 2025

Uh oh!

coderabbitai bot commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Consider a String object an object instead of string in node-api #25353

Are you sure you want to change the base?

Consider a String object an object instead of string in node-api #25353

Conversation

kjvalencik commented Dec 5, 2025

What does this PR do?

How did you verify your code works?

Uh oh!

coderabbitai bot commented Dec 5, 2025

Walkthrough

Changes

Pre-merge checks

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Consider a `String` object an `object` instead of `string` in node-api #25353

Consider a `String` object an `object` instead of `string` in node-api #25353