Skip to content

fix(napi): preserve generator class methods#3231

Merged
Brooooooklyn merged 2 commits intomainfrom
fix/3213-preserve-generator-methods
Apr 14, 2026
Merged

fix(napi): preserve generator class methods#3231
Brooooooklyn merged 2 commits intomainfrom
fix/3213-preserve-generator-methods

Conversation

@Brooooooklyn
Copy link
Copy Markdown
Member

@Brooooooklyn Brooooooklyn commented Apr 13, 2026

Note

Medium Risk
Touches N-API class registration and iterator prototype wiring, which can affect JS-visible behavior for all iterator-enabled classes across runtimes. While scoped behind implement_iterator and covered by new tests, regressions could surface as broken prototype chains or missing methods in consumer code.

Overview
Ensures #[napi(iterator)] classes gain JavaScript Iterator helper methods without overwriting their own prototype methods by moving prototype setup from per-instance mutation to class-level inheritance.

This threads an implement_iterator flag through codegen and register_class, records it in module registration metadata, and during module init calls setup_iterator_class to set Class.prototype’s parent to Iterator.prototype when available.

Updates the Fib4 example to add toJSON() and expands generator tests to assert correct instanceof Iterator behavior, preserved prototype chains (including subclasses), and JSON serialization support.

Reviewed by Cursor Bugbot for commit 01fd42a. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • New Features

    • Fib4 now exposes a toJSON() method that returns its internal state as a number array for JSON serialization.
  • Improvements

    • Iterator helper methods are now integrated into class prototype chains without breaking existing class methods, preserving method access and inheritance.
  • Tests

    • Expanded tests to validate prototype-chain preservation, instance identity, and toJSON/JSON.stringify behavior.

Copy link
Copy Markdown
Member Author


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • ready-to-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 13, 2026

📝 Walkthrough

Walkthrough

Adds a safe runtime helper to set up Iterator inheritance for exported classes, changes class registration to carry an implement_iterator flag, updates generated registration calls, and exposes a new Fib4::toJSON method with corresponding TypeScript/test updates validating prototype chains.

Changes

Cohort / File(s) Summary
Iterator runtime
crates/napi/src/bindgen_runtime/iterator.rs
Added pub unsafe fn setup_iterator_class(env, class_ctor) that checks global.Iterator, verifies whether the class already inherits from Iterator.prototype via napi_strict_equals, and conditionally sets the prototype chain using Object.setPrototypeOf. Removed inline prototype setup from create_iterator.
Module registration & metadata
crates/napi/src/bindgen_runtime/module_register.rs
Replaced map value with ClassRegistration { js_name, props, implement_iterator }; extended register_class(...) signature to accept implement_iterator: bool; accumulate implement_iterator and call setup_iterator_class at module registration when set.
Codegen: register_class calls
crates/backend/src/codegen/struct.rs
Generate napi::__private::register_class(...) calls with the new final implement_iterator argument; propagate class.implement_iterator into generated calls (non-wasm/wasm paths).
Fib4 implementation (Rust)
examples/napi/src/generator.rs
Added #[napi(js_name = "toJSON")] pub fn to_json(&self) -> Vec<u32> to Fib4, returning internal state as a two-element vector.
Type declarations & snapshots
examples/napi/index.d.cts, examples/napi/__tests__/__snapshots__/values.spec.ts.md
Added toJSON(): Array<number> instance method signature to exported Fib4 in .d.cts and updated snapshot typings.
Tests: iterator & prototype behavior
examples/napi/__tests__/generator.spec.ts
Expanded tests to assert iterator identity (t.is(iterator, fib)), require two-level prototype chain (instance proto ≠ Iterator.prototype, but next-level equals), and added tests verifying class methods (e.g., toJSON) are preserved while inheriting Iterator helpers; added subclass prototype-chain assertions.

Sequence Diagram(s)

sequenceDiagram
    participant ModuleReg as ModuleRegister
    participant Runtime as BindgenRuntime
    participant Env as NAPI_Env
    participant JS as JS_Global

    ModuleReg->>Runtime: for each registered class with implement_iterator
    Runtime->>Env: setup_iterator_class(env, class_ctor)
    Env->>JS: get global.Iterator
    JS-->>Env: Iterator (function) or undefined
    alt Iterator is function
        Env->>Env: get class.prototype and its parent prototype
        Env->>JS: napi_strict_equals(class_proto_parent, Iterator.prototype)
        alt already inherits
            Env-->>Runtime: no-op
        else not inheriting
            Env->>JS: Object.setPrototypeOf(class.prototype, Iterator.prototype)
            Env-->>Runtime: prototype updated
        end
    else Iterator missing
        Env-->>Runtime: skip setup
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nudged prototypes with a careful paw,
Ensured helpers sit above, but not in awe.
Fib4 keeps its methods, state in a row,
toJSON returns two numbers — ready to show.
Hooray for chains—tidy, steady, and small!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(napi): preserve generator class methods' directly matches the main objective of the PR: preserving generator class methods while providing Iterator helper methods through reworked prototype wiring.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/3213-preserve-generator-methods

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.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3274123565

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/napi/src/bindgen_runtime/iterator.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/napi/src/bindgen_runtime/iterator.rs (1)

