Migrate kg-mobiledoc-html-renderer to TypeScript#1795
Merged
kevinansfield merged 2 commits intomainfrom Mar 20, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughThe PR migrates the kg-mobiledoc-html-renderer package from JavaScript/CommonJS to TypeScript/ESM with dual compiled outputs (build/esm and build/cjs). Changes include: converting source to .ts with typed exports, adding type declaration shims, new tsconfig files (including a CJS build config and test config), updating package.json to point to build outputs and an exports map, switching ESLint config to TypeScript, replacing JS tests with TS tests, adding test bootstrap overrides, and updating .gitignore and test utilities. The package now publishes compiled artifacts from build/. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.ts (2)
184-194: Type assertions bridge simple-dom's internal types.The
as unknown as SimpleDomNodedouble assertion on line 194 is a workaround for type incompatibility between simple-dom's internal types and the customSimpleDomNodeinterface. This works but is fragile—if the actual simple-dom node shape differs from the interface, runtime errors could occur without compile-time warnings.Consider documenting this coupling or adding a runtime check in development builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.ts` around lines 184 - 194, The double cast "as unknown as SimpleDomNode" on rendered.result is a fragile compile-time-only bridge between simple-dom internals and our SimpleDomNode interface; replace it with a safer approach by adding a runtime shape/assertion check in development and documenting the coupling: implement a small dev-only validator that verifies rendered.result has expected properties (e.g., tagName, childNodes, removeChild) before calling modifier.modifyChildren(rendered.result), throw or log a descriptive error if the shape is invalid, and add a comment near the DomModifier / SimpleDomNode usage explaining the dependency on simple-dom internals; reference SimpleDomNode, rendered.result, DomModifier and modifier.modifyChildren when updating the code.
8-20: Consider using simple-dom's built-in type definitions instead of this custom interface.The
simple-dompackage (v1.4.0) ships with its own TypeScript declarations indist/types/index.d.ts. Rather than maintaining a customSimpleDomNodeinterface, you could import and use the types exported directly from simple-dom to reduce code duplication and stay in sync with the library's API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.ts` around lines 8 - 20, Replace the custom SimpleDomNode interface with the official types from the simple-dom package: remove the local interface declaration and import the appropriate node/element types exported by simple-dom (e.g., the Node/Element types) and use those in MobiledocHtmlRenderer method and property signatures that currently reference SimpleDomNode so the renderer uses the library-provided type declarations and stays in sync with simple-dom.packages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.ts (2)
55-56: Minor: Split chained statement for readability.The combined statement with semicolon reduces readability.
♻️ Suggested formatting
- p.appendChild(env.dom.createTextNode(options.testOption as string)); return p; + p.appendChild(env.dom.createTextNode(options.testOption as string)); + return p;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.ts` around lines 55 - 56, The chained statement that creates a paragraph element, appends a text node, and returns it should be split into separate statements for clarity: first call env.dom.createElement('p') and assign to p (as currently done), then call p.appendChild(env.dom.createTextNode(options.testOption as string)) on its own line, and finally return p; update the code around the p variable and the calls to env.dom.createElement, p.appendChild, and env.dom.createTextNode to reflect this clearer, multi-line form.
4-24: Consider importing shared types from the source module.The
Mobiledocinterface duplicates the shape likely already defined in the source. Ifpackages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.tsexports this type, importing it would reduce duplication and keep types in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.ts` around lines 4 - 24, Replace the duplicated Mobiledoc and CardRenderArgs interface declarations with imports from the renderer source: remove the local interfaces and add imports for Mobiledoc (and CardRenderArgs if exported) from the module that exports MobiledocHtmlRenderer (e.g., the MobiledocHtmlRenderer.ts source), then update any test references to use the imported types (Mobiledoc, CardRenderArgs) so types are shared and stay in sync with the implementation.packages/kg-mobiledoc-html-renderer/package.json (1)
19-21: Consider extracting duplicate build commands.The same three commands (
tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json) are repeated inbuild,prepare, andpretest. Consider extracting to reduce duplication.♻️ Proposed refactor
"scripts": { "dev": "echo \"Implement me!\"", - "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 && tsc -p tsconfig.test.json", + "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "prepare": "yarn build", + "pretest": "yarn build && tsc -p tsconfig.test.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-mobiledoc-html-renderer/package.json` around lines 19 - 21, The package.json repeats the same tsc + cjs + package.json creation command in the "build", "prepare", and "pretest" scripts; extract that duplicated command into a single reusable npm script (e.g., "build:all") and have "build", "prepare", and "pretest" invoke "npm run build:all" instead. Ensure "pretest" still runs the additional "tsc -p tsconfig.test.json" after the shared script (e.g., "npm run build:all && tsc -p tsconfig.test.json"). Update the scripts entries "build", "prepare", and "pretest" to reference the new "build:all" script name.
🤖 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-mobiledoc-html-renderer/package.json`:
- Around line 19-21: The package.json repeats the same tsc + cjs + package.json
creation command in the "build", "prepare", and "pretest" scripts; extract that
duplicated command into a single reusable npm script (e.g., "build:all") and
have "build", "prepare", and "pretest" invoke "npm run build:all" instead.
Ensure "pretest" still runs the additional "tsc -p tsconfig.test.json" after the
shared script (e.g., "npm run build:all && tsc -p tsconfig.test.json"). Update
the scripts entries "build", "prepare", and "pretest" to reference the new
"build:all" script name.
In `@packages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.ts`:
- Around line 184-194: The double cast "as unknown as SimpleDomNode" on
rendered.result is a fragile compile-time-only bridge between simple-dom
internals and our SimpleDomNode interface; replace it with a safer approach by
adding a runtime shape/assertion check in development and documenting the
coupling: implement a small dev-only validator that verifies rendered.result has
expected properties (e.g., tagName, childNodes, removeChild) before calling
modifier.modifyChildren(rendered.result), throw or log a descriptive error if
the shape is invalid, and add a comment near the DomModifier / SimpleDomNode
usage explaining the dependency on simple-dom internals; reference
SimpleDomNode, rendered.result, DomModifier and modifier.modifyChildren when
updating the code.
- Around line 8-20: Replace the custom SimpleDomNode interface with the official
types from the simple-dom package: remove the local interface declaration and
import the appropriate node/element types exported by simple-dom (e.g., the
Node/Element types) and use those in MobiledocHtmlRenderer method and property
signatures that currently reference SimpleDomNode so the renderer uses the
library-provided type declarations and stays in sync with simple-dom.
In `@packages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.ts`:
- Around line 55-56: The chained statement that creates a paragraph element,
appends a text node, and returns it should be split into separate statements for
clarity: first call env.dom.createElement('p') and assign to p (as currently
done), then call p.appendChild(env.dom.createTextNode(options.testOption as
string)) on its own line, and finally return p; update the code around the p
variable and the calls to env.dom.createElement, p.appendChild, and
env.dom.createTextNode to reflect this clearer, multi-line form.
- Around line 4-24: Replace the duplicated Mobiledoc and CardRenderArgs
interface declarations with imports from the renderer source: remove the local
interfaces and add imports for Mobiledoc (and CardRenderArgs if exported) from
the module that exports MobiledocHtmlRenderer (e.g., the
MobiledocHtmlRenderer.ts source), then update any test references to use the
imported types (Mobiledoc, CardRenderArgs) so types are shared and stay in sync
with the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa17cecc-68b0-4412-9ee0-66e32c10189f
📒 Files selected for processing (16)
packages/kg-mobiledoc-html-renderer/.gitignorepackages/kg-mobiledoc-html-renderer/eslint.config.mjspackages/kg-mobiledoc-html-renderer/index.jspackages/kg-mobiledoc-html-renderer/package.jsonpackages/kg-mobiledoc-html-renderer/src/MobiledocHtmlRenderer.tspackages/kg-mobiledoc-html-renderer/src/index.tspackages/kg-mobiledoc-html-renderer/src/types.d.tspackages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.jspackages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.tspackages/kg-mobiledoc-html-renderer/test/utils/assertions.tspackages/kg-mobiledoc-html-renderer/test/utils/index.tspackages/kg-mobiledoc-html-renderer/test/utils/overrides.jspackages/kg-mobiledoc-html-renderer/test/utils/overrides.tspackages/kg-mobiledoc-html-renderer/tsconfig.cjs.jsonpackages/kg-mobiledoc-html-renderer/tsconfig.jsonpackages/kg-mobiledoc-html-renderer/tsconfig.test.json
💤 Files with no reviewable changes (3)
- packages/kg-mobiledoc-html-renderer/index.js
- packages/kg-mobiledoc-html-renderer/test/utils/overrides.js
- packages/kg-mobiledoc-html-renderer/test/MobiledocHtmlRenderer.test.js
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1795 +/- ##
==========================================
+ Coverage 85.22% 85.35% +0.13%
==========================================
Files 198 198
Lines 12979 13031 +52
Branches 1964 1970 +6
==========================================
+ Hits 11061 11123 +62
+ Misses 1905 1895 -10
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Move lib/ to src/, rename .js to .ts - Add tsconfig.json (strict, NodeNext, ESM) - Add "type": "module" to package.json - Convert source and tests from CJS to ESM - Add type annotations and type declaration files - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc
kevinansfield
force-pushed
the
ts/kg-mobiledoc-html-renderer
branch
from
ca0a288 to
3b0392a
Compare
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
tsconfig.test.jsonfor test type-checkinganytypes andeslint-disablecomments from test file"node"condition),moduleResolution→NodeNext,tseslint.config()→defineConfig()overrides.tstyping withas unknown aspattern