-
-
Notifications
You must be signed in to change notification settings - Fork 15
Convert codebase to ESM, mostly #183
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: master
Are you sure you want to change the base?
Conversation
Curious what kind of perf hit we'll take with this. |
eaa2eb9
to
6ccb355
Compare
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.
Pull Request Overview
This PR converts the codebase from CommonJS to ES modules, updates test-task script extensions to .cjs
, and adjusts internal imports/exports accordingly.
- Switched
package.json
to"type": "module"
and updated npm script references to.cjs
- Rewrote
lib/*
andscripts/make-slink.js
fromrequire/module.exports
toimport/export
- Updated test-workspace task files to use
.cjs
extensions and adjustedrequire
paths
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
test-workspace/tasks/config1.cjs | Point to util.cjs instead of CJS module |
test-workspace/tasks/append1.cjs | Updated require path to .cjs |
test-workspace/tasks/append2.cjs | Updated require path to .cjs |
test-workspace/package.json | Changed all task scripts to use .cjs file extensions |
scripts/make-slink.js | Converted to ESM, added fileURLToPath and __dirname shim |
package.json | Switched to ESM ("type":"module" ) and extended test timeout |
lib/spawn.js | Replaced CJS branch export with import and export default |
lib/spawn-win32.js | Switched to ESM import and export default spawn |
lib/spawn-posix.js | Switched to ESM import and export default spawn |
lib/run-tasks.js | Converted to ESM and fixed spelling of “fulfilled” |
lib/run-task.js | Converted to ESM imports/exports and updated ansi-styles import |
lib/read-package-json.js | Converted to ESM import and export default |
lib/npm-run-all-error.js | Replaced module.exports with export default class |
lib/match-tasks.js | Converted to ESM import and export default |
lib/index.js | Converted to ESM imports/exports and fixed “sequential” spelling |
lib/create-prefix-transform-stream.js | Converted to ESM import and export default |
lib/create-header.js | Converted to ESM and replaced module.exports |
eslint.config.js | Converted to ESM config and simplified neostandard import |
.knip.jsonc | Updated entry glob to match .cjs task files |
Comments suppressed due to low confidence (2)
lib/run-task.js:15
- [nitpick] Since you're statically importing
ansi-styles
, the variableansiStylesPromise
is misleading—it’s not a promise. Consider renaming it (e.g.,ansiStylesModule
) and updating its usage accordingly.
import ansiStylesPromise from 'ansi-styles'
lib/run-task.js:146
- [nitpick] Awaiting a non-promise value is unnecessary here. You can directly use the imported
ansiStyles
object withoutawait
for clearer intent.
const ansiStyles = await ansiStylesPromise
Looks to be the same, which makes sense because the tests were already bottlenecked on a much slower io operation. |
This converts the codebase to esm, mainly in preparation for fixing the typedocs. Some of the parallel tests were written around sync behavior of cjs, so I preserved the sync nature of some of the test tasks.