Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Convert codebase to ESM, mostly #183

wants to merge 2 commits into from

Conversation

bcomnes
Copy link
Owner

@bcomnes bcomnes commented May 26, 2025

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.

@bcomnes
Copy link
Owner Author

bcomnes commented May 26, 2025

Curious what kind of perf hit we'll take with this.

@bcomnes bcomnes force-pushed the esm branch 3 times, most recently from eaa2eb9 to 6ccb355 Compare May 27, 2025 01:19
Copy link

@Copilot Copilot AI left a 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/* and scripts/make-slink.js from require/module.exports to import/export
  • Updated test-workspace task files to use .cjs extensions and adjusted require 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 variable ansiStylesPromise 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 without await for clearer intent.
const ansiStyles = await ansiStylesPromise

@bcomnes bcomnes marked this pull request as ready for review May 27, 2025 02:12
@bcomnes
Copy link
Owner Author

bcomnes commented May 27, 2025

Curious what kind of perf hit we'll take with this.

Looks to be the same, which makes sense because the tests were already bottlenecked on a much slower io operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant