Migrate kg-lexical-html-renderer to TypeScript#1791
Open
kevinansfield wants to merge 2 commits intomainfrom
Open
Migrate kg-lexical-html-renderer to TypeScript#1791kevinansfield wants to merge 2 commits intomainfrom
kevinansfield wants to merge 2 commits intomainfrom
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 packages/kg-lexical-html-renderer to an ESM-first layout with dual ESM/CJS published outputs. It adds new src entrypoints (including src/index.ts and src/transformers/index.ts), removes legacy lib transformer and index exports, converts runtime modules and tests from CommonJS to ES modules and updates import paths to explicit .js extensions, updates package.json entrypoints/exports and build/test scripts, introduces tsconfig variants for ESM/CJS/tests, tightens some types (any → unknown), and replaces the ESLint flat-config and related lint settings. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
![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
🧹 Nitpick comments (3)
packages/kg-lexical-html-renderer/test/cards.test.ts (1)
9-10: Prefer explicit test-local shapes overRecord<string, unknown>.Lines 9–10 use overly broad types that prevent TypeScript from validating nested property access. The test code actively depends on concrete nested fields (
root.children,imageOptimization.contentImageSizes,createDocument,target), and narrowing these to explicit interfaces will restore type safety instead of falling back tounknown.♻️ Proposed typing refinement
- let lexicalState: Record<string, unknown>; - let options: Record<string, unknown>; + type LexicalState = { + root: { + children: Array<Record<string, unknown>>; + direction: string | null; + format: string; + indent: number; + type: 'root'; + version: number; + }; + }; + + type RenderOptions = { + imageOptimization: { + contentImageSizes: Record<string, {width: number}>; + }; + createDocument: () => unknown; + target?: 'email'; + }; + + let lexicalState: LexicalState; + let options: RenderOptions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-lexical-html-renderer/test/cards.test.ts` around lines 9 - 10, The test declares overly broad types for lexicalState and options which prevents TS from validating nested accesses; replace Record<string, unknown> with small explicit interfaces matching the test's usage (include properties accessed like root: { children: unknown[] } or a proper Node[] shape for lexicalState, and for options include imageOptimization: { contentImageSizes: number[] }, createDocument: Function type matching usage, and target: string|Element as used), update the variable declarations for lexicalState and options to those interfaces, and adjust any helper usage (e.g., createDocument, imageOptimization.contentImageSizes, root.children, target) so TypeScript can type-check nested property access.packages/kg-lexical-html-renderer/test/utils/should-render.ts (1)
4-4: Create a freshJSDOMper test case.Line 4 shares a single DOM across every
shouldRenderinvocation. If any renderer path mutates the document or window state, later tests can become order-dependent. Construct the DOM inside the returned test function instead.♻️ Possible cleanup
-const dom = new JSDOM(); - function shouldRender({input, output, options = {}}: ShouldRenderParams) { return async function () { + const dom = new JSDOM(); const defaultOnError = (err: Error) => { throw err; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-lexical-html-renderer/test/utils/should-render.ts` at line 4, The shared JSDOM instance (const dom = new JSDOM()) causes state to leak between invocations of shouldRender; change the code so a new JSDOM() is constructed inside the returned test function instead of at module scope. Locate the shouldRender helper (the function that returns the test callback) and move the DOM creation into that returned callback so each test call gets its own dom/window/document, ensuring any mutations are isolated per invocation.packages/kg-lexical-html-renderer/package.json (1)
19-21: Extract the shared build chain once.Lines 19-21 repeat the same build pipeline three times. Pulling that into a dedicated script will keep
build,prepare, andpretestfrom drifting the next time the output layout changes.♻️ Possible cleanup
"scripts": { "dev": "tsc --watch --preserveWatchOutput --sourceMap", - "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: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 && 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-lexical-html-renderer/package.json` around lines 19 - 21, The build/prepare/pretest scripts in package.json duplicate the same pipeline; extract the common sequence into a single npm script (e.g., "build:core" or similar) and have "build", "prepare", and "pretest" call that script instead of repeating the commands. Update the package.json script entries "build", "prepare", and "pretest" to invoke the new shared script, ensuring the original behavior (running tsc, tsc -p tsconfig.cjs.json, and writing build/esm/package.json) is preserved.
🤖 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-lexical-html-renderer/package.json`:
- Line 23: The test script "test:unit" hardcodes node_modules/.bin/mocha which
breaks in hoisted workspaces; update the package.json "test:unit" script to
invoke mocha via the package-manager PATH (i.e., call mocha directly) and use
tsx only to register the TypeScript loader (e.g., run tsx to launch mocha rather
than referencing node_modules/.bin/mocha), so replace the
node_modules/.bin/mocha segment with just mocha in the "test:unit" script.
---
Nitpick comments:
In `@packages/kg-lexical-html-renderer/package.json`:
- Around line 19-21: The build/prepare/pretest scripts in package.json duplicate
the same pipeline; extract the common sequence into a single npm script (e.g.,
"build:core" or similar) and have "build", "prepare", and "pretest" call that
script instead of repeating the commands. Update the package.json script entries
"build", "prepare", and "pretest" to invoke the new shared script, ensuring the
original behavior (running tsc, tsc -p tsconfig.cjs.json, and writing
build/esm/package.json) is preserved.
In `@packages/kg-lexical-html-renderer/test/cards.test.ts`:
- Around line 9-10: The test declares overly broad types for lexicalState and
options which prevents TS from validating nested accesses; replace
Record<string, unknown> with small explicit interfaces matching the test's usage
(include properties accessed like root: { children: unknown[] } or a proper
Node[] shape for lexicalState, and for options include imageOptimization: {
contentImageSizes: number[] }, createDocument: Function type matching usage, and
target: string|Element as used), update the variable declarations for
lexicalState and options to those interfaces, and adjust any helper usage (e.g.,
createDocument, imageOptimization.contentImageSizes, root.children, target) so
TypeScript can type-check nested property access.
In `@packages/kg-lexical-html-renderer/test/utils/should-render.ts`:
- Line 4: The shared JSDOM instance (const dom = new JSDOM()) causes state to
leak between invocations of shouldRender; change the code so a new JSDOM() is
constructed inside the returned test function instead of at module scope. Locate
the shouldRender helper (the function that returns the test callback) and move
the DOM creation into that returned callback so each test call gets its own
dom/window/document, ensuring any mutations are isolated per invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f8589c9-47c3-48a9-baed-e64ec9f50998
📒 Files selected for processing (33)
packages/kg-lexical-html-renderer/eslint.config.mjspackages/kg-lexical-html-renderer/lib/index.tspackages/kg-lexical-html-renderer/lib/transformers/index.tspackages/kg-lexical-html-renderer/package.jsonpackages/kg-lexical-html-renderer/src/LexicalHTMLRenderer.tspackages/kg-lexical-html-renderer/src/convert-to-html-string.tspackages/kg-lexical-html-renderer/src/get-dynamic-data-nodes.tspackages/kg-lexical-html-renderer/src/index.tspackages/kg-lexical-html-renderer/src/kg-default-nodes.d.tspackages/kg-lexical-html-renderer/src/kg-utils.d.tspackages/kg-lexical-html-renderer/src/transformers/element/aside.tspackages/kg-lexical-html-renderer/src/transformers/element/blockquote.tspackages/kg-lexical-html-renderer/src/transformers/element/heading.tspackages/kg-lexical-html-renderer/src/transformers/element/list.tspackages/kg-lexical-html-renderer/src/transformers/element/paragraph.tspackages/kg-lexical-html-renderer/src/transformers/index.tspackages/kg-lexical-html-renderer/src/utils/TextContent.tspackages/kg-lexical-html-renderer/src/utils/generate-id.tspackages/kg-lexical-html-renderer/test/cards.test.tspackages/kg-lexical-html-renderer/test/headings.test.tspackages/kg-lexical-html-renderer/test/links.test.tspackages/kg-lexical-html-renderer/test/lists.test.tspackages/kg-lexical-html-renderer/test/quotes.test.tspackages/kg-lexical-html-renderer/test/render.test.tspackages/kg-lexical-html-renderer/test/text-formats.test.tspackages/kg-lexical-html-renderer/test/utils/assertions.tspackages/kg-lexical-html-renderer/test/utils/index.tspackages/kg-lexical-html-renderer/test/utils/overrides.tspackages/kg-lexical-html-renderer/test/utils/should-render.jspackages/kg-lexical-html-renderer/test/utils/should-render.tspackages/kg-lexical-html-renderer/tsconfig.cjs.jsonpackages/kg-lexical-html-renderer/tsconfig.jsonpackages/kg-lexical-html-renderer/tsconfig.test.json
💤 Files with no reviewable changes (3)
- packages/kg-lexical-html-renderer/lib/index.ts
- packages/kg-lexical-html-renderer/test/utils/should-render.js
- packages/kg-lexical-html-renderer/lib/transformers/index.ts
kevinansfield
force-pushed
the
ts/kg-lexical-html-renderer
branch
3 times, most recently
from
89df277 to
f75ef91
Compare
kevinansfield
commented
Mar 20, 2026
| const renderer = new Renderer({dom, nodes, onError: onError || defaultOnError} as Record<string, unknown>); | ||
| const renderedInput = await renderer.render(input, renderOptions); | ||
| renderedInput.should.equal(output); | ||
| (renderedInput as string).should.equal(output); |
Member
Author
There was a problem hiding this comment.
Why is this cast as string, doesn't renderer.render() always return a string?
kevinansfield
force-pushed
the
ts/kg-lexical-html-renderer
branch
3 times, most recently
from
e1a2f23 to
939ccf2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kg-lexical-html-renderer/eslint.config.mjs (1)
10-21: Turn offno-undeffor TypeScript files.
eslint.configs.recommendedenablesno-undef, and both ESLint and typescript-eslint document that this rule should generally be disabled on TypeScript because it doesn't understand TS/global types and is handled by the compiler. Add'no-undef': 'off'in this TS block before layering package-specific rules. (eslint.org)♻️ Proposed fix
rules: { ...ghostPlugin.configs.ts.rules, + 'no-undef': 'off', '@typescript-eslint/no-explicit-any': 'error' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-lexical-html-renderer/eslint.config.mjs` around lines 10 - 21, The TypeScript ESLint config is enabling the core no-undef rule via eslint.configs.recommended; update the rules object in the TS block (the object that currently spreads ghostPlugin.configs.ts.rules and sets '@typescript-eslint/no-explicit-any') to explicitly disable the core rule by adding 'no-undef': 'off' before layering package-specific rules (i.e., place it alongside the other entries in the rules object, prior to or immediately before ...ghostPlugin.configs.ts.rules) so TypeScript/ts-eslint handles undefined globals instead of core ESLint.
🤖 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-lexical-html-renderer/package.json`:
- Around line 10-16: The new "exports" field in package.json restricts package
subpaths and will break consumers that rely on deep imports (e.g.,
build/transformers/* or build/utils/* such as
`@tryghost/kg-lexical-html-renderer/build/transformers/element/aside.js`); either
remove or relax the "exports" map or explicitly add the needed subpath entries
under "exports" to re-export build/transformers/* and build/utils/*, or
otherwise prepare this as a major-version breaking change; locate the
package.json "exports" entry and update it to include explicit subpath keys for
the existing deep import folders (or revert the addition) so existing consumers
continue to work.
---
Nitpick comments:
In `@packages/kg-lexical-html-renderer/eslint.config.mjs`:
- Around line 10-21: The TypeScript ESLint config is enabling the core no-undef
rule via eslint.configs.recommended; update the rules object in the TS block
(the object that currently spreads ghostPlugin.configs.ts.rules and sets
'@typescript-eslint/no-explicit-any') to explicitly disable the core rule by
adding 'no-undef': 'off' before layering package-specific rules (i.e., place it
alongside the other entries in the rules object, prior to or immediately before
...ghostPlugin.configs.ts.rules) so TypeScript/ts-eslint handles undefined
globals instead of core ESLint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5649d4d-854b-4f59-93cc-3bacb509e2f4
📒 Files selected for processing (30)
packages/kg-lexical-html-renderer/eslint.config.mjspackages/kg-lexical-html-renderer/lib/index.tspackages/kg-lexical-html-renderer/lib/transformers/index.tspackages/kg-lexical-html-renderer/package.jsonpackages/kg-lexical-html-renderer/src/LexicalHTMLRenderer.tspackages/kg-lexical-html-renderer/src/convert-to-html-string.tspackages/kg-lexical-html-renderer/src/get-dynamic-data-nodes.tspackages/kg-lexical-html-renderer/src/index.tspackages/kg-lexical-html-renderer/src/kg-default-nodes.d.tspackages/kg-lexical-html-renderer/src/transformers/element/aside.tspackages/kg-lexical-html-renderer/src/transformers/element/blockquote.tspackages/kg-lexical-html-renderer/src/transformers/element/heading.tspackages/kg-lexical-html-renderer/src/transformers/element/list.tspackages/kg-lexical-html-renderer/src/transformers/element/paragraph.tspackages/kg-lexical-html-renderer/src/transformers/index.tspackages/kg-lexical-html-renderer/src/utils/TextContent.tspackages/kg-lexical-html-renderer/src/utils/generate-id.tspackages/kg-lexical-html-renderer/test/cards.test.tspackages/kg-lexical-html-renderer/test/headings.test.tspackages/kg-lexical-html-renderer/test/links.test.tspackages/kg-lexical-html-renderer/test/lists.test.tspackages/kg-lexical-html-renderer/test/quotes.test.tspackages/kg-lexical-html-renderer/test/render.test.tspackages/kg-lexical-html-renderer/test/text-formats.test.tspackages/kg-lexical-html-renderer/test/utils/index.tspackages/kg-lexical-html-renderer/test/utils/overrides.tspackages/kg-lexical-html-renderer/test/utils/should-render.tspackages/kg-lexical-html-renderer/tsconfig.cjs.jsonpackages/kg-lexical-html-renderer/tsconfig.jsonpackages/kg-lexical-html-renderer/tsconfig.test.json
💤 Files with no reviewable changes (2)
- packages/kg-lexical-html-renderer/lib/transformers/index.ts
- packages/kg-lexical-html-renderer/lib/index.ts
✅ Files skipped from review due to trivial changes (16)
- packages/kg-lexical-html-renderer/test/quotes.test.ts
- packages/kg-lexical-html-renderer/src/utils/generate-id.ts
- packages/kg-lexical-html-renderer/test/headings.test.ts
- packages/kg-lexical-html-renderer/test/links.test.ts
- packages/kg-lexical-html-renderer/src/get-dynamic-data-nodes.ts
- packages/kg-lexical-html-renderer/tsconfig.cjs.json
- packages/kg-lexical-html-renderer/src/utils/TextContent.ts
- packages/kg-lexical-html-renderer/test/lists.test.ts
- packages/kg-lexical-html-renderer/src/kg-default-nodes.d.ts
- packages/kg-lexical-html-renderer/tsconfig.test.json
- packages/kg-lexical-html-renderer/test/cards.test.ts
- packages/kg-lexical-html-renderer/src/convert-to-html-string.ts
- packages/kg-lexical-html-renderer/test/text-formats.test.ts
- packages/kg-lexical-html-renderer/src/index.ts
- packages/kg-lexical-html-renderer/test/render.test.ts
- packages/kg-lexical-html-renderer/src/transformers/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/kg-lexical-html-renderer/test/utils/index.ts
- packages/kg-lexical-html-renderer/src/transformers/element/aside.ts
- packages/kg-lexical-html-renderer/src/transformers/element/list.ts
- packages/kg-lexical-html-renderer/tsconfig.json
- packages/kg-lexical-html-renderer/src/LexicalHTMLRenderer.ts
- packages/kg-lexical-html-renderer/test/utils/should-render.ts
- packages/kg-lexical-html-renderer/src/transformers/element/heading.ts
- packages/kg-lexical-html-renderer/src/transformers/element/blockquote.ts
- packages/kg-lexical-html-renderer/src/transformers/element/paragraph.ts
- packages/kg-lexical-html-renderer/test/utils/overrides.ts
Comment on lines
+10
to
+16
| "exports": { | ||
| ".": { | ||
| "types": "./build/esm/index.d.ts", | ||
| "import": "./build/esm/index.js", | ||
| "require": "./build/cjs/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== published package subpaths =="
curl -sL "https://unpkg.com/%40tryghost/kg-lexical-html-renderer@1.3.38/?meta" | rg -n '"/build/(transformers|utils)/'
echo
echo "== repo references to deep imports =="
rg -nP '@tryghost/kg-lexical-html-renderer/' .Repository: TryGhost/Koenig
Length of output: 5923
🏁 Script executed:
#!/bin/bash
# Check the full package.json to see context around the exports field
cat -n packages/kg-lexical-html-renderer/package.jsonRepository: TryGhost/Koenig
Length of output: 2578
🏁 Script executed:
#!/bin/bash
# Search for any documentation or examples of deep imports in the package
rg -i 'import.*from.*kg-lexical-html-renderer.*/(transformers|utils)' . -t js -t ts -t mdRepository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there's a README with usage examples
cat packages/kg-lexical-html-renderer/README.md 2>/dev/null || echo "README not found"Repository: TryGhost/Koenig
Length of output: 2013
Adding an exports map restricts what consumers can import—this is a breaking change.
The exports field is new and adds encapsulation: anything not explicitly listed becomes inaccessible. The published package currently includes build/transformers/ and build/utils/, and any external consumer importing from those paths (e.g., @tryghost/kg-lexical-html-renderer/build/transformers/element/aside.js) will encounter ERR_PACKAGE_PATH_NOT_EXPORTED. If those deep imports are intentionally unsupported, this needs to ship as a major version bump. If they should remain available, add explicit subpath exports for them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-lexical-html-renderer/package.json` around lines 10 - 16, The new
"exports" field in package.json restricts package subpaths and will break
consumers that rely on deep imports (e.g., build/transformers/* or build/utils/*
such as `@tryghost/kg-lexical-html-renderer/build/transformers/element/aside.js`);
either remove or relax the "exports" map or explicitly add the needed subpath
entries under "exports" to re-export build/transformers/* and build/utils/*, or
otherwise prepare this as a major-version breaking change; locate the
package.json "exports" entry and update it to include explicit subpath keys for
the existing deep import folders (or revert the addition) so existing consumers
continue to work.
- Move lib/ to src/, update tsconfig rootDir - Update tsconfig.json: NodeNext module, verbatimModuleSyntax, declarationMap - Add "type": "module" to package.json - Convert source from CJS to ESM (require -> import, module.exports -> export) - Add .js extensions to all relative imports - Rename test files .js to .ts with type annotations - Switch test runner to tsx - Replace .eslintrc.js with eslint.config.js (flat config)
kevinansfield
force-pushed
the
ts/kg-lexical-html-renderer
branch
from
939ccf2 to
29d24b4
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
1 participant
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
lib/tosrc/and test files to.tsanywithunknownin theDecoratorNodetype shim andrenderDatamapLexicalHTMLRenderer) for correct CJSrequire()shape"node"from exports, fixesmoduleResolutiontoNodeNext, replaces deprecatedtseslint.config()withdefineConfig(), addstsconfig.test.json, changes test imports frombuild/cjsto source, adds/* c8 ignore */for V8 phantom branches