Migrate CLI output from CJS to ESM#2836
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2836 +/- ##
==========================================
- Coverage 72.07% 71.92% -0.16%
==========================================
Files 134 132 -2
Lines 7410 7358 -52
Branches 1550 1611 +61
==========================================
- Hits 5341 5292 -49
- Misses 1941 2021 +80
+ Partials 128 45 -83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the markbind-cli package from CommonJS to ES Modules (ESM), updating imports/paths accordingly and adjusting tooling (tests, linting) to work with ESM output while maintaining compatibility with existing CommonJS-only dependencies.
Changes:
- Convert
packages/clito ESM ("type": "module") and update internal imports to ESM-compatible.jsspecifiers. - Add
exportsmappings in@markbind/coreto allow ESM consumers (the CLI) to import specific deep paths. - Switch CLI unit tests from
ts-jestto Babel-based transpilation and adjust functional test fixtures for CJS plugins.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/package.json | Adds exports entries for deep imports used by the ESM CLI; pins @types/lodash version to avoid hoisting/type issues. |
| packages/cli/tsconfig.lint.json | Introduces a CLI-specific lint tsconfig (currently mirrors base). |
| packages/cli/tsconfig.json | Updates TS module settings to nodenext for ESM output and adjusts excludes. |
| packages/cli/test/unit/ipUtil.test.ts | Updates relative import to include .js extension for ESM TS emit. |
| packages/cli/test/unit/cliUtil.test.ts | Refactors FS mocking to use memfs vol directly and updates ESM import path. |
| packages/cli/test/functional/updatetest.ts | Updates imports for ESM-style paths and lodash usage. |
| packages/cli/test/functional/test_site_special_tags/site.json | Ignores added plugins package.json used to force CJS interpretation. |
| packages/cli/test/functional/test_site_special_tags/_markbind/plugins/package.json | Forces plugin scope to CommonJS for functional tests. |
| packages/cli/test/functional/test_site/site.json | Same as above for the main functional test site. |
| packages/cli/test/functional/test_site/_markbind/plugins/package.json | Forces plugin scope to CommonJS for functional tests. |
| packages/cli/test/functional/testUtil/diffPrinter.ts | Converts chalk import from require to ESM import. |
| packages/cli/test/functional/testUtil/diffChars.ts | Updates relative import to include .js extension. |
| packages/cli/test/functional/testUtil/compare.ts | Consolidates lodash imports to a single default import and updates relative import extension. |
| packages/cli/test/functional/testUtil/cleanup.ts | Converts require imports to ESM import. |
| packages/cli/test/functional/test.ts | Updates lodash import style, ESM pathing, and replaces __dirname usage with import.meta.url derivation. |
| packages/cli/test/functional/.eslintrc.js | Removes functional-test-local ESLint config (replaced by package-level config). |
| packages/cli/src/util/serveUtil.ts | Updates imports to ESM .js paths and new live-server export shape. |
| packages/cli/src/util/logger.ts | Switches to a CJS wrapper for winston v2 compatibility under ESM. |
| packages/cli/src/util/logger-wrapper.cjs | Adds CommonJS “barrel” wrapper to load winston v2 + rotate transport from ESM code. |
| packages/cli/src/util/cliUtil.ts | Switches lodash per-method import to default lodash import. |
| packages/cli/src/lib/live-server/index.js | Migrates vendored live-server patch file to ESM syntax and adjusts live-server path resolution/export. |
| packages/cli/src/lib/live-server/index.d.ts | Updates typings to match the new LiveServer instance export shape. |
| packages/cli/src/cmd/serve.ts | Converts imports to ESM .js paths; moves webpack dev config loading to ESM import. |
| packages/cli/src/cmd/init.ts | Converts logger import path to ESM .js and consolidates lodash usage. |
| packages/cli/src/cmd/deploy.ts | Converts internal imports to ESM .js paths and consolidates lodash usage. |
| packages/cli/src/cmd/build.ts | Converts internal imports to ESM .js paths and consolidates lodash usage. |
| packages/cli/package.json | Marks CLI as ESM, updates dev deps (lodash types, ignore), replaces ts-jest with Babel + babel-jest. |
| packages/cli/jest.config.js | Removes old CJS Jest config. |
| packages/cli/jest.config.cjs | Adds new Jest config for ESM-friendly import mapping. |
| packages/cli/index.ts | Updates ESM import paths and switches to JSON import to read CLI version. |
| packages/cli/babel.config.cjs | Adds Babel config for TS transpilation in Jest. |
| packages/cli/AGENTS.md | Updates package documentation to reflect TS + ESM and new testing setup. |
| packages/cli/.eslintrc.cjs | Adds CLI-level ESLint overrides for ESM import extensions and lodash rule exceptions. |
| AGENTS.md | Updates repo-level documentation to reflect CLI now being TypeScript/ESM. |
| .eslintignore | Ignores generated/fixture JS in CLI functional test directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Harjun751
force-pushed
the
cli-cjs-to-esm
branch
from
534d2ed to
f87c07e
Compare
|
impressive work! Let me know when it's ready for review |
Project uses airbnb rules throughout the codebase.Airbnb uses import/extensions rule to ensure that imports don't have extensions. This conflicts with ESM requirement of including extensions when using relative imports. Override rule such that errors are not shown when including import extensions.
Harjun751
force-pushed
the
cli-cjs-to-esm
branch
from
924b926 to
da1e96f
Compare
To utilize ESM, TypeScript has to use stricter module resolution algorithms. This requires updating some imports to use file extensions explicitly.
Towards ESM efforts, updated module resolution algorithm does not resolve nested package imports like: `'@markbind/core/src/utils/logger` by default. Such modules must be explicitly exported in the package.json of the corresponding package. Let's add all modules that are used in core by other packages explicitly into the package.json.
types/lodash package is currently set to a version older than the lodash package. To have updated and accurate type definitions, bump types/lodash to match the lodash package version.
Markbind CLI uses optimized lodash imports. With the ESM migration, the updated module resolution algorithm does not resolve lodash optimized imports with the types/lodash definitions. Change optimized imports to use named imports over the whole 'lodash' package. Refer to discussion at MarkBind#2615
markbind/cli uses an old version of ignore. The version used is not compatible with the nodenext module resolution algorithm. Let's bump ignore to the latest version Additional context on issue: kaelzhang/node-ignore#96
Winston import statements break after changing output of package to ESM - notably, transports object becomes undefined despite types being available for them. Add a commonJS wrapper around winston to allow for CJS require() syntax usage to import desired functionality. The wrapper then exports it, like a barrel file. This is not an ideal solution, but it allows for the least amount of changes for the puposes of the CJS -> ESM migration. We should migrate winston to v3 in the near future. It requires a good amount of changes and care.
markbind/cli uses ts-jest to run tests with. Both jest and ts-jest have shaky ESM support. ESM with jest is still experimental - it requires running node with an additional flag. Furthermore, performing manual mocking with ESM seems to be broken, with no official guidance on how to achieve this effectively. Further complicating matters is the fact that mocking default exports with ESM export syntax is not trivial. Configure jest to use babel to transpile ESM modules to CJS syntax. While this means that we are no longer testing against ESM output, I believe it is the way forward for now. (Context: I've been trying this for 3 days and I might be going insane) We should investigate other test runners such as vitest and bun which have native support for ESM in the future, if possible.
Some test sites include Javascript code for
testing plugin integration.
Due to update of CLI package.json, all javascript
files within the CLI folder are interpreted as
ecmascript modules. This causes an error when
attempting to run the test sites, as the plugins
in the test site use non-ESM compatible syntax
like `require('..')`
Add a package.json in the scope of the affected
test sites to signal that the files within
should be parsed as commonjs modules.
This is simpler than updating markbind core
code to allow for .cjs file extensions.
This issue should not occur when users use
markbind, unless they have a package.json present
within scope that sets files to use ESM
syntax. In such cases, users should already
be using the proper syntax and hence this
shouldn't be a problem. Either way,
markbind reports the error correctly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "extends": "../../tsconfig_base.json", | ||
| "exclude": ["node_modules", "**/*.test.ts", "dist", "**/*.test.js", "test/**/*.js", "**/__mocks__", "coverage"], | ||
| "exclude": ["node_modules", "dist", "**/*.test.js", "test/**/*.js", "**/__mocks__", "coverage"], |
There was a problem hiding this comment.
This tsconfig no longer excludes **/*.test.ts, so tsc will emit unit test files into dist/. Since the package publishes dist/, this will likely ship tests to consumers and inflate install size. Consider re-adding **/*.test.ts to exclude (and, if functional tests need to be emitted, include them explicitly via a separate tsconfig or a narrow include).
There was a problem hiding this comment.
For now this is required since the functional tests are built and then ran. Maybe we can consider moving the test files to be ran with tsx, but I think for now this works?
There was a problem hiding this comment.
Agree that functional tests need to be built
Several actionables for this particular issue:
-
Add
**/*.test.tsto the exclude array to prevent unit tests from being compiled intodist/. (The two unit tests are run directly through jest), as a short term fix. -
To prevent tests from being shipped, add to
.npmignore:dist/test. So it does not include it in the npm release.
Additionally, can consider opening an issue as per your suggestion on running with tsx to make sure it is documented and potentially worke on in the future
| extends: ['../../.eslintrc.js'], | ||
| overrides: [ | ||
| { | ||
| files: ['./**/*.ts'], |
There was a problem hiding this comment.
This config adds CLI-specific TS settings (NodeNext), but ESLint’s type-aware setup (from the root .eslintrc.js) won’t automatically use it unless parserOptions.project is pointed at this file. Without that, linting may still run against the root tsconfig.lint.json (CommonJS) and fail on import.meta / ESM-only syntax used in the functional tests. Consider adding parserOptions.project: ['./tsconfig.lint.json'] (or similar) to the TS override in packages/cli/.eslintrc.cjs, or wiring this file into the root ESLint config.
| files: ['./**/*.ts'], | |
| files: ['./**/*.ts'], | |
| parserOptions: { | |
| project: ['./tsconfig.lint.json'], | |
| }, |
There was a problem hiding this comment.
We need to update eslint to a higher version to allow use to use the latest ecmascript features. Currently, it's right that it fails on import.meta, but we should tackle this in another PR intended to upgrade the eslint dependency I think.
There was a problem hiding this comment.
Great PR, indeed it is a lot of work to migrate to ESM (though necessary)
Some of the things we should work on moving forward after we merge this (thank you for your work)
- Testing CJS -> ESM
- As per PR, now using Babel to transpile ESM back to CJS just for Jest. Switch to Vitest or Native ESM Jest so that it handles ESM natively without complex configuration. It uses the same API as Jest, so the migration is usually 90% find-and-replace. Faster tests, no need transpile.
- Full Monorepo Consistency
- With this PR, cli is ESM, but core is CommonJS. Converting core and all other packages to ESM. will allow us to remove used barrel file, workarounds and conduct much needed dependency updates
- Upgrading Legacy Dependencies
- Winston: Migrate to v3. It's a significant change to the logging API, but it's the standard for modern Node.js.
Lodash: Replace Lodash functions with native ES6+ methods (like Array.map, Object.keys, etc.) which are now standard.
-
Modernizing the Linting Infrastructure
Upgrade to ESLint 9+ and migrate to the Flat Config (eslint.config.js), etc. Define rules for specific folders without the "extension/inheritance" mess you're currently dealing with. It also has better native support for ESM and import.meta. -
Apply more Dependency Injection vs. Manual Mocks (Extension / Stretch Goal)
The issues with the tests show that the code right now is actually tightly coupled.
We should refactor and move toward Dependency Injection (DI) or modular designs where mockable services are passed in as arguments rather than being globally imported.
| isError, | ||
| }; | ||
| import _ from 'lodash'; | ||
| import * as cliUtil from '../util/cliUtil.js'; |
There was a problem hiding this comment.
Just a Note: Unlike the older CommonJS, ESM in Node.js strictly requires explicit file extensions, hence these updates are required here.
|
|
||
| // CHANGED: added absolute path that directs to the live-server directory | ||
| const pathToLiveServerDir = path.dirname(require.resolve('live-server')); | ||
| // Use createRequire for Node 18 compatibility (import.meta.resolve is synchronous only in Node 20.6.0+) |
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
Understand the situation with the tests in cli/tests/functional/
The fix to
add a package.json at the scope of the test sites to let those plugin files be interpreted as CJS instead of ESM.
As you mentioned,
Either way, if there is, the user should already be using ESM syntax then, so they shouldn't encounter a breakage.
Hence, the package.json here is intended to be a short term fix right? We should also be hopefully migrating these test custom plugins in to ESM formats, since we would expect custom plugins to be written in ESM moving forward.
Maybe we should open an issue to track this chore as well? I can offer to tackle this hopefully small task if needed.
Or we can directly migrate them in this PR as well.
| { | ||
| "extends": "../../tsconfig_base.json", | ||
| "exclude": ["node_modules", "**/*.test.ts", "dist", "**/*.test.js", "test/**/*.js", "**/__mocks__", "coverage"], | ||
| "exclude": ["node_modules", "dist", "**/*.test.js", "test/**/*.js", "**/__mocks__", "coverage"], |
There was a problem hiding this comment.
Actionables (add as separate comment so easier for you to see):
-
Add
**/*.test.tsto the exclude array to prevent unit tests from being compiled into dist/. (The two unit tests are run directly through jest), as a short term fix. -
To prevent tests from being shipped, add to
.npmignorethe line:dist/test. So it does not include it in the npm release.
Additionally, can consider opening an issue as per your suggestion on running with tsx to make sure it is documented and potentially worke on in the future
2 participants
What is the purpose of this pull request?
Overview of changes:
At a high level,
Anything you'd like to highlight/discuss:
(I have additional details written down in the commit bodies)
Happy CNY folks! Working on this PR was a lot of trouble due to the very many subtle ways that the packages broke when using ESM. There were about 4 major breakages:
Tests in
cli/tests/functional/(POSSIBLE BREAKING CHANGE....?!?)A significant thing that broke was the tests in the functional/ folder. Due to the added ESM configuration, it parsed the plugins (which are javascript files) within the test sites as ESM, so it broke when building the site with those files since they used CJS syntax (namely,
require).The fix was this was to add a package.json at the scope of the test sites to let those plugin files be interpreted as CJS instead of ESM.
I don't think this will be an issue for users since I don't think there's a use case where there will be a package.json in the scope of the custom plugins. Either way, if there is, the user should already be using ESM syntax then, so they shouldn't encounter a breakage. But, if in the case that
type: "module"(ESM syntax)Then, the site will not be able to be built.
Lodash
We discussed this in telegram - pasting my findings here for any poor soul in the future who somehow ends up here and needs this information for some reason:
Details
Yea okay I somewhat figured it out, but not really. Really insidious issue.I was working on cjs->esm migration, and due to that some of the imports and types even had to be updated, and one of the ones I needed to update was @types/lodash. I needed to bump that package up, it was currently set to ^4.14.181. So that I did, I bumped it up to ^4.17.0 and then when I ran npm i it failed completely.
It was raising an issue that the core package failed to build due to TS2790: The operand of a 'delete' operator must be optional. This was really weird to me because I did not touch the core package at all, and I don't see why this issue was being raised up only NOW.
For reference, it was erroring out on the delete call there at the bottom.
This code didn't even seem to be related to lodash, so why was it even erroring?
My first suspicion was that somehow running npm i bumped the typescript package version, causing stricter checks to be enacted. I looked at the package-lock.json diff and this was not the case - the ONLY changes were related to lodash.
Next line of thinking was did the htmlparser2 package change in some way? The lineNode object was of type DOMElement, and I was wondering if somehow in someway in hell updating lodash types changed the object structure of DOMElement.
Probably not the case, so I looked on. Checking the core package-json, I saw that types/lodash was actually defined in there too.
In monorepos/npm workspaces, when npm sees that more than 1 subpackage has a similar dependency, it "hoists" dependencies. npm will select a compatible version for the 2 subpackages using semver to evaluate. The dependencies will all be in the root node_modules folder and symlinked to the specific subpackage that requires them.
So what was happening was that by updating the cli's types/lodash, npm picked another "theoretically compatible" version of types/lodash to use in core. This updated version, for some reason, was NOT, in fact, compatible?!11?? EVEN though that 1) its just a types/ module and 2) it was a minor version bump, that broke core.
The funny thing is that I still don't even know how it broke. All i know is that THIS check occured before the delete call:
You can see that lodash was used here. But that doesn't explain how types/lodash somehow modifies the inferred type of DOMElement, from having an optional "attribs" element to it being strictly required, hence causing typescript to yell out.
Compouding on my confusion on this issue, the has function in lodash wasn't even touched according to the changelog of lodash itself. I also can't find changelogs for types/lodash because they don't seem to track that in the DefinitelyTyped repo.
The ""fix"" was to replace "^4.14.blahblahblah" in the core/ package to remove the "^" symbol so that that version was forced to be used for only the core package.
Winston
Winston v2 doesn't work with ESM. v3 fixes this, but migration is not trivial so we should put that off for a later time.
To fix this, I created a barrel file to import it using CJS syntax.
jest/ts-jest
This was the worst one to debug. It broke for specifically manual mocks - I tried a lot of things fixing it and it's ugly. Wish I had the resources to compile here so I could show yall, but I didn't save them and I don't want to go through the trauma of searching for it. Most of my time was on this and I was really going crazy lol. In the end, changed to using babel to transpile ESM -> CJS.
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Migrate CLI from CommonJS to ES Modules
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):