Skip to content

fix(perf_hooks): align default import export keys - #9885

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9202-native-default-export-keys
Closed

fix(perf_hooks): align default import export keys#9885
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9202-native-default-export-keys

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

The node-suite/globals/native-module-default-export-keys row in #9202 fails because Perry binds the node:perf_hooks default import to the ESM namespace object, whose enumerable default export does not exist on Node's CommonJS module.exports value.

Create the default import as a cached perf_hooks.default namespace with the CommonJS key surface, while keeping import * as hooks on the ESM namespace. Member reads and calls route back to the base module, preserving hooks.default.performance === hooks.performance and the existing constants object behavior.

Validation:

  • node-suite/globals/native-module-default-export-keys: 1/1 parity pass against Node 26.5.1
  • all four node-suite/perf_hooks/imports fixtures: exact Node output
  • perry-dispatch and perry-hir suites: pass
  • cargo test -p perry-runtime -- --test-threads=1: 3,227 passed, 4 ignored; doc tests green
  • scripts/run_lint_gates.sh: all 64 gates passed; 2 CI-expression gates skipped locally

No version bump.

Addresses node-suite/globals/native-module-default-export-keys in #9202.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed node:perf_hooks default imports to match Node.js behavior.
    • Preserved access to the shared default namespace and consistent export identity.
    • Corrected property access and key enumeration for perf_hooks.default, including performance and constants.

@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: 6521c44b-b0c0-49ae-87da-d650b7dadeba

📥 Commits

Reviewing files that changed from the base of the PR and between 890514a and fcc9688.

📒 Files selected for processing (6)
  • changelog.d/9885-perf-hooks-default-export-keys.md
  • crates/perry-dispatch/src/cjs_default_modules.rs
  • crates/perry-hir/src/lower/tests.rs
  • crates/perry-runtime/src/object/native_module.rs
  • crates/perry-runtime/src/object/native_module/module_keys.rs
  • crates/perry-runtime/src/object/native_module_dispatch.rs

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


📝 Walkthrough

Walkthrough

The change maps node:perf_hooks default imports to a cached perf_hooks.default namespace. It preserves shared performance and constants objects, exposes the default namespace keys, and adds lowering and dispatch coverage.

Changes

perf_hooks default namespace compatibility

Layer / File(s) Summary
Default namespace mapping and lowering coverage
crates/perry-dispatch/src/cjs_default_modules.rs, crates/perry-hir/src/lower/tests.rs
The CommonJS default-module table includes perf_hooks. A lowering test verifies that the default import reads the native module's default property.
Runtime namespace identity and property resolution
crates/perry-runtime/src/object/native_module.rs
perf_hooks.default now uses the shared CJS default namespace path. The namespace is cached, and its performance and constants properties resolve to the existing shared objects.
Enumerable keys and regression coverage
crates/perry-runtime/src/object/native_module/module_keys.rs, crates/perry-runtime/src/object/native_module_dispatch.rs, changelog.d/9885-perf-hooks-default-export-keys.md
The default namespace exposes the perf_hooks export keys. Dispatch tests include perf_hooks.default, and the changelog records the compatibility fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to fcc96

This change makes node:perf_hooks default imports expose a Node-compatible cached namespace while preserving shared exports and existing namespace behavior. The implementation and targeted validation indicate no remaining merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 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 identifies the main change: aligning perf_hooks default import export keys with expected behavior.
Description check ✅ Passed The description explains the issue, implementation, affected behavior, related issue, validation results, and version-bump status. It omits the template headings and checklist, but the required inform…
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 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 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 #9898. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,939 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