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:
WalkthroughMigrates packages/kg-default-atoms from CommonJS/lib JS to TypeScript/src with build outputs (build/esm and build/cjs). Removes legacy JS files/exports (index.js, lib atoms, soft-return JS) and adds TS sources for the soft-return atom and a package root re-export. Updates tests and test utilities from JS to TS, replaces global test plumbing with TypeScript-friendly globals, adds tsconfig and tsconfig.cjs, updates package.json for dual ESM/CJS exports, scripts, types, devDependencies, converts ESLint config to typescript-eslint, and adds .gitignore entries for build artifacts. 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 unit tests (beta)
📝 Coding Plan
Comment |
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
from
951611d to
2c8934c
Compare
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
from
2c8934c to
25e4996
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1759 +/- ##
==========================================
+ Coverage 88.51% 88.64% +0.12%
==========================================
Files 201 200 -1
Lines 18456 18454 -2
Branches 2019 2021 +2
==========================================
+ Hits 16337 16358 +21
+ Misses 2108 2085 -23
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
2 times, most recently
from
c53a493 to
ca0f5e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kg-default-atoms/package.json (1)
13-13: Use type-check only inpretestto avoid redundant emits.Line 13 currently rebuilds artifacts on every test run. Switching
pretestto no-emit keeps CI signal the same while reducing churn and runtime.♻️ Proposed fix
- "pretest": "tsc", + "pretest": "tsc --noEmit",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/package.json` at line 13, The pretest script in package.json currently runs "tsc" which emits artifacts; update the "pretest" script to run type-check only by replacing "tsc" with "tsc --noEmit" (or "tsc -p . --noEmit" if you prefer explicit project flag) so tests run type-checking without rebuilding artifacts; edit the "pretest" entry in package.json accordingly.
🤖 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-default-atoms/package.json`:
- Line 13: The pretest script in package.json currently runs "tsc" which emits
artifacts; update the "pretest" script to run type-check only by replacing "tsc"
with "tsc --noEmit" (or "tsc -p . --noEmit" if you prefer explicit project flag)
so tests run type-checking without rebuilding artifacts; edit the "pretest"
entry in package.json accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da5361fb-cbd8-4ad2-8bbb-ad445a187d4c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/kg-default-atoms/.gitignorepackages/kg-default-atoms/eslint.config.mjspackages/kg-default-atoms/index.jspackages/kg-default-atoms/lib/atoms/index.jspackages/kg-default-atoms/lib/atoms/soft-return.jspackages/kg-default-atoms/package.jsonpackages/kg-default-atoms/src/atoms/index.tspackages/kg-default-atoms/src/atoms/soft-return.tspackages/kg-default-atoms/src/index.tspackages/kg-default-atoms/test/atoms/soft-return.test.jspackages/kg-default-atoms/test/atoms/soft-return.test.tspackages/kg-default-atoms/test/utils/assertions.tspackages/kg-default-atoms/test/utils/index.tspackages/kg-default-atoms/test/utils/overrides.jspackages/kg-default-atoms/test/utils/overrides.tspackages/kg-default-atoms/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-default-atoms/lib/atoms/index.js
- packages/kg-default-atoms/test/utils/overrides.js
- packages/kg-default-atoms/lib/atoms/soft-return.js
- packages/kg-default-atoms/index.js
- packages/kg-default-atoms/test/atoms/soft-return.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/kg-default-atoms/test/utils/overrides.ts
- packages/kg-default-atoms/test/atoms/soft-return.test.ts
- packages/kg-default-atoms/src/index.ts
- packages/kg-default-atoms/src/atoms/index.ts
- packages/kg-default-atoms/src/atoms/soft-return.ts
- packages/kg-default-atoms/eslint.config.mjs
- packages/kg-default-atoms/.gitignore
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
2 times, most recently
from
05a2c23 to
3b64175
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/kg-default-atoms/src/atoms/soft-return.ts (1)
1-5: Theunknownreturn type oncreateElementdoes limit type information in strict TypeScript mode. However, the proposed generic solution has a critical flaw: theTElementtype parameter is unconstrained and has no inference mechanism from the'br'tag string, so callers would be forced to explicitly specify the type without gaining meaningful type safety. A more robust solution would require const generics to map tag names to element types, which adds complexity that may not be warranted here.For now, the current typing is acceptable since it remains DOM-implementation agnostic (supporting SimpleDom, jsdom, real DOM, etc.) and the actual runtime behavior is correct. If stricter typing is needed in the future, consider a mapped type approach where specific tag strings map to their proper element types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/src/atoms/soft-return.ts` around lines 1 - 5, The current change replacing the DOM-agnostic return type on AtomEnv.dom.createElement with an unconstrained generic should be reverted: keep createElement(tag: string): unknown on the AtomEnv interface so the typing remains implementation-agnostic (supports SimpleDom/jsdom/real DOM) and avoids forcing callers to supply explicit type arguments; do not introduce an unconstrained TElement generic on createElement (like createElement<TElement>(tag: string))—if stricter typing is needed later, implement a mapped tag-to-element type approach rather than the current unconstrained generic.packages/kg-default-atoms/package.json (1)
20-23: Build pipeline should cleanbuild/and avoid duplicated script bodies.
tscdoes not remove orphaned output files. Without a clean step, stale files can remain in published artifacts ("files": ["build"]). Also,build,prepare, andpretestduplicate the same command.Suggested 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", + "clean": "node -e \"require('node:fs').rmSync('build',{recursive:true,force:true})\"", + "build": "yarn clean && tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "prepare": "yarn build", + "pretest": "yarn build", "test": "NODE_ENV=testing c8 --all --reporter text --reporter cobertura mocha --require tsx './test/**/*.test.ts'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/package.json` around lines 20 - 23, Add a "clean" script that removes the build/ folder and factor the repeated tsc + echo command into a single "compile" script, then update the existing "build", "prepare", and "pretest" scripts to call the new scripts (e.g., "npm run clean && npm run compile" or have "compile" invoke clean) so you avoid duplicate command bodies and ensure stale files are removed before building; update "test" to rely on "pretest" as needed so the pipeline runs the cleaned compile step.packages/kg-default-atoms/eslint.config.mjs (1)
13-15: MoveecmaVersionandsourceTypeto top-levellanguageOptions.In ESLint flat config (v9.x), these are top-level properties of
languageOptions, not nested underparserOptions. TheparserOptionskey is reserved for parser-specific options. Moving them clarifies the config intent and follows the v9 migration guidance.🔧 Proposed refactor
languageOptions: { - parserOptions: {ecmaVersion: 2022, sourceType: 'module'} + ecmaVersion: 2022, + sourceType: 'module' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/eslint.config.mjs` around lines 13 - 15, The ESLint config currently nests ecmaVersion and sourceType under languageOptions.parserOptions; update the config so ecmaVersion and sourceType are direct properties of languageOptions (remove the parserOptions wrapper) to comply with ESLint flat config v9.x conventions; adjust any references to languageOptions.parserOptions in the same file to use languageOptions.ecmaVersion and languageOptions.sourceType and keep parser-specific options only under parserOptions if needed.
🤖 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-default-atoms/eslint.config.mjs`:
- Line 5: Replace the deprecated tseslint.config call with ESLint's modern
defineConfig: import defineConfig from "eslint/config" (or adjust existing
import) and change the export to use defineConfig([...]) instead of
tseslint.config([...]); update any tseslint-specific behavior inside the array
to match defineConfig expectations so the configuration remains equivalent while
removing the deprecated helper.
In `@packages/kg-default-atoms/package.json`:
- Around line 10-16: The "exports" conditional order causes ESM imports to pick
the "node" (CJS) target first; reorder the export condition keys so the "import"
condition appears before "node" (keep "types" first), e.g. change the key order
from ["types","node","import","require"] to ["types","import","require","node"]
so ESM resolution picks the ESM build; update the package.json exports object
accordingly (touch the "exports" block and the "types","import","require","node"
keys).
---
Nitpick comments:
In `@packages/kg-default-atoms/eslint.config.mjs`:
- Around line 13-15: The ESLint config currently nests ecmaVersion and
sourceType under languageOptions.parserOptions; update the config so ecmaVersion
and sourceType are direct properties of languageOptions (remove the
parserOptions wrapper) to comply with ESLint flat config v9.x conventions;
adjust any references to languageOptions.parserOptions in the same file to use
languageOptions.ecmaVersion and languageOptions.sourceType and keep
parser-specific options only under parserOptions if needed.
In `@packages/kg-default-atoms/package.json`:
- Around line 20-23: Add a "clean" script that removes the build/ folder and
factor the repeated tsc + echo command into a single "compile" script, then
update the existing "build", "prepare", and "pretest" scripts to call the new
scripts (e.g., "npm run clean && npm run compile" or have "compile" invoke
clean) so you avoid duplicate command bodies and ensure stale files are removed
before building; update "test" to rely on "pretest" as needed so the pipeline
runs the cleaned compile step.
In `@packages/kg-default-atoms/src/atoms/soft-return.ts`:
- Around line 1-5: The current change replacing the DOM-agnostic return type on
AtomEnv.dom.createElement with an unconstrained generic should be reverted: keep
createElement(tag: string): unknown on the AtomEnv interface so the typing
remains implementation-agnostic (supports SimpleDom/jsdom/real DOM) and avoids
forcing callers to supply explicit type arguments; do not introduce an
unconstrained TElement generic on createElement (like
createElement<TElement>(tag: string))—if stricter typing is needed later,
implement a mapped tag-to-element type approach rather than the current
unconstrained generic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 181404a6-1028-4030-abc0-95fffbdf79f6
📒 Files selected for processing (17)
packages/kg-default-atoms/.gitignorepackages/kg-default-atoms/eslint.config.mjspackages/kg-default-atoms/index.jspackages/kg-default-atoms/lib/atoms/index.jspackages/kg-default-atoms/lib/atoms/soft-return.jspackages/kg-default-atoms/package.jsonpackages/kg-default-atoms/src/atoms/index.tspackages/kg-default-atoms/src/atoms/soft-return.tspackages/kg-default-atoms/src/index.tspackages/kg-default-atoms/test/atoms/soft-return.test.jspackages/kg-default-atoms/test/atoms/soft-return.test.tspackages/kg-default-atoms/test/utils/assertions.tspackages/kg-default-atoms/test/utils/index.tspackages/kg-default-atoms/test/utils/overrides.jspackages/kg-default-atoms/test/utils/overrides.tspackages/kg-default-atoms/tsconfig.cjs.jsonpackages/kg-default-atoms/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-default-atoms/index.js
- packages/kg-default-atoms/lib/atoms/index.js
- packages/kg-default-atoms/test/utils/overrides.js
- packages/kg-default-atoms/test/atoms/soft-return.test.js
- packages/kg-default-atoms/lib/atoms/soft-return.js
✅ Files skipped from review due to trivial changes (1)
- packages/kg-default-atoms/.gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-default-atoms/test/atoms/soft-return.test.ts
- packages/kg-default-atoms/src/index.ts
- packages/kg-default-atoms/src/atoms/index.ts
- packages/kg-default-atoms/test/utils/index.ts
- packages/kg-default-atoms/tsconfig.json
| const ghost = fixupPluginRules(ghostPlugin); | ||
|
|
||
| export default [ | ||
| export default tseslint.config([ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Is tseslint.config(...)deprecated in typescript-eslint v8.33.1, and isdefineConfig(...) the recommended replacement for flat config?
💡 Result:
Yes.
- In typescript-eslint’s docs, the
config(...)utility (used astseslint.config(...)) is explicitly deprecated “in favor of ESLint core’sdefineConfig(...)”. [1] - For flat config, the docs’ “Getting Started” example uses and recommends composing configs via
defineConfig(...)fromeslint/config(e.g.,export default defineConfig(eslint.configs.recommended, tseslint.configs.recommended)). [2] - The migration section notes ESLint’s
defineConfig(...)is intended as a near-drop-in replacement fortseslint.config(...), with a few known edge-case differences. [3]
Sources:
[1] (typescript-eslint.io)
[2] (typescript-eslint.io)
[3] (typescript-eslint.io)
Citations:
- 1: https://typescript-eslint.io/packages/typescript-eslint
- 2: https://typescript-eslint.io/getting-started/
- 3: https://typescript-eslint.io/packages/typescript-eslint
Use defineConfig instead of deprecated tseslint.config.
Line 5 introduces a deprecated helper. Migrate to ESLint's defineConfig from eslint/config to align with current best practices and avoid future compatibility issues.
🔧 Refactor
import eslint from '@eslint/js';
+import {defineConfig} from 'eslint/config';
import ghostPlugin from 'eslint-plugin-ghost';
import tseslint from 'typescript-eslint';
-export default tseslint.config([
+export default defineConfig([
{ignores: ['build/**']},
{
files: ['**/*.ts'],
@@
-]);
+]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-atoms/eslint.config.mjs` at line 5, Replace the
deprecated tseslint.config call with ESLint's modern defineConfig: import
defineConfig from "eslint/config" (or adjust existing import) and change the
export to use defineConfig([...]) instead of tseslint.config([...]); update any
tseslint-specific behavior inside the array to match defineConfig expectations
so the configuration remains equivalent while removing the deprecated helper.
| "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
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json
from pathlib import Path
pkg = json.loads(Path("packages/kg-default-atoms/package.json").read_text())
conds = list(pkg.get("exports", {}).get(".", {}).keys())
print("export condition order:", conds)
if "node" in conds and "import" in conds and conds.index("node") < conds.index("import"):
print("WARNING: 'node' is before 'import' and may shadow Node ESM import resolution.")
PYRepository: TryGhost/Koenig
Length of output: 199
🌐 Web query:
Node.js conditional exports resolution algorithm import require node condition order
💡 Result:
Node.js conditional exports resolution: condition order + import vs require
When resolving a package entry via package.json "exports", Node evaluates conditional exports by iterating keys in object insertion order and picking the first matching condition. Key order is therefore part of the resolution algorithm. [1]
1) Active conditions depend on how you load the package
Node defines these core conditions (recommended to be written most-specific → least-specific): [1]
"node-addons""node""import"— when loaded viaimport/import()/ ESM resolver; mutually exclusive with"require"[1]"require"— when loaded viarequire(); mutually exclusive with"import"[1]"module-sync"— can match for bothimportandrequire(with restrictions) [1]"default"— always matches; should be last [1]
You can add extra user conditions with node --conditions=name ... (e.g. development), and Node will resolve those in addition to the built-ins. [1]
2) Matching algorithm (conceptual)
Given an "exports" target that is a conditions object:
- Node checks each key top-to-bottom (object order).
- The first key that is in the current active condition set is selected.
- If the selected value is another conditions object, Node continues similarly (nested conditions behave like nested
ifstatements). [1]
3) Practical implication for import vs require
If you write:
{
"exports": {
"import": "./index.mjs",
"require": "./index.cjs",
"default": "./fallback.js"
}
}import "pkg"matches"import"first.require("pkg")matches"require"first.- Anything that doesn’t match earlier conditions falls through to
"default"(so put"default"last). [1]
Source: Node.js “Packages” documentation (Conditional exports). [1]
🌐 Web query:
Node.js package.json exports field condition priority matching ESM CommonJS
💡 Result:
Node.js package.json#exports conditional matching: priority + ESM vs CJS
1) Condition priority is object key order
In a conditional exports object, earlier keys win: Node tries the conditions in the order they appear in exports, and the first matching condition takes precedence. So you should order keys from “most specific” → “least specific”. Node also recommends including a "default" fallback for unknown environments. [1]
2) Which conditions are active depends on how you load the package
-
ESM (
import) resolution uses Node’s default condition set:["node", "import"](in that order). [2]
-
CommonJS (
require) resolution uses:["node", "require"]- and if
require(esm)support is enabled (historically behind--experimental-require-module, and in newer Node lines often on by default), Node also includes"module-sync":["node", "require", "module-sync"]. [3]
So for Node itself, "import" and "require" generally don’t “compete” in the same request—import resolution won’t match "require", and vice versa; Node picks the branch whose key matches one of the active conditions for that resolver, using key order to break ties among present conditions. [1][2][3]
3) Nested condition objects behave like nested if statements
Node supports nesting (e.g. "node": { "import": ..., "require": ... }), and it still matches conditions in order; if a nested condition object doesn’t produce a mapping, Node continues checking remaining conditions at the parent level. [1]
4) TypeScript note (if you care about tooling consistency)
When TypeScript follows Node’s "exports" rules, it always considers "types" and "default" conditions if present (in addition to the mode-dependent conditions like "node"/"import"/"require"). [4]
Practical takeaway: put "types" (if used) first, then your most specific runtime/bundler conditions, and keep "default" last. [1][4]
Move "import" before "node" in the exports conditions.
The current order ["types", "node", "import", "require"] causes Node ESM imports to resolve to the "node" condition first (which points to CJS), since Node matches conditional exports in object key order and both conditions are active for ESM resolution. Reorder to ["types", "import", "require", ...] so ESM resolves to the correct ESM build.
Suggested 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"
}
},📝 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.
| "exports": { | |
| ".": { | |
| "types": "./build/esm/index.d.ts", | |
| "node": "./build/cjs/index.js", | |
| "import": "./build/esm/index.js", | |
| "require": "./build/cjs/index.js" | |
| } | |
| "exports": { | |
| ".": { | |
| "types": "./build/esm/index.d.ts", | |
| "import": "./build/esm/index.js", | |
| "require": "./build/cjs/index.js" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-atoms/package.json` around lines 10 - 16, The "exports"
conditional order causes ESM imports to pick the "node" (CJS) target first;
reorder the export condition keys so the "import" condition appears before
"node" (keep "types" first), e.g. change the key order from
["types","node","import","require"] to ["types","import","require","node"] so
ESM resolution picks the ESM build; update the package.json exports object
accordingly (touch the "exports" block and the "types","import","require","node"
keys).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/kg-default-atoms/test/utils/index.ts (1)
7-11: Update stale comments to match ESM importsLine 7 and Line 10 still say “Require”, but the code now uses
import. Small clarity fix.Suggested wording update
-// Require overrides - these add globals for tests +// Import overrides - these add globals for tests import './overrides.js'; -// Require assertions - adds custom should assertions +// Import assertions - adds custom should assertions import './assertions.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/test/utils/index.ts` around lines 7 - 11, Update the stale comments that say "Require" to accurately reflect ESM usage: change the comment above the import of './overrides.js' and the comment above the import of './assertions.js' to use "Import" (or similar wording) so comments match the actual ESM import statements in this file where './overrides.js' and './assertions.js' are imported.packages/kg-default-atoms/test/atoms/soft-return.test.ts (2)
15-15: Use an exact assertion for serialized outputLine 15 uses a permissive substring match. Prefer exact equality so markup regressions are caught.
Suggested assertion tightening
- serializer.serialize(atom.render(opts)).should.match('<br>'); + serializer.serialize(atom.render(opts)).should.equal('<br>');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/test/atoms/soft-return.test.ts` at line 15, The test currently uses a substring match on the serialized output; update the assertion so it checks exact equality of the serialized markup. Replace the permissive matcher on the expression serializer.serialize(atom.render(opts)) (currently .should.match('<br>')) with a strict equality assertion such as .should.equal('<br>') to ensure the test fails on any markup regressions.
1-3: Add one integration check for the package root exportThe new test validates
soft-returndirectly, but not thesrc/index.ts→src/atoms/index.tsexport path. Add a small assertion to cover the public default export list.Suggested integration assertion
import '../utils/index.js'; import atom from '../../src/atoms/soft-return.js'; +import atoms from '../../src/index.js'; import {Document as SimpleDomDocument, HTMLSerializer, voidMap} from 'simple-dom'; @@ describe('Soft return atom', function () { it('generates a `br` tag', function () { @@ serializer.serialize(atom.render(opts)).should.equal('<br>'); }); + + it('is included in the package default atoms export', function () { + atoms.should.containEql(atom); + }); });Also applies to: 7-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/test/atoms/soft-return.test.ts` around lines 1 - 3, Add an integration assertion that the package root export surface (imported from the package entry that re-exports src/index.ts → src/atoms/index.ts) includes the same soft-return atom you import in this test (the local symbol atom from '../../src/atoms/soft-return.js'); import the package root default/object export and assert that it contains or references the same atom (e.g., Object.values(packageRootExport).includes(atom) or strict equality against the appropriate key) so the public export path is covered.
🤖 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-default-atoms/test/atoms/soft-return.test.ts`:
- Line 15: The test currently uses a substring match on the serialized output;
update the assertion so it checks exact equality of the serialized markup.
Replace the permissive matcher on the expression
serializer.serialize(atom.render(opts)) (currently .should.match('<br>')) with a
strict equality assertion such as .should.equal('<br>') to ensure the test fails
on any markup regressions.
- Around line 1-3: Add an integration assertion that the package root export
surface (imported from the package entry that re-exports src/index.ts →
src/atoms/index.ts) includes the same soft-return atom you import in this test
(the local symbol atom from '../../src/atoms/soft-return.js'); import the
package root default/object export and assert that it contains or references the
same atom (e.g., Object.values(packageRootExport).includes(atom) or strict
equality against the appropriate key) so the public export path is covered.
In `@packages/kg-default-atoms/test/utils/index.ts`:
- Around line 7-11: Update the stale comments that say "Require" to accurately
reflect ESM usage: change the comment above the import of './overrides.js' and
the comment above the import of './assertions.js' to use "Import" (or similar
wording) so comments match the actual ESM import statements in this file where
'./overrides.js' and './assertions.js' are imported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e0f55bb-58e7-41c0-a7a7-97a14c3ef0b5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/kg-default-atoms/.gitignorepackages/kg-default-atoms/eslint.config.mjspackages/kg-default-atoms/index.jspackages/kg-default-atoms/package.jsonpackages/kg-default-atoms/src/atoms/index.tspackages/kg-default-atoms/src/atoms/soft-return.tspackages/kg-default-atoms/src/index.tspackages/kg-default-atoms/test/atoms/soft-return.test.tspackages/kg-default-atoms/test/utils/index.tspackages/kg-default-atoms/test/utils/overrides.tspackages/kg-default-atoms/tsconfig.cjs.jsonpackages/kg-default-atoms/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/kg-default-atoms/index.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/kg-default-atoms/tsconfig.json
- packages/kg-default-atoms/test/utils/overrides.ts
- packages/kg-default-atoms/src/atoms/soft-return.ts
- packages/kg-default-atoms/package.json
- packages/kg-default-atoms/.gitignore
- packages/kg-default-atoms/tsconfig.cjs.json
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
from
3b64175 to
fc146d0
Compare
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
from
fc146d0 to
1851619
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/kg-default-atoms/package.json (1)
12-15:⚠️ Potential issue | 🟠 MajorReorder export conditions so ESM imports resolve to ESM output.
At Line 13,
"node"appears before"import"(Line 14). In conditional exports, that ordering can make Nodeimportresolve to the CJS path first. Put"import"before"node"(or remove"node"if redundant).Suggested change
"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", + "node": "./build/cjs/index.js" } },#!/bin/bash python - <<'PY' import json from pathlib import Path pkg = json.loads(Path("packages/kg-default-atoms/package.json").read_text()) conds = list(pkg["exports"]["."].keys()) print("Export condition order:", conds) active_for_import = {"node", "import", "default"} first = next((c for c in conds if c in active_for_import), None) print("First matching condition for Node ESM import (conceptual):", first) if "node" in conds and "import" in conds and conds.index("node") < conds.index("import"): print("WARNING: 'node' precedes 'import' and may select CJS for ESM imports.") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-atoms/package.json` around lines 12 - 15, The export conditions in package.json currently list "node" before "import", which can cause Node ESM imports to resolve to the CJS path; update the export map for "." so that the "import" condition appears before "node" (or remove the redundant "node" condition entirely) ensuring the keys order puts "import" prior to "node" under the exports object (the fields to change are "import", "node", "require", "default" in the package.json export mapping).
🧹 Nitpick comments (1)
packages/kg-default-atoms/package.json (1)
20-22: Deduplicate repeated build command across scripts.Lines 20-22 duplicate the same build pipeline. Consider making
prepare/pretestcallyarn buildto avoid drift.Suggested change
- "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": "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-default-atoms/package.json` around lines 20 - 22, The package.json currently duplicates the same build pipeline in the "build", "prepare", and "pretest" scripts; update "prepare" and "pretest" to invoke the single source of truth ("build") instead of repeating commands — e.g., change the "prepare" and "pretest" script values to run "yarn build" (or "npm run build") so the "build" script remains the canonical command and avoids drift; edit the scripts in package.json accordingly and keep "build" unchanged.
🤖 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-default-atoms/package.json`:
- Around line 12-15: The export conditions in package.json currently list "node"
before "import", which can cause Node ESM imports to resolve to the CJS path;
update the export map for "." so that the "import" condition appears before
"node" (or remove the redundant "node" condition entirely) ensuring the keys
order puts "import" prior to "node" under the exports object (the fields to
change are "import", "node", "require", "default" in the package.json export
mapping).
---
Nitpick comments:
In `@packages/kg-default-atoms/package.json`:
- Around line 20-22: The package.json currently duplicates the same build
pipeline in the "build", "prepare", and "pretest" scripts; update "prepare" and
"pretest" to invoke the single source of truth ("build") instead of repeating
commands — e.g., change the "prepare" and "pretest" script values to run "yarn
build" (or "npm run build") so the "build" script remains the canonical command
and avoids drift; edit the scripts in package.json accordingly and keep "build"
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30a283ab-9016-4ddf-81b5-055ea399251a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
packages/kg-default-atoms/.gitignorepackages/kg-default-atoms/eslint.config.mjspackages/kg-default-atoms/index.jspackages/kg-default-atoms/lib/atoms/index.jspackages/kg-default-atoms/lib/atoms/soft-return.jspackages/kg-default-atoms/package.jsonpackages/kg-default-atoms/src/atoms/index.tspackages/kg-default-atoms/src/atoms/soft-return.tspackages/kg-default-atoms/src/index.tspackages/kg-default-atoms/test/atoms/soft-return.test.jspackages/kg-default-atoms/test/atoms/soft-return.test.tspackages/kg-default-atoms/test/utils/assertions.tspackages/kg-default-atoms/test/utils/index.tspackages/kg-default-atoms/test/utils/overrides.jspackages/kg-default-atoms/test/utils/overrides.tspackages/kg-default-atoms/tsconfig.cjs.jsonpackages/kg-default-atoms/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-default-atoms/lib/atoms/index.js
- packages/kg-default-atoms/index.js
- packages/kg-default-atoms/lib/atoms/soft-return.js
- packages/kg-default-atoms/test/utils/overrides.js
- packages/kg-default-atoms/test/atoms/soft-return.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/kg-default-atoms/src/atoms/index.ts
- packages/kg-default-atoms/test/utils/index.ts
- packages/kg-default-atoms/.gitignore
- packages/kg-default-atoms/test/utils/overrides.ts
- packages/kg-default-atoms/test/atoms/soft-return.test.ts
- packages/kg-default-atoms/src/index.ts
- packages/kg-default-atoms/tsconfig.cjs.json
- 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 throughout - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc
kevinansfield
force-pushed
the
ts/kg-default-atoms
branch
from
1851619 to
40eaace
Compare
2 participants
Summary
Test plan
yarn testpasses in kg-default-atomsyarn lintpasses in kg-default-atoms