Fixes #79 Resolved ERR_MODULE_NOT_FOUND during Vitest ESM resolution by adding explicit .js extensions to ESM imports.#106
Conversation
…M import issue - Created comprehensive documentation (WALLET_COMPONENT_FIX.md) explaining the ESM import issue - Added fix-imports-script.mjs to automatically add .js extensions to imports - Script successfully fixes module resolution in strict Node.js ESM environments This addresses the ERR_MODULE_NOT_FOUND error when using wallet-svelte-component in Vitest and other strict ESM contexts. The fix needs to be applied upstream to ergo-basics/wallet-svelte-component. This documentation provides detailed instructions for creating a PR.
WalkthroughAdds a new Node.js post-build script that recursively scans a distribution directory and rewrites relative import/export paths to include explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fix-imports-script.mjs (1)
56-63: Consider preserving original quote style for consistency.Patterns 1 and 4 normalize all imports to single quotes via template literals, which changes the original code style. While functional, preserving the original quote style would maintain code consistency.
This is a minor style consideration—if consistency with the original code is preferred, you could apply similar quote detection logic as suggested for Patterns 2 and 3.
Also applies to: 85-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
NEXT_STEPS.md(1 hunks)SUMMARY.md(1 hunks)WALLET_COMPONENT_FIX.md(1 hunks)fix-imports-script.mjs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
SUMMARY.md
[style] ~115-~115: Consider using a different verb for a more formal wording.
Context: ... # This file ``` ## 🎯 The Issue to Be Fixed Upstream Repository: https://githu...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
NEXT_STEPS.md
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
117-117: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (13)
fix-imports-script.mjs (4)
1-24: LGTM! Clear setup with flexible directory configuration.The script setup is well-structured with proper imports, configurable dist directory, and progress tracking. The default path targets the typical node_modules location, and the CLI argument allows overriding it for different use cases.
26-45: LGTM! Robust directory traversal with proper error handling.The recursive directory processing is well-implemented with efficient
withFileTypes: trueusage and graceful error handling that allows the script to continue if individual directories fail.
94-104: LGTM! Efficient file writing with proper tracking.The conditional write optimization avoids unnecessary disk I/O, and the progress logging with counters provides good visibility into the script's operations.
106-116: LGTM! Clean script execution with proper error handling.The IIFE pattern is appropriate for top-level async execution, and the error handling with exit code 1 ensures the script properly signals failures in CI/CD pipelines.
WALLET_COMPONENT_FIX.md (5)
1-32: LGTM! Clear problem statement with concrete examples.The issue description effectively communicates the problem with specific import paths and error messages, making it easy for developers to understand and diagnose the issue.
33-46: LGTM! Accurate root cause analysis.The explanation correctly identifies the ESM strict mode requirements and the appropriate location for the fix.
64-99: Documentation script is simpler and avoids the quote-handling bug.Interestingly, the simplified script in this documentation avoids the quote-handling bug present in the actual
fix-imports-script.mjs(Patterns 2 and 3) by using straightforward template string replacements. While this normalizes all quotes to single quotes, it's more robust than the substring-based approach in the actual script.Note: Once the bug in
fix-imports-script.mjsis fixed, both approaches will be valid—the actual script preserves quote style while this simplified version normalizes to single quotes.
116-141: LGTM! Clear testing and PR workflow guidance.The testing steps and PR creation instructions provide a complete workflow for applying and verifying the fix upstream.
142-181: LGTM! Comprehensive testing guidance and helpful references.The testing section and documentation references provide developers with both practical commands and deeper understanding of ESM requirements.
NEXT_STEPS.md (2)
1-81: LGTM! Clear actionable steps for upstream fix.The step-by-step guide provides a complete workflow from forking through building, with appropriate Windows-specific commands for the developer's environment.
141-170: LGTM! Helpful alternative and clear cross-references.The quick local fix option and references to detailed documentation provide flexibility for different workflows. The git status example helps developers verify their current state.
SUMMARY.md (2)
1-104: LGTM! Comprehensive summary with clear status tracking.The summary effectively documents what was accomplished and provides multiple authentication solutions for the Git push issue. The mention of "fixed 8 imports across 4 files" validates that the script was successfully tested locally.
105-144: LGTM! Well-organized summary with clear rationale.The file inventory, issue summary, and explanation of the fix's importance provide excellent context for reviewers and future maintainers.
|
|
||
| 1. Go to your fork: `https://github.com/ankitrraj/wallet-svelte-component` | ||
| 2. Click "Pull Requests" → "New Pull Request" | ||
| 3. Select: | ||
| - **base repository:** `ergo-basics/wallet-svelte-component` | ||
| - **base branch:** `main` (or whatever their default is) | ||
| - **head repository:** `ankitrraj/wallet-svelte-component` | ||
| - **compare branch:** `fix/add-js-extensions-to-esm-imports` | ||
| 4. Title: **"fix: Add .js extensions to ESM imports for strict Node.js compatibility"** | ||
| 5. Description: Use the content from `WALLET_COMPONENT_FIX.md` sections | ||
|
|
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code block at line 107 is missing a language identifier, which affects syntax highlighting and accessibility tools.
As per static analysis hints, apply this diff:
-```
+```bash
git add .
git commit -m "fix: Add .js extensions to ESM imports for strict Node.js compatibility
...
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
117-117: Bare URL used
(MD034, no-bare-urls)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In NEXT_STEPS.md around lines 107 to 117 the fenced code block is missing a
language identifier which prevents proper syntax highlighting; update the
opening fence to include the language (e.g., change "" to "bash") and
ensure the matching closing fence remains present so the block renders correctly
and accessibility/syntax tools recognize it.
</details>
<!-- fingerprinting:phantom:poseidon:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
- Fixed hardcoded single quotes to detect and use correct quote style (single or double) - Ensures script works correctly regardless of quote style in source files - Addresses CodeRabbit critical security issue
Keeping only the fix script as reference for upstream PR
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
fix-imports-script.mjs (3)
65-74: Quote detection logic is fragile but likely sufficient for typical dist files.The quote detection on line 70 addresses the previous review concern but uses a heuristic that could fail if the matched text contains quotes in other contexts (e.g.,
export { "default" as foo } from './path'). However, for typical compiled dist files, this should work in practice.For a more robust solution, consider capturing the quote character directly in the regex:
// Pattern 2: export { ... } from './path' content = content.replace( - /export\s+{[^}]+}\s+from\s+['"](\.[^'"]+)(?<!\.js|\.svelte|\.json)['"]/g, - (match, path) => { + /export\s+{[^}]+}\s+from\s+(['"])(\.[^'"]+)(?<!\.js|\.svelte|\.json)\1/g, + (match, quote, path) => { fileImportCount++; - const quote = match.includes('"') && match.lastIndexOf('"') > match.lastIndexOf("'") ? '"' : "'"; - const exportPart = match.substring(0, match.lastIndexOf(quote)); - return `${exportPart}${quote}${path}.js${quote}`; + return match.replace(new RegExp(`${quote}${path}${quote}$`), `${quote}${path}.js${quote}`); } );This captures the opening quote and uses
\1to ensure the closing quote matches.
76-85: Quote detection logic has the same fragility as Pattern 2.The quote detection on line 81 uses the same heuristic approach as Pattern 2. While it addresses the previous review concern, it could fail in edge cases. For typical dist files, this should work adequately.
For consistency, apply the same regex-based quote capture suggested for Pattern 2:
// Pattern 3: import ... from './path' content = content.replace( - /import\s+.*?from\s+['"](\.[^'"]+)(?<!\.js|\.svelte|\.json)['"]/g, - (match, path) => { + /import\s+.*?from\s+(['"])(\.[^'"]+)(?<!\.js|\.svelte|\.json)\1/g, + (match, quote, path) => { fileImportCount++; - const quote = match.includes('"') && match.lastIndexOf('"') > match.lastIndexOf("'") ? '"' : "'"; - const importPart = match.substring(0, match.lastIndexOf(quote)); - return `${importPart}${quote}${path}.js${quote}`; + return match.replace(new RegExp(`${quote}${path}${quote}$`), `${quote}${path}.js${quote}`); } );
58-58: Overly cautious negative lookbehind for dist context.The negative lookbehind
(?<!\.js|\.svelte|\.json)includes.svelte, but Svelte files shouldn't exist in a compiled dist directory. This is harmless but suggests the regex might have been designed for a broader context than post-build dist files.Also applies to: 67-67, 78-78, 89-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fix-imports-script.mjs(1 hunks)
🔇 Additional comments (4)
fix-imports-script.mjs (4)
29-45: LGTM!The recursive directory traversal logic is clean and appropriate. Error handling allows the script to continue processing remaining files if one fails, which is correct behavior for a build script.
97-102: LGTM!The write optimization (only writing when changes occur) is good practice and avoids unnecessary file system operations. The logging provides clear feedback on what was fixed.
108-118: LGTM!The main execution logic is clean and follows best practices for Node.js scripts, including proper error handling and exit codes.
50-106: Consider whether dynamic imports need handling.The script handles static imports and re-exports but doesn't process dynamic imports like
import('./path'). If wallet-svelte-component's dist files contain dynamic imports without extensions, they would also fail ESM resolution.Verify whether the package uses dynamic imports by checking the dist directory for patterns like
import('...')orimport("..."). If found, add this pattern to handle them:// Pattern 5: import('./path') content = content.replace( /import\s*\(\s*['"](\.[^'"]+)(?<!\.js|\.svelte|\.json)['"]\s*\)/g, (match, path) => { fileImportCount++; return `import('${path}.js')`; } );
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| // Get dist directory from command line argument or use default | ||
| const distDir = process.argv[2] || join(__dirname, 'node_modules', 'wallet-svelte-component', 'dist'); |
There was a problem hiding this comment.
Architectural concern: Patching node_modules is not sustainable.
The default path targets node_modules/wallet-svelte-component/dist, meaning this script modifies an installed dependency. Changes to node_modules are lost on reinstall, requiring this script to run after every npm install.
Consider these more maintainable alternatives:
- File an issue/PR with the upstream
wallet-svelte-componentpackage to ship with proper ESM imports - Use patch-package to create a persistent patch
- Fork the package and publish a fixed version
- Use a bundler that handles this automatically (e.g., Vite, esbuild)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fix-imports-script.mjs (1)
19-23: Defaulting tonode_modules/.../distbakes in a fragile workflow
This still encourages patching installed deps as a default behavior (lost on reinstall / lockfile churn). Consider making the dist dir required (or default to your own package’s dist) and pass the dependency path explicitly in CI when needed.
| async function resolveImportPath(importPath, fileDir) { | ||
| // Skip if already has extension | ||
| if (importPath.match(/\.(js|svelte|json)$/)) { | ||
| return importPath; | ||
| } | ||
|
|
||
| // Remove leading './' | ||
| const relativePath = importPath.replace(/^\.\//, ''); | ||
| const fullPath = join(fileDir, relativePath); | ||
|
|
||
| // Try path + .js (file) | ||
| if (await pathExists(fullPath + '.js')) { | ||
| return importPath + '.js'; | ||
| } | ||
|
|
||
| // Try path + /index.js (directory with index) | ||
| if (await pathExists(join(fullPath, 'index.js'))) { | ||
| return importPath + '/index.js'; | ||
| } | ||
|
|
||
| // Default to .js if neither exists | ||
| return importPath + '.js'; | ||
| } |
There was a problem hiding this comment.
Handle ../ imports and don’t corrupt non-JS asset imports
Right now you only match ./... and you only treat .js|.svelte|.json as “already has extension”. That can (a) miss valid ../... relative imports and (b) incorrectly rewrite import './styles.css' into ./styles.css.js.
async function resolveImportPath(importPath, fileDir) {
- // Skip if already has extension
- if (importPath.match(/\.(js|svelte|json)$/)) {
+ // Skip if it already has *any* file extension (e.g. .css, .svg, .wasm, .map, ...)
+ // This script is only meant to fix extensionless ESM specifiers.
+ if (/\.[A-Za-z0-9]+$/.test(importPath)) {
return importPath;
}
- // Remove leading './'
- const relativePath = importPath.replace(/^\.\//, '');
- const fullPath = join(fileDir, relativePath);
+ // Keep './' and '../' semantics intact; `join` handles both.
+ const fullPath = join(fileDir, importPath); const patterns = [
// Pattern 1: export * from './path'
- /export\s+\*\s+from\s+(['"])(\.\/[^'"]+)\1/g,
+ /export\s+\*\s+from\s+(['"])(\.\.?(?:\/[^'"]+)+)\1/g,
// Pattern 2: export { ... } from './path'
- /export\s+{[^}]+}\s+from\s+(['"])(\.\/[^'"]+)\1/g,
+ /export\s+{[^}]+}\s+from\s+(['"])(\.\.?(?:\/[^'"]+)+)\1/g,
// Pattern 3: import ... from './path'
- /import\s+[^'"]+from\s+(['"])(\.\/[^'"]+)\1/g,
+ /import\s+[^'"]+from\s+(['"])(\.\.?(?:\/[^'"]+)+)\1/g,
// Pattern 4: import './path'
- /import\s+(['"])(\.\/[^'"]+)\1/g
+ /import\s+(['"])(\.\.?(?:\/[^'"]+)+)\1/g
];- // Skip if already has extension
- if (!path.match(/\.(js|svelte|json)$/)) {
+ // Skip if it already has any extension
+ if (!/\.[A-Za-z0-9]+$/.test(path)) {
matches.push({
fullMatch: match[0],
quote,
path,
index: match.index
});
}Also applies to: 98-107, 115-117
🤖 Prompt for AI Agents
In fix-imports-script.mjs around lines 63-85 (also update analogous logic at
98-107 and 115-117): the resolver only strips a leading "./" and only treats
.js|.svelte|.json as valid extensions, so it misses "../" relative imports and
corrupts imports with other extensions (e.g. .css). Change the leading-path
handling to accept and preserve both "./" and "../" prefixes (don't always
remove just "./"), detect any existing extension generically (use a regex like
/\.[^\/]+$/ to mean “has an extension”) and return the original importPath
unchanged if it already has an extension, and only attempt to append ".js" or
"/index.js" for paths without any extension after checking the filesystem;
ensure returned paths preserve the original relative prefix (./ or ../) and
apply the same fixes to the other occurrences at the stated line ranges.
| // Sort by index in reverse to replace from end to start | ||
| matches.sort((a, b) => b.index - a.index); | ||
|
|
||
| // Process each match | ||
| for (const { fullMatch, quote, path } of matches) { | ||
| const newPath = await resolveImportPath(path, fileDir); | ||
| const newStatement = fullMatch.replace(path, newPath); | ||
| content = content.replace(fullMatch, newStatement); | ||
| fileImportCount++; | ||
| } |
There was a problem hiding this comment.
Bug: content.replace(fullMatch, ...) can miss fixes when statements repeat
Because .replace(string, ...) only replaces the first occurrence, identical fullMatch strings elsewhere in the file may remain unchanged (and fileImportCount becomes misleading). Use the stored index and splice the string (you already sort descending, which makes this safe).
- for (const { fullMatch, quote, path } of matches) {
+ for (const { fullMatch, quote, path, index } of matches) {
const newPath = await resolveImportPath(path, fileDir);
const newStatement = fullMatch.replace(path, newPath);
- content = content.replace(fullMatch, newStatement);
- fileImportCount++;
+ if (newStatement !== fullMatch) {
+ content =
+ content.slice(0, index) +
+ newStatement +
+ content.slice(index + fullMatch.length);
+ fileImportCount++;
+ }
}🤖 Prompt for AI Agents
In fix-imports-script.mjs around lines 127 to 136, using
content.replace(fullMatch, ...) only replaces the first identical occurrence and
can leave other identical matches unchanged; instead use the stored match.index
(you already sorted descending) to splice the newStatement into content at that
exact index and remove the original fullMatch length, increment fileImportCount
for each successful splice, and avoid using String.replace so repeated identical
statements are reliably updated.
3 participants
Fixes #79
Resolved ERR_MODULE_NOT_FOUND during Vitest ESM resolution
by adding explicit .js extensions to ESM imports.
the resolve complete
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.