Fixing package.json module paths and remove helpers#295
Fixing package.json module paths and remove helpers#295
Conversation
- Fixing the repackage script to conditionally include "helpers/" based on its existence. - Fix the "module" entry in multiple package.json files to point to "dist/index.js" instead of specific binding files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdated many package.json files to standardize ESM entrypoints to Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as tools/repackage.mjs
participant FS as Filesystem
participant PKG as package.json (output)
Tool->>FS: check existence of "helpers/" (fs.access)
FS-->>Tool: hasHelpers? (true/false)
Tool->>Tool: build packageData (include helpers when true)
Tool->>PKG: write package.json with conditional "files" and "exports"
PKG-->>Tool: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/observable/package.json (1)
9-19:⚠️ Potential issue | 🟠 MajorAdd a changeset for the exported surface change.
Removing
./helpers/*changes public import behavior. Please include a.changesetentry documenting affected packages and bump type.Based on learnings: Applies to .changeset/** : Create changeset files with
npx changeset addfor PRs that change package behavior to document affected packages, bump type, and describe changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/observable/package.json` around lines 9 - 19, The package export change removed the "./helpers/*" public surface, so add a changeset file documenting this export removal and the package(s) affected: run npx changeset add (or create a .changeset/*.md file) naming the package(s) in packages/observable, set an appropriate bump type (minor or patch per your release policy — use minor if this is a breaking export change), and include a short description that the "./helpers/*" exports were removed and how consumers should migrate; ensure the changeset is committed under .changeset/.packages/utils.parser/package.json (1)
32-38:⚠️ Potential issue | 🟠 MajorAdd a changeset entry to document the removal of the
./helpers/*public export from utils.parser.Lines 32-38 remove the
./helpers/*public subpath export, which is a breaking API change. This package behavior change must be documented in a changeset file usingnpx changeset addto track the affected package, semver bump type, and changes for release notes. No changeset entry was found for this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils.parser/package.json` around lines 32 - 38, This change removes the public subpath export "./helpers/*" from the utils.parser package which is a breaking API change; add a changeset by running npx changeset add and select the utils.parser package, describe that "./helpers/*" subpath export was removed, and set the semver bump to major (breaking change) so release notes and versioning reflect the removal.packages/provider.bindingstring/package.json (1)
27-33:⚠️ Potential issue | 🟠 MajorCreate a changeset entry to document the breaking removal of the
./helpers/*export.The
./helpers/*export was removed from the package exports (previously defined as"./helpers/*": "./helpers/*"). This is a breaking change for any consumers importing@tko/provider.bindingstring/helpers/*. Add a changeset entry usingnpx changeset addto document this package behavior change before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/provider.bindingstring/package.json` around lines 27 - 33, Add a Changeset documenting the breaking removal of the "./helpers/*" export from the package; run `npx changeset add`, choose the package corresponding to provider.bindingstring (packages/provider.bindingstring), mark the change as a breaking change, describe that the `./helpers/*` export was removed and consumers importing `@tko/provider.bindingstring/helpers/*` must update imports or restore helpers, and save the generated markdown changeset file before merging.
🤖 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/binding.foreach/package.json`:
- Around line 5-9: The package.json was changed to alter the published
entrypoints ("module": "dist/index.js" and "files": ["dist/","types/"]) and this
is a public packaging change that requires a Changeset; run npx changeset add
and create a .changeset/*.md entry documenting the package
(packages/binding.foreach) with a description of the breaking/public change and
the appropriate version bump (patch/minor/major as applicable) so the change
will be published and tracked; ensure the changeset file names the package and
the change type, and commit the new .changeset/*.md file alongside the
package.json change.
In `@tools/repackage.mjs`:
- Around line 10-11: The helper function hasDir currently treats any fs.access
error as "not present" which hides permission/IO errors; update hasDir to only
return false for a missing path (e.g., err.code === 'ENOENT') and rethrow any
other errors so permission/IO problems fail fast. Locate the hasDir function and
change its promise rejection handler to inspect err.code and return false only
for ENOENT (optionally ENOTDIR if desired), otherwise throw or reject the
original error instead of returning false.
---
Outside diff comments:
In `@packages/observable/package.json`:
- Around line 9-19: The package export change removed the "./helpers/*" public
surface, so add a changeset file documenting this export removal and the
package(s) affected: run npx changeset add (or create a .changeset/*.md file)
naming the package(s) in packages/observable, set an appropriate bump type
(minor or patch per your release policy — use minor if this is a breaking export
change), and include a short description that the "./helpers/*" exports were
removed and how consumers should migrate; ensure the changeset is committed
under .changeset/.
In `@packages/provider.bindingstring/package.json`:
- Around line 27-33: Add a Changeset documenting the breaking removal of the
"./helpers/*" export from the package; run `npx changeset add`, choose the
package corresponding to provider.bindingstring
(packages/provider.bindingstring), mark the change as a breaking change,
describe that the `./helpers/*` export was removed and consumers importing
`@tko/provider.bindingstring/helpers/*` must update imports or restore helpers,
and save the generated markdown changeset file before merging.
In `@packages/utils.parser/package.json`:
- Around line 32-38: This change removes the public subpath export "./helpers/*"
from the utils.parser package which is a breaking API change; add a changeset by
running npx changeset add and select the utils.parser package, describe that
"./helpers/*" subpath export was removed, and set the semver bump to major
(breaking change) so release notes and versioning reflect the removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2242e0fd-c366-4a71-af8f-6179fab85385
📒 Files selected for processing (26)
builds/reference/package.jsonpackages/bind/package.jsonpackages/binding.component/package.jsonpackages/binding.core/package.jsonpackages/binding.foreach/package.jsonpackages/binding.if/package.jsonpackages/binding.template/package.jsonpackages/builder/package.jsonpackages/computed/package.jsonpackages/filter.punches/package.jsonpackages/lifecycle/package.jsonpackages/observable/package.jsonpackages/provider.attr/package.jsonpackages/provider.bindingstring/package.jsonpackages/provider.component/package.jsonpackages/provider.databind/package.jsonpackages/provider.multi/package.jsonpackages/provider.mustache/package.jsonpackages/provider.native/package.jsonpackages/provider.virtual/package.jsonpackages/provider/package.jsonpackages/utils.component/package.jsonpackages/utils.functionrewrite/package.jsonpackages/utils.jsx/package.jsonpackages/utils.parser/package.jsontools/repackage.mjs
| "module": "dist/index.js", | ||
| "files": [ | ||
| "dist/", | ||
| "helpers/", | ||
| "types/" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this PR includes a changeset describing affected packages.
fd -e md . .changeset
rg -n '@tko/(binding\.foreach|filter\.punches|provider\.multi|utils\.component|binding\.component|provider\.mustache|utils\.functionrewrite|binding\.core)' .changesetRepository: knockout/tko
Length of output: 527
🏁 Script executed:
#!/bin/bash
# List all files in .changeset directory to identify actual changeset entries
ls -lah .changeset/ | grep -v "^d" | grep -v "^total"
# Also count markdown files (excluding README)
find .changeset -maxdepth 1 -name "*.md" -type f ! -name "README.md" | wc -l
# Show any markdown files that are not README
find .changeset -maxdepth 1 -name "*.md" -type f ! -name "README.md"Repository: knockout/tko
Length of output: 179
Add a changeset entry for this public packaging change.
This PR removes previously published/importable helpers and changes package entrypoint metadata across multiple packages. Create a .changeset/*.md file using npx changeset add to document the affected packages and bump type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/binding.foreach/package.json` around lines 5 - 9, The package.json
was changed to alter the published entrypoints ("module": "dist/index.js" and
"files": ["dist/","types/"]) and this is a public packaging change that requires
a Changeset; run npx changeset add and create a .changeset/*.md entry
documenting the package (packages/binding.foreach) with a description of the
breaking/public change and the appropriate version bump (patch/minor/major as
applicable) so the change will be published and tracked; ensure the changeset
file names the package and the change type, and commit the new .changeset/*.md
file alongside the package.json change.
| const hasDir = (name) => | ||
| fs.access(name).then(() => true, () => false) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current implementation swallows all fs.access errors
rg -n -C3 "const hasDir|fs\\.access\\(name\\)\\.then\\(\\(\\) => true, \\(\\) => false\\)" tools/repackage.mjsRepository: knockout/tko
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Find all usages of hasDir in the file
rg -n "hasDir" tools/repackage.mjsRepository: knockout/tko
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Get context around line 52 to see how hasHelpers is used
sed -n '48,65p' tools/repackage.mjsRepository: knockout/tko
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Get the packageData function implementation
sed -n '13,35p' tools/repackage.mjsRepository: knockout/tko
Length of output: 568
Do not treat every fs.access error as "directory missing".
This currently converts permission/IO errors into false, which silently excludes the helpers directory from package exports and files metadata instead of failing fast.
Suggested fix
-const hasDir = (name) =>
- fs.access(name).then(() => true, () => false)
+const hasDir = async (name) => {
+ try {
+ await fs.access(name)
+ return true
+ } catch (error) {
+ if (error?.code === 'ENOENT') return false
+ throw error
+ }
+}📝 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.
| const hasDir = (name) => | |
| fs.access(name).then(() => true, () => false) | |
| const hasDir = async (name) => { | |
| try { | |
| await fs.access(name) | |
| return true | |
| } catch (error) { | |
| if (error?.code === 'ENOENT') return false | |
| throw error | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/repackage.mjs` around lines 10 - 11, The helper function hasDir
currently treats any fs.access error as "not present" which hides permission/IO
errors; update hasDir to only return false for a missing path (e.g., err.code
=== 'ENOENT') and rethrow any other errors so permission/IO problems fail fast.
Locate the hasDir function and change its promise rejection handler to inspect
err.code and return false only for ENOENT (optionally ENOTDIR if desired),
otherwise throw or reject the original error instead of returning false.
The module field in 22 package.json files pointed to non-existent files (e.g. dist/bind.js). esbuild outputs dist/index.js. Bundlers using the module field would get resolution errors. Also remove helpers/ from files and exports — test helpers shouldn't ship to npm consumers. Based on phillipc's work in PR #295, applied cleanly to current main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary by CodeRabbit