fix: normalize "." and ".." in resolved module paths to dedupe modules - #1977
Conversation
Sorry, something went wrong.
ResolveModuleCallback builds the on-disk path for a relative specifier without collapsing "." and ".." segments, so the same file imported as "./x" from /a/b and as "../x" from /a/b/c lands under two different registry keys. stat() resolves the segments so both locate the file, but the differing keys make V8 compile and evaluate the module twice: two instances with separate state, and import.meta.dirname becomes "/a/b/c/.." for the ".." spelling. Normalize the resolved path before it becomes the registry key, the same way the HTTP branch canonicalizes through CanonicalizeHttpUrlKey. The pass is lexical and runs for every on-disk candidate kind, and is a no-op for already-canonical paths. Adds a test-app spec that imports one module as "./counter.mjs" and "../counter.mjs" and asserts a single shared instance.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 322d5fbf-a6ab-471f-be3f-393ae43c2a92 📥 CommitsReviewing files that changed from the base of the PR and between 92c2654 and e4e84cd. 📒 Files selected for processing (5)
📝 Walkthrough WalkthroughAdds NormalizeDotSegments canonicalization to ResolveModuleCallback in ModuleInternalCallbacks.cpp so that paths with ./ or ../ segments resolve to the same canonical key. Three new fixture modules (counter.mjs, viaSameDir.mjs, nested/viaParentDir.mjs) and a Jasmine spec verify that two imports using different relative paths share one module instance. ChangesESM Deduplication Fix and Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
|
@NathanWalker, (and @coderabbitai), I checked the iOS runtime and it doesn't need an equivalent of this change. Vanilla NativeScript/ios already strips . and .. out of a resolved module path before that path becomes the registry key, so the ./x vs ../x double-instantiation this PR fixes on Android can't happen there. It's all in ModuleInternalCallbacks.mm:
So canonicalization on iOS happens earlier and in more places than the single pass here, and both sides end up keying the registry by the same canonical path. No parity PR needed on iOS. |
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. @adrian-niculescu Thanks for the thorough cross-platform investigation. The breakdown is very clear:
Both approaches converge on the same canonical path as the registry key, so ./x and ../x correctly deduplicate on both platforms. No iOS follow-up needed — noted. ✏️ Learnings added Learnt from: adrian-niculescu URL: https://github.com/NativeScript/android/pull/1977 Timestamp: 2026-06-24T11:37:48.132Z Learning: In NativeScript, the iOS runtime (`NativeScript/ios`) already prevents `./x` vs `../x` module double-instantiation without any additional fix. `NormalizePath()` in `NativeScript/runtime/ModuleInternalCallbacks.mm` (around L50-L66) uses `stringByStandardizingPath` which canonicalizes `.` and `..` segments by consulting the real filesystem (folds symlinks too, not just a lexical rewrite). This normalization is applied at multiple call sites: candidate base during resolution (~L1161), joined relative path when building candidates (~L967-976), and the final `absPath` just before it is used as the `g_moduleRegistry` key (~L1317). The Android PR `#1977` achieves the same dedup via a single lexical `NormalizeDotSegments` pass on the confirmed on-disk path before it becomes the registry key. No parity PR is needed on iOS.
|
Sorry, something went wrong.
#1977) ResolveModuleCallback builds the on-disk path for a relative specifier without collapsing "." and ".." segments, so the same file imported as "./x" from /a/b and as "../x" from /a/b/c lands under two different registry keys. stat() resolves the segments so both locate the file, but the differing keys make V8 compile and evaluate the module twice: two instances with separate state, and import.meta.dirname becomes "/a/b/c/.." for the ".." spelling. Normalize the resolved path before it becomes the registry key, the same way the HTTP branch canonicalizes through CanonicalizeHttpUrlKey. The pass is lexical and runs for every on-disk candidate kind, and is a no-op for already-canonical paths.
ResolveModuleCallback builds the on-disk path for a relative specifier without collapsing . and .. segments, so the same file imported as ./x from /a/b and as ../x from /a/b/c lands under two different registry keys. stat() resolves the segments so both locate the file, but the differing keys make V8 compile and evaluate the module twice: two instances with separate module state, broken cyclic resolution, and an import.meta.dirname of /a/b/c/.. for the .. spelling.
This normalizes the resolved path before it becomes the registry key, the same way the HTTP branch already canonicalizes through CanonicalizeHttpUrlKey. The pass is lexical (matching how specifiers are joined, not a realpath) and applies to every on-disk candidate kind; it is a no-op for already-canonical paths.
Test: a test-app spec imports one module as ./counter.mjs and ../counter.mjs and asserts they share one instance (===, and a counter incremented to 2). It fails before the change and passes after.
Summary by CodeRabbit