Skip to content

fix(runtime): link cluster default to EventEmitter prototype - #9905

Closed
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9202-cluster-default-prototype
Closed

fix(runtime): link cluster default to EventEmitter prototype#9905
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9202-cluster-default-prototype

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

The node:cluster default export reported cluster instanceof EventEmitter === true through a special case, but its actual [[Prototype]] was still Object.prototype. As a result, Object.getPrototypeOf(cluster) === EventEmitter.prototype returned false, unlike Node.

This routes every cluster default-export lookup through the cached cluster singleton, materializes the canonical EventEmitter.prototype under runtime handles, and records it as the singleton's prototype once. Cluster event-method chaining keeps returning the same cached object.

Refs #9202.

Validation:

  • cluster::cluster_default_prototype_tests::default_export_inherits_from_event_emitter
  • full perry-runtime suite: 3,243 passed, 4 ignored; doc tests 8 ignored
  • ./run_parity_tests.sh --suite node-suite --filter cluster/exports/prototypes: 1/1 passed against Node 26.5.1
  • ./scripts/run_lint_gates.sh: all 64 gates passed, 2 CI-only skipped

Summary by CodeRabbit

  • Bug Fixes
    • Fixed the node:cluster default export so it inherits from the canonical EventEmitter prototype.
    • Improved compatibility with reflective prototype checks while preserving existing cluster event behavior.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 1d2d58e7-ce16-4d9d-b900-88c9ebc3010c

📥 Commits

Reviewing files that changed from the base of the PR and between 504e180 and 4f49591.

📒 Files selected for processing (3)
  • changelog.d/9905-cluster-default-prototype.md
  • crates/perry-runtime/src/cluster.rs
  • crates/perry-runtime/src/object/native_module.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The node:cluster CommonJS default export now uses a cached namespace whose prototype is events.EventEmitter.prototype. A regression test verifies the prototype relationship.

Changes

Cluster default prototype

Layer / File(s) Summary
Default prototype wiring and validation
crates/perry-runtime/src/cluster.rs, crates/perry-runtime/src/object/native_module.rs, changelog.d/9905-cluster-default-prototype.md
cluster_default_value() roots the namespace and EventEmitter, assigns the EventEmitter prototype when needed, and the CommonJS export uses this value. A test verifies the prototype relationship, and the changelog records the fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4f495

The cluster default export now matches Node’s EventEmitter prototype behavior while retaining its cached singleton. The targeted regression coverage supports merge readiness.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: linking the node:cluster default export to the EventEmitter prototype.
Description check ✅ Passed The description explains the problem, implementation, related issue, and validation results. It does not use every template heading, but it provides the required information and is sufficiently comple…
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main via merge train #9922. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,962 tests, 0 failures). Thanks!

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.

1 participant