fix(compile): close Ajv CJS source and hoist gaps - #7068
Conversation
|
@coderabbitai review |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe compiler detects hybrid CommonJS/ESM TypeScript inputs, keeps affected packages on published JavaScript entries, and preserves safe later-declared helpers when hoisting CommonJS classes. Resolver, unit, end-to-end, and real-library coverage was updated. ChangesCJS emit and hoisting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PackageResolver
participant SourceMapResolver
participant HybridCjsDetector
participant CjsClassHoister
participant PublishedJavaScript
PackageResolver->>SourceMapResolver: recover package original source
SourceMapResolver->>HybridCjsDetector: inspect hybrid CJS/ESM syntax
HybridCjsDetector-->>PackageResolver: identify CJS emit input
PackageResolver->>PublishedJavaScript: retain published JavaScript graph
CjsClassHoister->>CjsClassHoister: classify function captures and value usage
CjsClassHoister-->>PublishedJavaScript: emit safe helpers with hoisted classes
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/resolve.rs (1)
752-791: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider memoizing
is_hybrid_cjs_emit_input— it re-reads and re-strips the same files on every resolution.
resolve_package_source_entryrunspackage_entry_requires_cjs_emiton every call (including per-subpath), which reads the package entry, its source map, and the mapped TS source, then allocates a full stripped copy viastrip_comments_and_strings. The same paths are then re-checked at Lines 804/815/824/834/848. For a large package graph this multiplies file I/O and O(n) scans over identical inputs.A small path→bool cache (e.g.
OnceLock<Mutex<HashMap<PathBuf, bool>>>) keyed on the canonical path would keep behavior identical while collapsing the repeats.🤖 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/resolve.rs` around lines 752 - 791, Memoize the result of is_hybrid_cjs_emit_input by canonical file path so repeated package resolution does not reread or restrip identical sources. Add a small shared path-to-boolean cache and use it inside is_hybrid_cjs_emit_input, preserving the existing false result when reading or canonicalizing a path fails and keeping package_entry_requires_cjs_emit behavior unchanged.
🤖 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/cjs_wrap/hoist_classes.rs`:
- Around line 286-304: Extend the unsafe_to_duplicate check in the
function-declaration loop to detect references to decl.name outside
decl.block_text when the identifier is used as a value, including member writes,
aliases, identity comparisons, and Object.defineProperty calls. Mark such
helpers unsafe and preserve the existing reassignment, export, and blocker
checks so only capture-free call references remain eligible for duplication.
---
Nitpick comments:
In `@crates/perry/src/commands/compile/resolve.rs`:
- Around line 752-791: Memoize the result of is_hybrid_cjs_emit_input by
canonical file path so repeated package resolution does not reread or restrip
identical sources. Add a small shared path-to-boolean cache and use it inside
is_hybrid_cjs_emit_input, preserving the existing false result when reading or
canonicalizing a path fails and keeping package_entry_requires_cjs_emit behavior
unchanged.
🪄 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: 4b1eb9a8-9c5d-4557-a9c7-1e7302e33d33
📒 Files selected for processing (9)
changelog.d/7068-cjs-ajv-source-routing.mdcrates/perry/src/commands/compile/cjs_wrap/detect.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/issue_6585_tests.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/declaration_map_source_tests.rscrates/perry/tests/issue_6559_real_libs_e2e.rscrates/perry/tests/issue_6585_cjs_class_forward_function.rs
|
Addressed the CodeRabbit performance nitpick in fcfc7a5 as well: hybrid-source classification is now memoized by canonical path, avoiding repeated reads and full-source stripping for each package subpath resolution. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs (1)
1031-1046: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full-source re-stripping per top-level function declaration.
identifier_used_as_value_outsidere-runsstrip_comments_and_strings(source)(Line 1041) on every call, and it's invoked once per entry infunction_declsinside the loop at Lines 286-310.collect_top_level_function_declsalready computed an equivalentstrippedsource once (Line 910) to find these declarations. For N top-level functions this means N (+1) full-source strips, each O(source length) — quadratic-ish overhead on exactly the kind of large CJS bundles (ajv, fast-json-stringify) this PR targets.Consider computing the stripped source once in
extract_top_level_class_declsand threading it through bothcollect_top_level_function_declsandidentifier_used_as_value_outside.♻️ Sketch: hoist the strip out of the per-decl loop
-fn identifier_used_as_value_outside( - source: &str, +fn identifier_used_as_value_outside( + stripped: &str, name: &str, excluded_start: usize, excluded_end: usize, ) -> bool { fn is_ident_byte(b: u8) -> bool { b == b'_' || b == b'$' || b.is_ascii_alphanumeric() } - let stripped = super::detect::strip_comments_and_strings(source); let bytes = stripped.as_bytes();And at the call site, compute
strippedonce before the loop (or havecollect_top_level_function_declsreturn it alongside the decls) and pass&strippedinto eachidentifier_used_as_value_outside(...)call instead ofsource.🤖 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/cjs_wrap/hoist_classes.rs` around lines 1031 - 1046, Compute the comment/string-stripped source once in extract_top_level_class_decls and reuse it through collect_top_level_function_decls and the per-declaration checks. Update identifier_used_as_value_outside to accept the precomputed stripped source rather than re-running strip_comments_and_strings on each call, while preserving its existing exclusion-range matching behavior.
🤖 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.
Nitpick comments:
In `@crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs`:
- Around line 1031-1046: Compute the comment/string-stripped source once in
extract_top_level_class_decls and reuse it through
collect_top_level_function_decls and the per-declaration checks. Update
identifier_used_as_value_outside to accept the precomputed stripped source
rather than re-running strip_comments_and_strings on each call, while preserving
its existing exclusion-range matching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ac17cf5-3c04-46f7-b91f-035e35285e08
📒 Files selected for processing (3)
crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/issue_6585_tests.rscrates/perry/src/commands/compile/resolve.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry/src/commands/compile/resolve.rs
1 participant
Summary
module.exportsepilogue on their published JavaScript package graphValidation
cargo fmt --all -- --checkgit diff --checkcargo test --profile perry-dev -p perry --bin perry commands::compile::cjs_wrap::tests -- --nocapture(100 passed before test-only module split)cargo test --profile perry-dev -p perry --bin perry commands::compile::cjs_wrap::issue_6585_tests -- --nocapture(3 passed)cargo test --profile perry-dev -p perry --bin perry commands::compile::resolve::declaration_map_source_tests -- --nocapture(10 passed)cargo test --profile perry-dev -p perry --test issue_6585_cjs_class_forward_function -- --nocapturecargo test --profile perry-dev -p perry --test issue_6586_namespace_cjs_default_import -- --nocapturePERRY_REQUIRE_NPM_E2E=1 cargo test --profile perry-dev -p perry --test issue_6559_real_libs_e2e real_fast_json_stringify_serializer -- --exact --nocaptureCloses #6585
Closes #6586
Refs #6559
Summary by CodeRabbit
module.exports.fast-json-stringifyserializer end-to-end test (when available).ajvtest skipping.