255-306: ⚠️ Potential issue | 🟠 Major

Fix prototype chain flattening in Iterator setup.

Calling Object.setPrototypeOf(class_proto, iterator_proto) replaces the existing [[Prototype]] link of class_proto, completely removing any previously inherited base prototype from the chain. If a user subclasses a derived generator class, methods from the base class become inaccessible. Instead, walk the full prototype chain and insert Iterator.prototype between the highest non-Iterator prototype and its current parent.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/napi/src/bindgen_runtime/iterator.rs` around lines 255 - 306, The
current code calls Object.setPrototypeOf(class_proto, iterator_proto) which
flattens the prototype chain and discards any intermediate base prototypes;
instead, walk the prototype chain starting at class_proto using repeated
napi_get_prototype and napi_strict_equals (the same checks used for
class_proto_parent/already_inherits_iterator) to find the highest prototype
whose parent is not Iterator.prototype, then call Object.setPrototypeOf on that
prototype to set its parent to iterator_proto (preserving existing links);
update the argv/ni_get_named_property/Object.setPrototypeOf usage so the
setPrototypeOf call targets the identified prototype rather than class_proto,
ensuring generator classes keep their own methods while inheriting
Iterator.prototype.
🧹 Nitpick comments (1)
examples/napi/__tests__/generator.spec.ts (1)

104-117: Please add a derived-class regression case here.

This proves the single-level Fib4.prototype -> Iterator.prototype path, but not class Child extends Fib4 {}. That is the case most likely to regress if the runtime rewires the wrong link, so it would be worth asserting that both Child.prototype and Fib4.prototype stay in the chain and that toJSON() still resolves from the base prototype.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@examples/napi/__tests__/generator.spec.ts` around lines 104 - 117, Add a
derived-class regression test by defining class Child extends Fib4 and
instantiating it (e.g., const child = new Child(0,1)); assert that
Object.getPrototypeOf(child) === Child.prototype,
Object.getPrototypeOf(Child.prototype) === Fib4.prototype and
Object.getPrototypeOf(Fib4.prototype) === Iterator.prototype (when Iterator
exists); also assert typeof child.toJSON === 'function' and child.toJSON()
returns the same array (and JSON.stringify(child) matches), and that iterator
helpers like child.map are available — this ensures Child.prototype and
Fib4.prototype remain in the chain and toJSON still resolves from
Fib4.prototype.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@crates/napi/src/bindgen_runtime/iterator.rs`:
- Around line 255-306: The current code calls Object.setPrototypeOf(class_proto,
iterator_proto) which flattens the prototype chain and discards any intermediate
base prototypes; instead, walk the prototype chain starting at class_proto using
repeated napi_get_prototype and napi_strict_equals (the same checks used for
class_proto_parent/already_inherits_iterator) to find the highest prototype
whose parent is not Iterator.prototype, then call Object.setPrototypeOf on that
prototype to set its parent to iterator_proto (preserving existing links);
update the argv/ni_get_named_property/Object.setPrototypeOf usage so the
setPrototypeOf call targets the identified prototype rather than class_proto,
ensuring generator classes keep their own methods while inheriting
Iterator.prototype.

---

Nitpick comments:
In `@examples/napi/__tests__/generator.spec.ts`:
- Around line 104-117: Add a derived-class regression test by defining class
Child extends Fib4 and instantiating it (e.g., const child = new Child(0,1));
assert that Object.getPrototypeOf(child) === Child.prototype,
Object.getPrototypeOf(Child.prototype) === Fib4.prototype and
Object.getPrototypeOf(Fib4.prototype) === Iterator.prototype (when Iterator
exists); also assert typeof child.toJSON === 'function' and child.toJSON()
returns the same array (and JSON.stringify(child) matches), and that iterator
helpers like child.map are available — this ensures Child.prototype and
Fib4.prototype remain in the chain and toJSON still resolves from
Fib4.prototype.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 54fed686-e476-4577-95bf-83ae218873fc

📥 Commits

Reviewing files that changed from the base of the PR and between bdc492a and 3274123.

⛔ Files ignored due to path filters (1)
  • examples/napi/__tests__/__snapshots__/values.spec.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • crates/napi/src/bindgen_runtime/iterator.rs
  • examples/napi/__tests__/__snapshots__/values.spec.ts.md
  • examples/napi/__tests__/generator.spec.ts
  • examples/napi/index.d.cts
  • examples/napi/src/generator.rs

@Brooooooklyn Brooooooklyn merged commit 44aa08f into main Apr 14, 2026
75 checks passed
@Brooooooklyn Brooooooklyn deleted the fix/3213-preserve-generator-methods branch April 14, 2026 01:53
@github-actions github-actions Bot mentioned this pull request Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: ScopedGenerator overrides other class methods

1 participant