Migrate kg-clean-basic-html to TypeScript#1760
Open
kevinansfield wants to merge 2 commits intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR migrates the kg-clean-basic-html package to a TypeScript-first build. It adds TypeScript configs (tsconfig.json, tsconfig.cjs.json), converts source and tests to TS/ESM (including typed clean-basic-html and a re-export in src/index.ts), updates test globals via test/utils/overrides.ts, replaces Rollup build config (deleted rollup.config.mjs) with a build/ cjs/esm output layout, updates package.json with types/exports and new devDependencies, replaces ESLint config with a typescript-eslint setup, and updates .gitignore to ignore build/ and tsconfig.tsbuildinfo. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
from
a2d70c8 to
3677be0
Compare
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
from
3677be0 to
92a708c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kg-clean-basic-html/cjs/clean-basic-html.js (1)
23-96:⚠️ Potential issue | 🔴 CriticalRemove this unpublished legacy file.
The
cjs/clean-basic-html.jsis not part of the npm package (filesarray only includesbuild), and the build system already generates CJS output from the TypeScript source (src/clean-basic-html.ts) viatsconfig.cjs.jsontobuild/cjs/. This hand-maintained version creates unnecessary drift risk and should be deleted to eliminate confusion about the actual source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/cjs/clean-basic-html.js` around lines 23 - 96, This legacy, hand-maintained CommonJS implementation (function cleanBasicHtml and module.exports = cleanBasicHtml) is untracked by package files and duplicates the generated build output; remove the entire cjs/clean-basic-html.js file to avoid drift and confusion so the project relies on the TypeScript source (src/clean-basic-html.ts) and the automated build output instead.
🧹 Nitpick comments (4)
packages/kg-clean-basic-html/src/clean-basic-html.ts (1)
1-6: Export the options interface for consumer type reuse.Making the options type public improves API usability for downstream TypeScript callers.
💡 Proposed refactor
-interface CleanBasicHtmlOptions { +export interface CleanBasicHtmlOptions { allowBr?: boolean; firstChildInnerContent?: boolean; removeCodeWrappers?: boolean; createDocument?: (html: string) => Document; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/src/clean-basic-html.ts` around lines 1 - 6, The CleanBasicHtmlOptions interface should be made public so downstream TypeScript consumers can reuse it; update the declaration of CleanBasicHtmlOptions to export it (i.e., add the export modifier) and ensure any local references (e.g., function signatures that accept CleanBasicHtmlOptions) still compile against the exported type (adjust imports/exports in the module if necessary).packages/kg-clean-basic-html/test/clean-basic-html.test.ts (2)
9-9: Consider importingCleanBasicHtmlOptionstype from the source module.The inline type definition works but could drift from the actual interface. Importing the type would ensure consistency:
import cleanBasicHtml, { CleanBasicHtmlOptions } from '../src/clean-basic-html.js';Then use
Partial<CleanBasicHtmlOptions>or a subset type for the test options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts` at line 9, Replace the inline type for the test options with the exported CleanBasicHtmlOptions to keep the test in sync with the implementation: import cleanBasicHtml and the CleanBasicHtmlOptions type from the module (e.g., import cleanBasicHtml, { CleanBasicHtmlOptions } from '../src/clean-basic-html.js') and change the declaration of options to use Partial<CleanBasicHtmlOptions> (or a specific subset) instead of the inline { createDocument: (html: string) => Document } type so the test uses the canonical interface; keep createDocument referenced as before.
60-65: Test name is misleading about what it actually tests.This test passes but for the wrong reason. When
htmlis empty string:
- After initial cleaning,
cleanHtmlremains''- The
if (cleanHtml)check is falsy, so the DOM parsing (wherefirstChildInnerContentis evaluated) is skipped entirely- The function returns
''without ever checkingfirstChildInnerContentThe test name suggests it's verifying behavior when there's no first child element, but it's actually testing that empty input returns empty output regardless of options.
Consider either:
- Renaming to clarify:
"returns empty string for empty input regardless of firstChildInnerContent option"- Or testing with HTML that has content but no first child element (if such a case exists)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts` around lines 60 - 65, The test name is misleading because it never exercises the firstChildInnerContent branch when html is empty; either rename the test to something like "returns empty string for empty input regardless of firstChildInnerContent option" or change the test input to a non-empty string that still has no element child so the DOM parsing runs (for example use a text-only string or an HTML comment) and then assert behavior of cleanBasicHtml(..., { firstChildInnerContent: true }) against that case; reference the cleanBasicHtml function and the firstChildInnerContent option to locate the relevant test and adjust the input or name accordingly.packages/kg-clean-basic-html/package.json (1)
19-23: Build scripts are duplicated and missing CJS package.json marker.Two concerns:
- Script duplication:
build,prepare, andpretestare identical. Consider havingprepareandpretestcallbuild:♻️ Suggested refactor
"scripts": { "dev": "tsc --watch --preserveWatchOutput", "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "prepare": "yarn build", + "pretest": "yarn build",
- Dual package safety: The package uses dual exports (CJS and ESM), but the build script only creates
build/esm/package.jsonwith"type":"module". Add a marker file tobuild/cjs/to ensure CommonJS files are correctly interpreted:♻️ Suggested fix for dual package
- "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"commonjs\"}' > build/cjs/package.json && echo '{\"type\":\"module\"}' > build/esm/package.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 19 - 23, The package.json currently duplicates the build steps across "build", "prepare", and "pretest" and only writes an ESM marker; change "prepare" and "pretest" to simply invoke the single "build" script (e.g., "npm run build") to remove duplication, and update the "build" script (the command referenced by "build") so it still runs both TypeScript builds (tsc and tsc -p tsconfig.cjs.json) but writes both package marker files: write build/esm/package.json with {"type":"module"} and write build/cjs/package.json with {"type":"commonjs"} so the output folders are explicitly marked for ESM and CJS consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-clean-basic-html/es/clean-basic-html.js`:
- Around line 1-80: The repo still contains stale built folders `es/` and `cjs/`
(example file: clean-basic-html.js under packages/kg-clean-basic-html/es/) which
conflict with the new TypeScript outputs in build/; remove those tracked
artifacts from Git or add them to .gitignore and untrack them: delete the `es/`
and `cjs/` directories (or run git rm -r --cached on them) and add entries for
`/es/` and `/cjs/` to .gitignore, then commit the removal so only the new build
outputs (build/esm, build/cjs) remain tracked.
---
Outside diff comments:
In `@packages/kg-clean-basic-html/cjs/clean-basic-html.js`:
- Around line 23-96: This legacy, hand-maintained CommonJS implementation
(function cleanBasicHtml and module.exports = cleanBasicHtml) is untracked by
package files and duplicates the generated build output; remove the entire
cjs/clean-basic-html.js file to avoid drift and confusion so the project relies
on the TypeScript source (src/clean-basic-html.ts) and the automated build
output instead.
---
Nitpick comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 19-23: The package.json currently duplicates the build steps
across "build", "prepare", and "pretest" and only writes an ESM marker; change
"prepare" and "pretest" to simply invoke the single "build" script (e.g., "npm
run build") to remove duplication, and update the "build" script (the command
referenced by "build") so it still runs both TypeScript builds (tsc and tsc -p
tsconfig.cjs.json) but writes both package marker files: write
build/esm/package.json with {"type":"module"} and write build/cjs/package.json
with {"type":"commonjs"} so the output folders are explicitly marked for ESM and
CJS consumers.
In `@packages/kg-clean-basic-html/src/clean-basic-html.ts`:
- Around line 1-6: The CleanBasicHtmlOptions interface should be made public so
downstream TypeScript consumers can reuse it; update the declaration of
CleanBasicHtmlOptions to export it (i.e., add the export modifier) and ensure
any local references (e.g., function signatures that accept
CleanBasicHtmlOptions) still compile against the exported type (adjust
imports/exports in the module if necessary).
In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts`:
- Line 9: Replace the inline type for the test options with the exported
CleanBasicHtmlOptions to keep the test in sync with the implementation: import
cleanBasicHtml and the CleanBasicHtmlOptions type from the module (e.g., import
cleanBasicHtml, { CleanBasicHtmlOptions } from '../src/clean-basic-html.js') and
change the declaration of options to use Partial<CleanBasicHtmlOptions> (or a
specific subset) instead of the inline { createDocument: (html: string) =>
Document } type so the test uses the canonical interface; keep createDocument
referenced as before.
- Around line 60-65: The test name is misleading because it never exercises the
firstChildInnerContent branch when html is empty; either rename the test to
something like "returns empty string for empty input regardless of
firstChildInnerContent option" or change the test input to a non-empty string
that still has no element child so the DOM parsing runs (for example use a
text-only string or an HTML comment) and then assert behavior of
cleanBasicHtml(..., { firstChildInnerContent: true }) against that case;
reference the cleanBasicHtml function and the firstChildInnerContent option to
locate the relevant test and adjust the input or name accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28384750-4f5b-4d4e-8886-2c312c4b8ec4
⛔ Files ignored due to path filters (2)
packages/kg-clean-basic-html/es/clean-basic-html.js.mapis excluded by!**/*.mapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-clean-basic-html/.gitignorepackages/kg-clean-basic-html/cjs/clean-basic-html.jspackages/kg-clean-basic-html/es/clean-basic-html.jspackages/kg-clean-basic-html/eslint.config.mjspackages/kg-clean-basic-html/package.jsonpackages/kg-clean-basic-html/rollup.config.mjspackages/kg-clean-basic-html/src/clean-basic-html.tspackages/kg-clean-basic-html/src/index.tspackages/kg-clean-basic-html/test/clean-basic-html.test.tspackages/kg-clean-basic-html/test/utils/assertions.tspackages/kg-clean-basic-html/test/utils/index.tspackages/kg-clean-basic-html/test/utils/overrides.jspackages/kg-clean-basic-html/test/utils/overrides.tspackages/kg-clean-basic-html/tsconfig.cjs.jsonpackages/kg-clean-basic-html/tsconfig.json
💤 Files with no reviewable changes (2)
- packages/kg-clean-basic-html/rollup.config.mjs
- packages/kg-clean-basic-html/test/utils/overrides.js
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
from
92a708c to
7ebd2d7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kg-clean-basic-html/src/clean-basic-html.ts (1)
43-43:⚠️ Potential issue | 🔴 CriticalFix regex pattern: trailing
entity missing semicolon.The regex on line 43 is
/^ | $/g, but it should be/^ | $/g. The leading pattern correctly matches with a semicolon, while the trailing pattern is missing the semicolon. This inconsistency means trailing entities won't be removed as intended. Change $to $.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/src/clean-basic-html.ts` at line 43, The regex used in the .replace call currently reads `/^ | $/g` and misses the semicolon on the trailing entity; update that regex to include the semicolon on the trailing pattern (e.g. change ` $` to ` $`, resulting in `/^ | $/g`) in the .replace call in clean-basic-html.ts so leading and trailing ` ` entities are both matched and removed.
🧹 Nitpick comments (3)
packages/kg-clean-basic-html/test/clean-basic-html.test.ts (2)
25-30: Type assertions are acceptable but consider alternatives.The
(result as string)pattern is repeated throughout the tests. While this works, consider either:
- Adding a
should.exist(result)assertion before the equality check- Using a helper function that asserts and narrows the type
This is a minor style preference and the current approach is valid for test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts` around lines 25 - 30, In the test around cleanBasicHtml where the result variable is asserted with (result as string).should.equal(''), replace the repeated type assertions by first asserting existence to narrow type (e.g., add a should.exist(result) before the equality) or create and use a small helper (e.g., assertString(value) that asserts existence and returns the value as string) and then call that helper before comparing; update the tests that use (result as string) to use the existence assertion or helper to avoid repeated inline type assertions while keeping the same equality checks.
9-9: Consider using theCleanBasicHtmlOptionsinterface.The inline type
{createDocument: (html: string) => Document}could be replaced withPick<CleanBasicHtmlOptions, 'createDocument'>or the fullCleanBasicHtmlOptionstype imported from the source module for better maintainability.♻️ Suggested change
+import cleanBasicHtml, { type CleanBasicHtmlOptions } from '../src/clean-basic-html.js'; +// Note: requires exporting the interface from the source file describe('cleanBasicHtml', function () { - let options: {createDocument: (html: string) => Document}; + let options: CleanBasicHtmlOptions;This would require exporting the interface from
clean-basic-html.ts:export interface CleanBasicHtmlOptions { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts` at line 9, Replace the inline type for the options variable with the exported CleanBasicHtmlOptions (or Pick<CleanBasicHtmlOptions, 'createDocument'>) from the clean-basic-html module: export the CleanBasicHtmlOptions interface from clean-basic-html.ts if not already exported, then change the tests' declaration (the variable named options with createDocument) to use that imported type instead of the inline `{createDocument: (html: string) => Document}` so the test types remain consistent with the implementation.packages/kg-clean-basic-html/package.json (1)
19-22: Build command duplication.The same build command is duplicated across
build,prepare, andpretestscripts. Consider extracting this to a single script and referencing it.♻️ Suggested refactor
"scripts": { "dev": "tsc --watch --preserveWatchOutput", "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "prepare": "yarn build", + "pretest": "yarn build",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 19 - 22, The package.json repeats the same build steps in the "build", "prepare", and "pretest" scripts; create a single reusable script (e.g., "build:all" or "prepare:build") that runs "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json" and then update "build", "prepare", and "pretest" to call that new script (using npm run <script>), leaving the original logic centralized in the new script name so maintenance is easier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/kg-clean-basic-html/src/clean-basic-html.ts`:
- Line 43: The regex used in the .replace call currently reads
`/^ | $/g` and misses the semicolon on the trailing entity; update that
regex to include the semicolon on the trailing pattern (e.g. change ` $` to
` $`, resulting in `/^ | $/g`) in the .replace call in
clean-basic-html.ts so leading and trailing ` ` entities are both matched
and removed.
---
Nitpick comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 19-22: The package.json repeats the same build steps in the
"build", "prepare", and "pretest" scripts; create a single reusable script
(e.g., "build:all" or "prepare:build") that runs "tsc && tsc -p
tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json" and
then update "build", "prepare", and "pretest" to call that new script (using npm
run <script>), leaving the original logic centralized in the new script name so
maintenance is easier.
In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts`:
- Around line 25-30: In the test around cleanBasicHtml where the result variable
is asserted with (result as string).should.equal(''), replace the repeated type
assertions by first asserting existence to narrow type (e.g., add a
should.exist(result) before the equality) or create and use a small helper
(e.g., assertString(value) that asserts existence and returns the value as
string) and then call that helper before comparing; update the tests that use
(result as string) to use the existence assertion or helper to avoid repeated
inline type assertions while keeping the same equality checks.
- Line 9: Replace the inline type for the options variable with the exported
CleanBasicHtmlOptions (or Pick<CleanBasicHtmlOptions, 'createDocument'>) from
the clean-basic-html module: export the CleanBasicHtmlOptions interface from
clean-basic-html.ts if not already exported, then change the tests' declaration
(the variable named options with createDocument) to use that imported type
instead of the inline `{createDocument: (html: string) => Document}` so the test
types remain consistent with the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dba6efc0-5373-496b-b32f-18bd8bcbf2b5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-clean-basic-html/.gitignorepackages/kg-clean-basic-html/eslint.config.mjspackages/kg-clean-basic-html/package.jsonpackages/kg-clean-basic-html/rollup.config.mjspackages/kg-clean-basic-html/src/clean-basic-html.tspackages/kg-clean-basic-html/src/index.tspackages/kg-clean-basic-html/test/clean-basic-html.test.tspackages/kg-clean-basic-html/test/utils/assertions.tspackages/kg-clean-basic-html/test/utils/index.tspackages/kg-clean-basic-html/test/utils/overrides.jspackages/kg-clean-basic-html/test/utils/overrides.tspackages/kg-clean-basic-html/tsconfig.cjs.jsonpackages/kg-clean-basic-html/tsconfig.json
💤 Files with no reviewable changes (2)
- packages/kg-clean-basic-html/rollup.config.mjs
- packages/kg-clean-basic-html/test/utils/overrides.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-clean-basic-html/test/utils/overrides.ts
- packages/kg-clean-basic-html/.gitignore
- packages/kg-clean-basic-html/src/index.ts
- packages/kg-clean-basic-html/test/utils/index.ts
- packages/kg-clean-basic-html/tsconfig.cjs.json
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
4 times, most recently
from
436e4fc to
f51bc83
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/kg-clean-basic-html/.gitignore (1)
2-3: Optional cleanup: remove legacycjs/andes/ignores if obsolete.If outputs are now only under
build/, keepingcjs/andes/may be misleading. Safe to clean up once confirmed unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/.gitignore` around lines 2 - 3, Remove the legacy ignore entries "cjs/" and "es/" from packages/kg-clean-basic-html/.gitignore if build outputs have consolidated under "build/"; verify the repository no longer produces cjs/ or es/ artifacts (check build scripts and CI config referencing these directories) and then delete the two lines referencing cjs/ and es/ so .gitignore accurately reflects current output paths.packages/kg-clean-basic-html/package.json (1)
20-23: Deduplicate repeated build pipeline commands in scripts.Lines 20-22 duplicate the same command chain three times, which is easy to desync later.
♻️ Proposed refactor
"scripts": { "dev": "tsc --watch --preserveWatchOutput", - "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build:dist": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build": "yarn build:dist", + "prepare": "yarn build:dist", + "pretest": "yarn build:dist", "test": "NODE_ENV=testing c8 --all --reporter text --reporter cobertura tsx node_modules/.bin/mocha './test/**/*.test.ts'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 20 - 23, The scripts "build", "prepare", and "pretest" currently duplicate the same command chain; extract that common command into a single script (e.g., "build:all") and have "build", "prepare", and "pretest" invoke it (e.g., "npm run build:all") so the pipeline is defined in one place; update the package.json scripts to add the new shared script name (build:all) and replace the bodies of "build", "prepare", and "pretest" to call that script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 10-15: The "node" conditional in package.json exports causes ESM
imports to resolve to the CJS build because Node matches "node" before "import";
remove the "node" property from the exports map and ensure the export object
uses "import" for the ESM path and "require" for the CJS path (with "import"
listed before "require") so that ESM consumers get "./build/esm/index.js" and
CJS consumers get "./build/cjs/index.js" (update the exports block that
currently contains "types", "node", "import", "require" accordingly).
In `@packages/kg-clean-basic-html/tsconfig.cjs.json`:
- Around line 4-5: Update the CJS TypeScript build by changing the
"moduleResolution" setting in tsconfig.cjs.json from "Node10" to "NodeNext" so
the compiler resolves .js-suffixed relative imports (e.g.,
./clean-basic-html.js, ./utils/index.js) and the package.json exports map
correctly; modify the "moduleResolution" property value and run a quick build to
verify outputs for files referenced in this package's build config.
---
Nitpick comments:
In `@packages/kg-clean-basic-html/.gitignore`:
- Around line 2-3: Remove the legacy ignore entries "cjs/" and "es/" from
packages/kg-clean-basic-html/.gitignore if build outputs have consolidated under
"build/"; verify the repository no longer produces cjs/ or es/ artifacts (check
build scripts and CI config referencing these directories) and then delete the
two lines referencing cjs/ and es/ so .gitignore accurately reflects current
output paths.
In `@packages/kg-clean-basic-html/package.json`:
- Around line 20-23: The scripts "build", "prepare", and "pretest" currently
duplicate the same command chain; extract that common command into a single
script (e.g., "build:all") and have "build", "prepare", and "pretest" invoke it
(e.g., "npm run build:all") so the pipeline is defined in one place; update the
package.json scripts to add the new shared script name (build:all) and replace
the bodies of "build", "prepare", and "pretest" to call that script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41a11c7c-93a2-4154-ae40-b185e83a8b3d
📒 Files selected for processing (13)
packages/kg-clean-basic-html/.gitignorepackages/kg-clean-basic-html/eslint.config.mjspackages/kg-clean-basic-html/package.jsonpackages/kg-clean-basic-html/rollup.config.mjspackages/kg-clean-basic-html/src/clean-basic-html.tspackages/kg-clean-basic-html/src/index.tspackages/kg-clean-basic-html/test/clean-basic-html.test.tspackages/kg-clean-basic-html/test/utils/assertions.tspackages/kg-clean-basic-html/test/utils/index.tspackages/kg-clean-basic-html/test/utils/overrides.jspackages/kg-clean-basic-html/test/utils/overrides.tspackages/kg-clean-basic-html/tsconfig.cjs.jsonpackages/kg-clean-basic-html/tsconfig.json
💤 Files with no reviewable changes (2)
- packages/kg-clean-basic-html/rollup.config.mjs
- packages/kg-clean-basic-html/test/utils/overrides.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-clean-basic-html/test/utils/overrides.ts
- packages/kg-clean-basic-html/src/clean-basic-html.ts
- packages/kg-clean-basic-html/tsconfig.json
- packages/kg-clean-basic-html/test/clean-basic-html.test.ts
Comment on lines
+10
to
+15
| "exports": { | ||
| ".": { | ||
| "types": "./build/esm/index.d.ts", | ||
| "node": "./build/cjs/index.js", | ||
| "import": "./build/esm/index.js", | ||
| "require": "./build/cjs/index.js" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js package.json "exports", does condition key order affect resolution such that "node" placed before "import" causes ESM imports in Node to match "node" first? Please cite the official Node.js package exports documentation and a minimal example.
💡 Result:
Yes. In Node.js conditional exports are matched in object key order, and the first matching condition wins. Node’s docs state: “Within the exports object, key order is significant… earlier entries have higher priority and take precedence over later entries.” They also define "node" as matching “any Node.js environment” and "import" as matching when loaded via import/import(). Therefore, for an ESM import running in Node, both conditions are true; if you put "node" before "import", Node will match "node" first. [1]
Minimal example
package.json
{
"name": "cond-order-demo",
"type": "module",
"exports": {
".": {
"node": "./node.js",
"import": "./esm.js",
"default": "./default.js"
}
}
}node.js
export default "NODE";esm.js
export default "ESM";Consumer (ESM):
import v from "cond-order-demo";
console.log(v);Result: prints "NODE" (because "node" is encountered before "import" and matches in Node). If you swap the keys so "import" comes before "node", the same ESM import prints "ESM". [1]
Source: Node.js “Modules: Packages” → “Conditional exports”. [1]
Remove "node" condition to prevent ESM imports resolving to the CommonJS build.
The "node" condition matches any Node.js environment (both ESM and CJS). When placed before "import", it causes Node's ESM imports to match "node" first and resolve to ./build/cjs/index.js instead of the ESM build. Per Node.js conditional exports behavior, conditions are matched in key order, with the first match winning.
Use "import" and "require" to distinguish between ESM and CJS imports instead:
🔧 Proposed fix
"exports": {
".": {
"types": "./build/esm/index.d.ts",
- "node": "./build/cjs/index.js",
"import": "./build/esm/index.js",
- "require": "./build/cjs/index.js"
+ "require": "./build/cjs/index.js",
+ "default": "./build/esm/index.js"
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-clean-basic-html/package.json` around lines 10 - 15, The "node"
conditional in package.json exports causes ESM imports to resolve to the CJS
build because Node matches "node" before "import"; remove the "node" property
from the exports map and ensure the export object uses "import" for the ESM path
and "require" for the CJS path (with "import" listed before "require") so that
ESM consumers get "./build/esm/index.js" and CJS consumers get
"./build/cjs/index.js" (update the exports block that currently contains
"types", "node", "import", "require" accordingly).
Comment on lines
+4
to
+5
| "module": "CommonJS", | ||
| "moduleResolution": "Node10", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig module settings =="
fd 'tsconfig.*\.json$' packages/kg-clean-basic-html --exec sh -c '
echo "--- $1 ---"
rg -n "\"module\"|\"moduleResolution\"|\"verbatimModuleSyntax\"" "$1" || true
' sh {}
echo
echo "== .js-suffixed relative imports in TS sources/tests =="
rg -nP --type=ts '(from|import)\s+["'\''](\./|\.\./)[^"'\'']+\.js["'\'']' packages/kg-clean-basic-html || true
echo
echo "== package exports map (if present) =="
rg -n "\"exports\"|\"type\"|\"main\"|\"module\"" packages/kg-clean-basic-html/package.json || trueRepository: TryGhost/Koenig
Length of output: 1101
Switch CJS build resolution from Node10 to NodeNext for modern package compatibility.
The codebase uses .js-suffixed relative imports (e.g., ./clean-basic-html.js, ./utils/index.js) and includes a package.json exports map. Node10 module resolution cannot properly handle these modern patterns and risks incorrect CJS build output. NodeNext aligns with Node.js semantics and matches the ESM config's bundler resolution mode.
Proposed change
"module": "CommonJS",
- "moduleResolution": "Node10",
+ "moduleResolution": "NodeNext",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "module": "CommonJS", | |
| "moduleResolution": "Node10", | |
| "module": "CommonJS", | |
| "moduleResolution": "NodeNext", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-clean-basic-html/tsconfig.cjs.json` around lines 4 - 5, Update
the CJS TypeScript build by changing the "moduleResolution" setting in
tsconfig.cjs.json from "Node10" to "NodeNext" so the compiler resolves
.js-suffixed relative imports (e.g., ./clean-basic-html.js, ./utils/index.js)
and the package.json exports map correctly; modify the "moduleResolution"
property value and run a quick build to verify outputs for files referenced in
this package's build config.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/kg-clean-basic-html/test/clean-basic-html.test.ts (1)
29-29: Consider using type-safe assertion patterns instead ofas stringcasts.The
result as stringcasts throughout the tests bypass TypeScript's null checking for thestring | nullreturn type. While the casts work because the tested inputs produce non-null results, a more type-safe pattern would be:should(result).equal(''); // or result!.should.equal('');The
shouldlibrary's.equal()acceptsnull, so the cast is unnecessary. This applies to lines 29, 36, 43, 50, 57, 64, 72, 79, and 86.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts` at line 29, Replace the unsafe TypeScript casts in the test assertions that use `(result as string)` with a type-safe pattern: either call should(result).equal('') or use the non-null assertion `result!.should.equal('')` so you don't bypass null checking; update all occurrences referencing the test variable `result` in clean-basic-html.test.ts (the assertions currently using `(result as string).should.equal(...)`) to one of these patterns.packages/kg-clean-basic-html/package.json (1)
19-22: Consider extracting duplicated build commands to a reusable script.The
dev,build,prepare, andpretestscripts contain duplicated build logic (tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json). While npm lifecycle hooks require separate entries, you could extract the core build to reduce duplication:"scripts": { "build:esm": "tsc", "build:cjs": "tsc -p tsconfig.cjs.json", "build": "yarn build:esm && yarn build:cjs && echo '{\"type\":\"module\"}' > build/esm/package.json", "prepare": "yarn build", "pretest": "yarn build" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 19 - 22, The package.json scripts duplicate the same build commands across "dev", "build", "prepare", and "pretest"; extract the core steps into reusable scripts (e.g., "build:esm" for tsc, "build:cjs" for tsc -p tsconfig.cjs.json) and consolidate the echo of '{"type":"module"}' into the main "build" script, then have "prepare", "pretest" (and "dev" if appropriate) invoke the new build scripts (using npm/yarn run) instead of repeating the full command; update the "dev" script to either run "build:esm" in watch mode or call the consolidated script as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 19-22: The package.json scripts duplicate the same build commands
across "dev", "build", "prepare", and "pretest"; extract the core steps into
reusable scripts (e.g., "build:esm" for tsc, "build:cjs" for tsc -p
tsconfig.cjs.json) and consolidate the echo of '{"type":"module"}' into the main
"build" script, then have "prepare", "pretest" (and "dev" if appropriate) invoke
the new build scripts (using npm/yarn run) instead of repeating the full
command; update the "dev" script to either run "build:esm" in watch mode or call
the consolidated script as needed.
In `@packages/kg-clean-basic-html/test/clean-basic-html.test.ts`:
- Line 29: Replace the unsafe TypeScript casts in the test assertions that use
`(result as string)` with a type-safe pattern: either call
should(result).equal('') or use the non-null assertion
`result!.should.equal('')` so you don't bypass null checking; update all
occurrences referencing the test variable `result` in clean-basic-html.test.ts
(the assertions currently using `(result as string).should.equal(...)`) to one
of these patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed7175c9-8c34-4db1-868c-30159bb10f99
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
packages/kg-clean-basic-html/.gitignorepackages/kg-clean-basic-html/eslint.config.mjspackages/kg-clean-basic-html/package.jsonpackages/kg-clean-basic-html/rollup.config.mjspackages/kg-clean-basic-html/src/clean-basic-html.tspackages/kg-clean-basic-html/src/index.tspackages/kg-clean-basic-html/test/clean-basic-html.test.tspackages/kg-clean-basic-html/test/utils/index.tspackages/kg-clean-basic-html/test/utils/overrides.tspackages/kg-clean-basic-html/tsconfig.cjs.jsonpackages/kg-clean-basic-html/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/kg-clean-basic-html/rollup.config.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-clean-basic-html/src/index.ts
- packages/kg-clean-basic-html/tsconfig.cjs.json
- packages/kg-clean-basic-html/tsconfig.json
- packages/kg-clean-basic-html/src/clean-basic-html.ts
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
from
f51bc83 to
3421fd7
Compare
- Remove Rollup build (rollup.config.mjs, @rollup/plugin-babel, @babel/*) - Move lib/ to src/, rename .js to .ts - Add tsconfig.json (strict, NodeNext, ESM) - Add "type": "module" to package.json - Convert source and tests to ESM with type annotations - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc (replaces cjs/ and es/ dirs)
kevinansfield
force-pushed
the
ts/kg-clean-basic-html
branch
from
3421fd7 to
0000660
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/kg-clean-basic-html/package.json (1)
10-16:⚠️ Potential issue | 🟠 Major
exportscondition order currently favors CJS in Node ESM imports.At Line 13,
"node"is evaluated before"import", so Node ESM consumers can resolve to./build/cjs/index.jsinstead of the ESM build.Proposed fix
"exports": { ".": { "types": "./build/esm/index.d.ts", - "node": "./build/cjs/index.js", "import": "./build/esm/index.js", - "require": "./build/cjs/index.js" + "require": "./build/cjs/index.js", + "default": "./build/esm/index.js" } },#!/bin/bash set -euo pipefail pkg="packages/kg-clean-basic-html/package.json" echo "Export condition order under exports['.']:" jq -r '.exports["."] | to_entries[] | .key' "$pkg" echo echo "Check that \"node\" condition is absent:" jq -e '.exports["."] | has("node") | not' "$pkg" >/dev/null \ && echo "OK: no node condition present" \ || { echo "FAIL: found node condition"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 10 - 16, The package.json "exports" block currently lists the "node" condition before "import", which can cause Node ESM consumers to resolve to the CJS build; update the exports["."] ordering to remove or move the "node" condition so "import" (./build/esm/index.js) is evaluated before any CJS fallback (ensure "import" appears and "node" is either removed or placed after "import"), and verify the keys under exports["."] are: "types", "import", "require" (or keep "node" only if purposely required but after "import") so Node ESM resolves the ESM build correctly.
🧹 Nitpick comments (2)
packages/kg-clean-basic-html/package.json (1)
20-22: Build command is repeated across three scripts; extract a shared script.At Line 20-Line 22, the same compile command is duplicated in
build,prepare, andpretest. A single shared script reduces drift risk.Refactor sketch
"scripts": { + "build:artifacts": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build": "yarn build:artifacts", + "prepare": "yarn build:artifacts", + "pretest": "yarn build:artifacts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/package.json` around lines 20 - 22, The three npm scripts (build, prepare, pretest) duplicate the same compile command; add a single shared script name (e.g., "compile" or "build:compile") that runs "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", then update the existing "build", "prepare", and "pretest" scripts to invoke that shared script (npm run compile) to eliminate duplication; edit the package.json scripts section accordingly referencing the new shared script name so build, prepare, and pretest call it instead of repeating the command.packages/kg-clean-basic-html/src/clean-basic-html.ts (1)
1-6: Consider exportingCleanBasicHtmlOptionsfor consumer typing ergonomics.Right now callers can use the function, but cannot directly import the options type name for annotations.
Small API polish
-interface CleanBasicHtmlOptions { +export interface CleanBasicHtmlOptions {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-clean-basic-html/src/clean-basic-html.ts` around lines 1 - 6, The interface CleanBasicHtmlOptions is currently not exported; export it so consumers can import the type for annotations by adding the export keyword to the interface declaration (export interface CleanBasicHtmlOptions { ... }); update any internal references (e.g., the parameter type on the cleanBasicHtml function or other functions that consume the type) to use the now-exported symbol and run type checks to ensure no breakages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 10-16: The package.json "exports" block currently lists the "node"
condition before "import", which can cause Node ESM consumers to resolve to the
CJS build; update the exports["."] ordering to remove or move the "node"
condition so "import" (./build/esm/index.js) is evaluated before any CJS
fallback (ensure "import" appears and "node" is either removed or placed after
"import"), and verify the keys under exports["."] are: "types", "import",
"require" (or keep "node" only if purposely required but after "import") so Node
ESM resolves the ESM build correctly.
---
Nitpick comments:
In `@packages/kg-clean-basic-html/package.json`:
- Around line 20-22: The three npm scripts (build, prepare, pretest) duplicate
the same compile command; add a single shared script name (e.g., "compile" or
"build:compile") that runs "tsc && tsc -p tsconfig.cjs.json && echo
'{\"type\":\"module\"}' > build/esm/package.json", then update the existing
"build", "prepare", and "pretest" scripts to invoke that shared script (npm run
compile) to eliminate duplication; edit the package.json scripts section
accordingly referencing the new shared script name so build, prepare, and
pretest call it instead of repeating the command.
In `@packages/kg-clean-basic-html/src/clean-basic-html.ts`:
- Around line 1-6: The interface CleanBasicHtmlOptions is currently not
exported; export it so consumers can import the type for annotations by adding
the export keyword to the interface declaration (export interface
CleanBasicHtmlOptions { ... }); update any internal references (e.g., the
parameter type on the cleanBasicHtml function or other functions that consume
the type) to use the now-exported symbol and run type checks to ensure no
breakages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f431baa-de8d-452f-9557-fa60e64e487d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-clean-basic-html/.gitignorepackages/kg-clean-basic-html/eslint.config.mjspackages/kg-clean-basic-html/package.jsonpackages/kg-clean-basic-html/rollup.config.mjspackages/kg-clean-basic-html/src/clean-basic-html.tspackages/kg-clean-basic-html/src/index.tspackages/kg-clean-basic-html/test/clean-basic-html.test.tspackages/kg-clean-basic-html/test/utils/assertions.tspackages/kg-clean-basic-html/test/utils/index.tspackages/kg-clean-basic-html/test/utils/overrides.jspackages/kg-clean-basic-html/test/utils/overrides.tspackages/kg-clean-basic-html/tsconfig.cjs.jsonpackages/kg-clean-basic-html/tsconfig.json
💤 Files with no reviewable changes (2)
- packages/kg-clean-basic-html/test/utils/overrides.js
- packages/kg-clean-basic-html/rollup.config.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-clean-basic-html/test/utils/overrides.ts
- packages/kg-clean-basic-html/test/utils/index.ts
- packages/kg-clean-basic-html/tsconfig.json
- packages/kg-clean-basic-html/.gitignore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
yarn testpasses in kg-clean-basic-htmlyarn lintpasses in kg-clean-basic-html