Skip to content

fix: load CLI configs with top-level await #4100

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.

Sign up for GitHub

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

Merged
UziTech merged 1 commit into from
Sep 29, 2026
+9 −1
Merged

fix: load CLI configs with top-level await #4100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export async function main(nodeProcess) {
// try require for json
markedConfig = require(configFile);

@styfle styfle Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove require(configFile) now since we can always run await import(pathToFileURL(configFile).href) with any LTS version of Node.js, and it works with ESM and CJS

@pengboyu-dev pengboyu-dev Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Agreed that dynamic import() can handle ordinary ESM and CJS configs. I checked the existing loader behavior and found two compatibility details we'd need to preserve:

  • The CLI documents and automatically loads ~/.marked.json. A plain import() fails for JSON unless we handle it explicitly or supply the appropriate import attribute.
  • CJS configs such as exports.default = { breaks: true } currently work. With import(), the additional namespace wrapper means the existing single .default unwrap no longer reaches the config; the CLI exits successfully but the option is not applied.

I verified the current patch across Node 20, 22, 24, and 26: it fixes configs with top-level await while preserving those existing cases. My preference would be to keep this PR as the targeted fix and handle the loader simplification separately with compatibility tests. Does that sound reasonable?

@UziTech UziTech Sep 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM @styfle thoughts?

} catch(err) {
if (err.code !== 'ERR_REQUIRE_ESM') {
if (err.code !== 'ERR_REQUIRE_ESM' && err.code !== 'ERR_REQUIRE_ASYNC_MODULE') {
throw err;
}
// must import esm
Expand Down
5 changes: 5 additions & 0 deletions test/unit/bin.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ describe('bin/marked', () => {
stdout: '<p>line1<br>line2</p>',
}));

it('config with top-level await', testInput({
args: ['--config', fixturePath('bin-config-await.mjs'), '-s', 'line1\nline2'],
stdout: '<p>line1<br>line2</p>',
}));

it('config not found', testInput({
args: ['--config', fixturePath('does-not-exist.js'), '-s', 'line1\nline2'],
stderr: `Cannot load config file '${fixturePath('does-not-exist.js')}'`,
Expand Down
3 changes: 3 additions & 0 deletions test/unit/fixtures/bin-config-await.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default await Promise.resolve({
breaks: true,
});
Loading