Skip to content

feat!: esm - #485

Open
gameroman wants to merge 1 commit into
ekalinin:masterfrom
gameroman:esm
Open

feat!: esm#485
gameroman wants to merge 1 commit into
ekalinin:masterfrom
gameroman:esm

Conversation

@gameroman

Copy link
Copy Markdown

Closes #484

@gameroman
gameroman marked this pull request as ready for review June 23, 2026 19:33
@gameroman

Copy link
Copy Markdown
Author

hi @derduher you asked me to tag you

@derduher

Copy link
Copy Markdown
Collaborator

Review

I built this branch locally to verify the riskier parts of the ESM migration rather than reviewing the diff text alone. That surfaced two build-breaking issues plus a few doc/process gaps.

🔴 Confirmed build-breaking issues

1. npm run build fails outright (tsc exits 2).
Two compile errors as-is:

  • tests/sitemap-parser.test.ts:15import normalizedSample from './mocks/sampleconfig.normalized.json'; is missing the with { type: 'json' } import attribute required under module: "NodeNext". tests/cli.test.ts correctly adds this attribute for the same kind of import; sitemap-parser.test.ts was missed.
  • tests/xmllint.test.ts:36,46assert.ok(!result) negates a void-typed value (TS1345), since xmlLint() returns Promise<void>. Looks like a mechanical Jest→node:test conversion slip.

test:full and prepublishOnly both hard-depend on a clean tsc build, so as submitted neither can succeed. gh pr checks shows no CI ran on this PR at all, which is presumably why this wasn't caught.

2. The built CLI is broken — --version/--help/every CLI test will fail.
In cli.ts:

const currentDir = new URL('.', import.meta.url).pathname
const packageJson = JSON.parse(
  readFileSync(resolve(currentDir, '../../package.json'), 'utf8')
);

The comment was updated ("one level up from dist") but the path literal ('../../package.json', two levels up) wasn't. cli.js now compiles directly to dist/cli.js (per the new tsconfig.json outDir: "dist"), so this needs to be '../package.json'.

Verified concretely — after building, node dist/cli.js --version throws:

Error: ENOENT: no such file or directory, open '/private/tmp/package.json'

i.e. it resolves one directory above the repo itself. Every case in tests/cli.test.ts invokes node ./dist/cli.js ..., so the whole CLI suite would fail once the build error above is fixed and tests actually run.

🟡 Other issues

  • Coverage enforcement silently dropped. The old jest.config.cjs enforced coverageThreshold (80% branches / 90% functions/lines/statements — also documented in CLAUDE.md). The new test:coverage script collects coverage but enforces no threshold, and test:full/CI don't call it at all — no automated coverage gate currently. CLAUDE.md's "Coverage Requirements (enforced by jest.config.cjs)" section is now stale (points at a deleted file).
  • tsconfig.json now includes tests and lib, not just index.ts/cli.ts. This causes tsc to emit compiled test files into dist/tests/*.js. .npmignore keeps these out of the published tarball, so it's not a publish-time issue, but it does mean test-file errors now block the library build, which is probably not intended.
  • Docs not updated — filing a follow-up issue for this since it should land after this PR merges (README/api.md CommonJS examples, CHANGELOG entry, stale CLAUDE.md coverage reference).

Recommendation

Don't merge as-is — the branch doesn't build and the CLI is non-functional post-build. Fixes are small and mechanical:

  1. Add with { type: 'json' } to the import in tests/sitemap-parser.test.ts.
  2. Fix (or rework) the assert.ok(!result) check in tests/xmllint.test.ts.
  3. Change '../../package.json' to '../package.json' in cli.ts.
  4. Decide whether test files should stay in the main tsconfig.json's include, and whether coverage thresholds should be restored somewhere in the pipeline.

Once fixed, would want to see green CI on this PR before merging.

@gameroman

Copy link
Copy Markdown
Author

@derduher I think I've addressed the review

@derduher derduher left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall this is a solid ESM-only modernization — the verbatimModuleSyntax conversion, the Jest→node:test rewrite, Windows support, and pinned action SHAs all look careful and correct. Node >=22 is fine.

There's one blocker, though: the build emits no JavaScript, so both the test suite and the published package are broken. tsconfig.build.json sets emitDeclarationOnly: true, so npm run build produces only .d.ts files:

$ npm run build && find dist -name '*.js' | wc -l   → 0
                   find dist -name '*.d.ts' | wc -l  → 15

Because of this:

  • The tests fail. They now import from ../dist/lib/*.js / ../dist/index.js, none of which exist after a build:
    Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../dist/lib/errors.js'
    ✖ tests/sitemap-stream.test.ts … pass 0 fail 1
    
    CI runs build then test, so it should be red for the same reason.
  • The published package is brokenexports.default./dist/index.js and bin./dist/cli.js won't exist.

Fix is to drop emitDeclarationOnly: true from tsconfig.build.json so tsc emits .js alongside the declarations. Typecheck itself is clean (tsc --noEmit exits 0), so this is purely the emit config. See inline comments.

Minor: worth calling out the CJS removal as a breaking change in the PR description for consumers migrating off require('sitemap').

Comment thread tsconfig.build.json
"extends": "./tsconfig.json",
"compilerOptions": {
"noEmit": false,
"emitDeclarationOnly": true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocker: emitDeclarationOnly: true means tsc emits only .d.ts — no runtime .js. After npm run build, dist/ contains 15 .d.ts files and zero .js, so dist/index.js, dist/cli.js, and dist/lib/*.js never get created.

This breaks the published package (exports.default/bin point at nonexistent .js) and the test suite (tests import ../dist/lib/*.js, which fail with ERR_MODULE_NOT_FOUND).

Suggest removing this line so tsc emits JS alongside declarations:

Suggested change
"emitDeclarationOnly": true,
"noEmit": false,
"declaration": true,
"outDir": "dist"

Comment thread CLAUDE.md
npm test # Run Jest tests with coverage
npm run test:full # Run lint, build, Jest, and xmllint validation
npm run test:full # Run lint, build, coverage-enforced tests, and xmllint validation
npm run test:typecheck # Type check only (tsc)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Stale after this PR: it's no longer Jest, and npm test no longer collects coverage (that moved to test:coverage / test:full). Suggest updating to something like # Run tests with the Node.js test runner.

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.

discussion: esm-only

2 participants