wip(events): eventemitter3 native-EventEmitter link investigation (#5140)#5172
Merged
Conversation
…5140) PARTIAL — not yet a complete fix. Investigation of the eventemitter3 link failure (undefined _js_event_emitter_* symbols) found three layers; this commit addresses two of them: 1. Routing: a program that constructs a native EventEmitter *by name* (e.g. eventemitter3's default export, local binding `EventEmitter`) now marks the build as using events — detected in collect_modules via the lowered `class_name: "EventEmitter"` token (ctx.uses_event_emitter, new field in types.rs, threaded into the optimized-libs cache key) — and inserts "events" into native_module_imports so the full node:events wiring fires (well-known archive routing + bundled-events + external-events-construct), exactly as a real `import 'events'` would. 2. Symbol retention: perry-ext-events is a non-tokio wrapper, so the auto-optimize linker uses its prebuilt standalone staticlib. Thin-LTO was internalizing and dropping the crate's entire #[no_mangle] C API from that staticlib (verified: lto=false restores the symbols; a #[used] address-anchor does NOT beat thin-LTO here). Mirroring the UI crates' profile (strip=false + codegen-units=16) keeps the exported symbols in target/release/libperry_ext_events.a. REMAINING (3rd layer, unsolved): even with (1) and (2), the eventemitter3 repro still fails to link. The well-known events archive IS resolved and added to OptimizedLibs.well_known_libs (the 'routing events -> libperry_ext_events.a' message prints), and the archive now contains the symbols, yet for a compilePackages program the archive does NOT appear on the final cc link line (confirmed via a cc shim: 1 occurrence for a real `import 'events'` program, 0 for the eventemitter3 program). So well_known_libs is being dropped from the link composition specifically on the compilePackages path. That last hop still needs tracing. No changelog/version bump.
📝 WalkthroughWalkthroughRefactors optional-feature detection in Perry by centralizing scattered HIR analysis logic into a new ChangesEventEmitter support and feature detection refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Around line 1896-1907: The implicit EventEmitter detection block sets
ctx.uses_event_emitter = true and inserts "events" into
ctx.native_module_imports, but it does not set ctx.needs_stdlib = true like
other implicit stdlib detections do. This causes link paths that gate
stdlib/well-known archives on needs_stdlib to be missing the events module. Add
ctx.needs_stdlib = true; in the same conditional block where
ctx.uses_event_emitter is set to true to ensure the stdlib is properly flagged
when EventEmitter usage is detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83bc2c98-8241-49e1-b5b2-ebfa97e1cda4
📒 Files selected for processing (4)
Cargo.tomlcrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/types.rs
Comment on lines
+1896
to
+1907
| let hir_debug: String = format!("{:?}{:?}", &hir_module.init, &hir_module.functions); | ||
| if hir_debug.contains("class_name: \"EventEmitter\"") | ||
| || hir_debug.contains("class_name: \"EventEmitterAsyncResource\"") | ||
| { | ||
| ctx.uses_event_emitter = true; | ||
| // Treat native EventEmitter use exactly like a `node:events` import | ||
| // so the full events wiring fires: the perry-ext-events well-known | ||
| // archive (which defines `js_event_emitter_*`) is linked, the | ||
| // `bundled-events` feature is enabled, and the construct dispatcher | ||
| // is registered (`external-events-construct`). Idempotent — a set. | ||
| ctx.native_module_imports.insert("events".to_string()); | ||
| } |
There was a problem hiding this comment.
Set needs_stdlib when implicit EventEmitter detection fires.
Line 1900 enables EventEmitter routing, but this block never sets ctx.needs_stdlib = true like other implicit stdlib detections. That can leave link paths that gate stdlib/well-known archives on needs_stdlib without events, which matches the unresolved compilePackages missing-archive behavior.
Suggested fix
if hir_debug.contains("class_name: \"EventEmitter\"")
|| hir_debug.contains("class_name: \"EventEmitterAsyncResource\"")
{
+ ctx.needs_stdlib = true;
ctx.uses_event_emitter = true;
// Treat native EventEmitter use exactly like a `node:events` import
// so the full events wiring fires: the perry-ext-events well-known
// archive (which defines `js_event_emitter_*`) is linked, the
// `bundled-events` feature is enabled, and the construct dispatcher📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let hir_debug: String = format!("{:?}{:?}", &hir_module.init, &hir_module.functions); | |
| if hir_debug.contains("class_name: \"EventEmitter\"") | |
| || hir_debug.contains("class_name: \"EventEmitterAsyncResource\"") | |
| { | |
| ctx.uses_event_emitter = true; | |
| // Treat native EventEmitter use exactly like a `node:events` import | |
| // so the full events wiring fires: the perry-ext-events well-known | |
| // archive (which defines `js_event_emitter_*`) is linked, the | |
| // `bundled-events` feature is enabled, and the construct dispatcher | |
| // is registered (`external-events-construct`). Idempotent — a set. | |
| ctx.native_module_imports.insert("events".to_string()); | |
| } | |
| let hir_debug: String = format!("{:?}{:?}", &hir_module.init, &hir_module.functions); | |
| if hir_debug.contains("class_name: \"EventEmitter\"") | |
| || hir_debug.contains("class_name: \"EventEmitterAsyncResource\"") | |
| { | |
| ctx.needs_stdlib = true; | |
| ctx.uses_event_emitter = true; | |
| // Treat native EventEmitter use exactly like a `node:events` import | |
| // so the full events wiring fires: the perry-ext-events well-known | |
| // archive (which defines `js_event_emitter_*`) is linked, the | |
| // `bundled-events` feature is enabled, and the construct dispatcher | |
| // is registered (`external-events-construct`). Idempotent — a set. | |
| ctx.native_module_imports.insert("events".to_string()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry/src/commands/compile/collect_modules.rs` around lines 1896 -
1907, The implicit EventEmitter detection block sets ctx.uses_event_emitter =
true and inserts "events" into ctx.native_module_imports, but it does not set
ctx.needs_stdlib = true like other implicit stdlib detections do. This causes
link paths that gate stdlib/well-known archives on needs_stdlib to be missing
the events module. Add ctx.needs_stdlib = true; in the same conditional block
where ctx.uses_event_emitter is set to true to ensure the stdlib is properly
flagged when EventEmitter usage is detected.
…er 2000-line cap) The #5140 EventEmitter-detection block pushed collect_modules.rs to 2023 lines, over the 2000-line lint cap. Move the whole optional-feature detection sequence (fetch/wasm/crypto/regex/temporal/events/url/normalize/ diagnostics/dgram/readline/ioredis) into a new collect_modules/feature_detect.rs submodule and call it from collect_module_finish.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/perry/src/commands/compile/collect_modules/feature_detect.rs (1)
125-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
ctx.needs_stdlib = truewhen EventEmitter detection fires.This block sets
ctx.uses_event_emitterand inserts"events"intonative_module_imports, but unlike other implicit stdlib detections (crypto at line 67, readline at line 216, ioredis at line 238), it does not setctx.needs_stdlib = true. Per theuses_event_emitterdocumentation intypes.rs, thejs_event_emitter_*helpers live in perry-stdlib. Without this flag, link paths gating stdlib/well-known archives onneeds_stdlibmay exclude the events archive entirely—matching the unresolved compilePackages missing-archive behavior described in the PR objectives.Suggested fix
{ let hir_debug: String = format!("{:?}{:?}", &hir_module.init, &hir_module.functions); if hir_debug.contains("class_name: \"EventEmitter\"") || hir_debug.contains("class_name: \"EventEmitterAsyncResource\"") { + ctx.needs_stdlib = true; ctx.uses_event_emitter = true; // Treat native EventEmitter use exactly like a `node:events` import // so the full events wiring fires: the perry-ext-events well-known // archive (which defines `js_event_emitter_*`) is linked, the // `bundled-events` feature is enabled, and the construct dispatcher // is registered (`external-events-construct`). Idempotent — a set. ctx.native_module_imports.insert("events".to_string()); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/collect_modules/feature_detect.rs` around lines 125 - 137, The EventEmitter detection block sets ctx.uses_event_emitter and inserts "events" into ctx.native_module_imports, but fails to set ctx.needs_stdlib = true as required by other implicit stdlib detections (crypto, readline, ioredis). Since js_event_emitter_* helpers live in perry-stdlib, add ctx.needs_stdlib = true in the conditional block where uses_event_emitter is set to ensure the events archive is properly linked and not excluded due to missing stdlib requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/perry/src/commands/compile/collect_modules/feature_detect.rs`:
- Around line 125-137: The EventEmitter detection block sets
ctx.uses_event_emitter and inserts "events" into ctx.native_module_imports, but
fails to set ctx.needs_stdlib = true as required by other implicit stdlib
detections (crypto, readline, ioredis). Since js_event_emitter_* helpers live in
perry-stdlib, add ctx.needs_stdlib = true in the conditional block where
uses_event_emitter is set to ensure the events archive is properly linked and
not excluded due to missing stdlib requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59b0ea94-ed8d-49f2-8fc1-cc0a464b3648
📒 Files selected for processing (2)
crates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/feature_detect.rs
This was referenced Jun 15, 2026
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
1 participant
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.
Re #5140. Partial / WIP — not yet a complete fix, pushed to preserve the investigation.
What's done
Two of the three root causes of the eventemitter3 link failure (
undefined _js_event_emitter_*) are addressed:Routing — a program that constructs a native
EventEmitterby name (eventemitter3's default export binds localEventEmitter, routed through the builtin-new path that emits directjs_event_emitter_*externs) now marks the build as using events. Detected incollect_modulesfrom the loweredclass_name: "EventEmitter"token → newctx.uses_event_emitterflag (threaded into the auto-optimize cache key) → inserts"events"intonative_module_imports, so the fullnode:eventswiring fires (well-known archive routing +bundled-events+external-events-construct), identical to a realimport 'events'.Symbol retention — perry-ext-events is a non-tokio wrapper, so the auto-optimize linker uses its prebuilt standalone staticlib. Thin-LTO was internalizing and dropping the crate's entire
#[no_mangle]C API from that.a(verified:lto=falserestores them; a#[used]address anchor does not beat thin-LTO here). Mirroring the UI crates' profile (strip=false+codegen-units=16) keeps the exported symbols inlibperry_ext_events.a.What remains (the blocker)
Even with (1)+(2), the eventemitter3 repro still fails to link. The events archive is resolved and added to
OptimizedLibs.well_known_libs(therouting events -> libperry_ext_events.alog prints) and now contains the symbols — yet for a compilePackages program the archive never reaches the finalcclink line. Confirmed with accshim: a realimport 'events'program has the archive on its link line (1 occurrence) and links fine; the eventemitter3 program has 0. Sowell_known_libsis being dropped from the link composition specifically on the compilePackages path. That last hop still needs tracing — likely a second compile/link pass for compilePackages that recomputes or discardswell_known_libs.A real
import 'events'program (regression check) builds and runs correctly with these changes.No changelog/version bump.
Summary by CodeRabbit