-
-
Notifications
You must be signed in to change notification settings - Fork 22
fix(build): preserve ESM imports in published package #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||||||||||||||||||||||||||||||||
| import { existsSync } from "node:fs" | ||||||||||||||||||||||||||||||||||||
| import { readdir, readFile, stat, writeFile } from "node:fs/promises" | ||||||||||||||||||||||||||||||||||||
| import path from "node:path" | ||||||||||||||||||||||||||||||||||||
| import { fileURLToPath } from "node:url" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..") | ||||||||||||||||||||||||||||||||||||
| const distDir = path.join(repoRoot, "dist") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const importPatterns = [ | ||||||||||||||||||||||||||||||||||||
| /(from\s*["'])(\.{1,2}\/[^"']+)(["'])/g, | ||||||||||||||||||||||||||||||||||||
| /(import\s*\(\s*["'])(\.{1,2}\/[^"']+)(["'])/g, | ||||||||||||||||||||||||||||||||||||
| /(\bimport\s*["'])(\.{1,2}\/[^"']+)(["'])/g, | ||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function hasExtension(specifier) { | ||||||||||||||||||||||||||||||||||||
| return path.posix.extname(specifier) !== "" | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function resolveSpecifier(filePath, specifier) { | ||||||||||||||||||||||||||||||||||||
| if (hasExtension(specifier)) return specifier | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const targetPath = path.resolve(path.dirname(filePath), specifier) | ||||||||||||||||||||||||||||||||||||
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | ||||||||||||||||||||||||||||||||||||
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | ||||||||||||||||||||||||||||||||||||
| return specifier | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent passthrough leaves broken specifiers unfixed without any signal. When neither Adding a 🛡️ Proposed fix function resolveSpecifier(filePath, specifier) {
if (hasExtension(specifier)) return specifier
const targetPath = path.resolve(path.dirname(filePath), specifier)
if (existsSync(`${targetPath}.js`)) return `${specifier}.js`
if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js`
+ console.warn(`[fix-esm-imports] Could not resolve specifier "${specifier}" in ${filePath} — leaving unchanged`)
return specifier
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| async function listJsFiles(dir) { | ||||||||||||||||||||||||||||||||||||
| const entries = await readdir(dir) | ||||||||||||||||||||||||||||||||||||
| const files = [] | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| for (const entry of entries) { | ||||||||||||||||||||||||||||||||||||
| const entryPath = path.join(dir, entry) | ||||||||||||||||||||||||||||||||||||
| const entryStat = await stat(entryPath) | ||||||||||||||||||||||||||||||||||||
| if (entryStat.isDirectory()) { | ||||||||||||||||||||||||||||||||||||
| files.push(...await listJsFiles(entryPath)) | ||||||||||||||||||||||||||||||||||||
| } else if (entry.endsWith(".js")) { | ||||||||||||||||||||||||||||||||||||
| files.push(entryPath) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return files | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| async function fixFile(filePath) { | ||||||||||||||||||||||||||||||||||||
| const source = await readFile(filePath, "utf8") | ||||||||||||||||||||||||||||||||||||
| let output = source | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| for (const pattern of importPatterns) { | ||||||||||||||||||||||||||||||||||||
| output = output.replace(pattern, (match, prefix, specifier, suffix) => { | ||||||||||||||||||||||||||||||||||||
| return `${prefix}${resolveSpecifier(filePath, specifier)}${suffix}` | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+49
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: script/fix-esm-imports.mjs
Line: 49-53
Comment:
**Stateful `/g` regex applied per-file in a loop** — `importPatterns` is a module-level array of regex literals all carrying the `/g` flag. `String.prototype.replace()` resets `lastIndex` to 0 before iterating, so this is safe in practice — but it is an easy trap if someone later changes the loop to use `.exec()` or `.test()`. Moving the regex construction inside `fixFile` (or inside `main` per-run) would eliminate the statefulness entirely and make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (output !== source) { | ||||||||||||||||||||||||||||||||||||
| await writeFile(filePath, output) | ||||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| async function main() { | ||||||||||||||||||||||||||||||||||||
| if (!existsSync(distDir)) return | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const files = await listJsFiles(distDir) | ||||||||||||||||||||||||||||||||||||
| let changed = 0 | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| for (const file of files) { | ||||||||||||||||||||||||||||||||||||
| if (await fixFile(file)) changed += 1 | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| console.log(`Fixed ESM import specifiers in ${changed} dist file(s)`) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| await main() | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${specifier}.jsnor${specifier}/index.jsexists on disk, the function returns the original extension-less specifier unchanged. The output file will silently contain an unresolvable import, which only fails at runtime in the consuming project rather than failing the build step where it would be easy to catch.Prompt To Fix With AI