Include docs and semver dependencies in published packages#24
Include docs and semver dependencies in published packages#24
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (2)
scripts/write-dist-manifest.mjs (2)
54-74: Add warning when workspace root is not found.The function correctly walks up the directory tree to find the workspace root. However, if no workspace root is found (returns
null), therootCatalogwill be an empty object, and catalog references won't be resolved. This would result in invaliddist/package.jsonfiles containing"catalog:"references that npm cannot resolve.Consider adding a warning or error when the workspace root is not found:
const workspaceInfo = await resolveWorkspaceRoot(); +if (!workspaceInfo) { + console.warn( + `⚠️ No workspace root found for ${packageRoot}. Catalog references will not be resolved.` + ); +} const rootCatalog = workspaceInfo?.manifest?.catalog ?? {};
76-93: Add warning for unresolved catalog references.The function correctly handles catalog reference resolution. However, when a catalog entry is not found (line 90 returns the original
range), it leaves"catalog:"references in the output, which will cause npm publishing to fail.Consider logging a warning when catalog entries are missing:
if (typeof range === "string" && range.startsWith("catalog:")) { const catalogKey = range === "catalog:" ? name : range.slice("catalog:".length); const resolved = catalog?.[catalogKey]; if (typeof resolved === "string" && resolved.length > 0) { return [name, resolved]; + } else { + console.warn( + `⚠️ Catalog entry "${catalogKey}" not found for dependency "${name}". Leaving as "${range}".` + ); } } return [name, range];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
package.json(1 hunks)packages/api/LICENSE(1 hunks)packages/api/package.json(2 hunks)packages/auth/LICENSE(1 hunks)packages/auth/package.json(2 hunks)packages/db/LICENSE(1 hunks)packages/db/package.json(1 hunks)packages/types/LICENSE(1 hunks)packages/types/package.json(2 hunks)scripts/write-dist-manifest.mjs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/*/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Reference shared dependencies via the "catalog:" protocol in package manifests
Files:
packages/api/package.jsonpackages/types/package.jsonpackages/db/package.jsonpackages/auth/package.json
package.json
📄 CodeRabbit inference engine (AGENTS.md)
package.json: Pin shared dependency versions in the root package.json catalog
Add new shared dependencies to the root catalog before consuming them in packages
Files:
package.json
🧠 Learnings (2)
📚 Learning: 2025-10-02T12:40:33.718Z
Learnt from: CR
PR: listee-dev/listee-libs#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T12:40:33.718Z
Learning: Applies to packages/*/package.json : Reference shared dependencies via the "catalog:" protocol in package manifests
Applied to files:
packages/api/package.json
📚 Learning: 2025-10-02T12:40:33.718Z
Learnt from: CR
PR: listee-dev/listee-libs#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T12:40:33.718Z
Learning: Applies to package.json : Add new shared dependencies to the root catalog before consuming them in packages
Applied to files:
scripts/write-dist-manifest.mjs
🔇 Additional comments (15)
packages/db/LICENSE (1)
1-21: LGTM! Standard MIT license.The MIT license text is correct and appropriate for the package.
packages/auth/LICENSE (1)
1-21: LGTM! Consistent MIT licensing.The license is identical to other packages, ensuring consistent licensing across the monorepo.
packages/types/package.json (2)
27-29: LGTM! Catalog reference aligns with guidelines.The change from
workspace:^tocatalog:follows the coding guidelines and enables semver resolution during the build process.Based on coding guidelines
13-17: No action required. README.md exists in packages/types/ and is correctly listed in the package.json files array. The package will publish successfully.packages/types/LICENSE (1)
1-21: LGTM! Consistent licensing.packages/db/package.json (1)
13-17: LGTM! Adds documentation and license to published artifacts.The changes correctly include README.md and LICENSE in the published package. Ensure README.md exists (see verification script in packages/types/package.json review).
scripts/write-dist-manifest.mjs (2)
3-3: LGTM! Import needed for workspace resolution.The
dirnameimport is required for the newresolveWorkspaceRootfunction.
113-114: LGTM! Catalog resolution properly integrated.The catalog resolution is correctly integrated into the manifest generation flow. All dependency types (dependencies, devDependencies, peerDependencies, optionalDependencies) are processed, ensuring comprehensive catalog reference substitution.
Also applies to: 139-154
packages/api/LICENSE (1)
1-21: LGTM! Consistent licensing.packages/api/package.json (2)
13-17: LGTM! Documentation and license included in published artifacts.Ensure README.md exists (see verification script in packages/types/package.json review).
27-32: LGTM! Internal dependencies now use catalog references.The change from
workspace:^tocatalog:for internal@listeepackages aligns with the coding guidelines and enables proper semver resolution during the build process.Based on coding guidelines
package.json (2)
10-13: Ensure jose version is consistent across all usages.Jose is now defined in the root catalog at ^5.2.3 and should be referenced via "catalog:" in all packages. Verify that packages/auth/package.json (and all other packages) consistently use this catalog entry rather than direct version specifications.
The catalog entries are properly pinned with semver ranges. This aligns with the PR objective to establish catalog-based dependency resolution. Ensure the build process successfully substitutes these catalog references to absolute versions in published dist manifests.
10-13: Version alignment and catalog entries verified successfully.All internal packages (auth, db, types) are correctly pinned to version ^0.2.0 in the root catalog, matching their respective package.json files. Packages that depend on internal @Listee packages properly use the "catalog:" protocol for dependency resolution.
packages/auth/package.json (2)
13-17: Both required files are present in packages/auth/.README.md and LICENSE files exist in the package directory and will be included in the npm package correctly.
28-30: Catalog reference substitution is correctly implemented.The verification confirms:
- packages/auth/package.json properly uses the "catalog:" protocol for all dependencies
- Root package.json contains a complete catalog with semver versions for @listee/db, @listee/types, and jose
- scripts/write-dist-manifest.mjs postbuild script correctly implements catalog reference replacement by resolving the workspace root, extracting the catalog, and substituting "catalog:" strings with their corresponding semver versions in the dist/package.json during build
The implementation aligns with the coding guideline requirement to reference shared dependencies via the "catalog:" protocol in package manifests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/api/package.json(2 hunks)packages/auth/package.json(1 hunks)packages/types/package.json(1 hunks)scripts/write-dist-manifest.mjs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/auth/package.json
- packages/types/package.json
- packages/api/package.json
🔇 Additional comments (5)
scripts/write-dist-manifest.mjs (5)
2-3: LGTM!The added imports (
readdir,dirname) are correctly used by the new workspace resolution functions.
54-74: LGTM!The workspace root resolution correctly climbs the directory tree and stops at the filesystem root. The logic handles errors gracefully and returns the appropriate result.
137-146: LGTM!The workspace reference handling correctly preserves
^and~prefixes while defaulting to exact versions forworkspace:*andworkspace:(empty). This covers the common workspace specifier patterns.
171-173: LGTM!The workspace resolution and dependency substitution is correctly integrated into the manifest generation flow. All dependency types (
dependencies,devDependencies,peerDependencies,optionalDependencies) are properly handled.Also applies to: 198-217
92-94: All workspace patterns in the codebase end with/*.Verification confirms that the workspace configuration uses only glob patterns, so the code at lines 92-94 correctly processes all patterns. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/write-dist-manifest.mjs (1)
231-237: Add a final manifest scan to catch any leaked catalog:/workspace: ranges.You collect unresolved during substitution (good), but a defensive final scan of distManifest guarantees no protocol ranges slip through (e.g., via nested catalog values or future edits). This aligns with the earlier review suggestion.
Apply below just before writing the file:
distManifest.optionalDependencies = replaceSpecialReferences( distManifest.optionalDependencies, rootCatalog, workspaceVersions, unresolvedReferences, ); - if (unresolvedReferences.length > 0) { + if (unresolvedReferences.length > 0) { console.error("❌ Unresolved workspace/catalog references detected:"); for (const { name, range, type } of unresolvedReferences) { console.error(` ${name}: ${range} (${type})`); } process.exit(1); } + + // Defensive validation: ensure no protocol ranges remain in the final manifest + const sections = [ + ["dependencies", distManifest.dependencies], + ["devDependencies", distManifest.devDependencies], + ["peerDependencies", distManifest.peerDependencies], + ["optionalDependencies", distManifest.optionalDependencies], + ]; + const leaked = sections + .flatMap(([section, obj]) => + Object.entries(obj || {}).map(([dep, r]) => [section, dep, r]), + ) + .filter(([, , r]) => typeof r === "string" && (r.startsWith("catalog:") || r.startsWith("workspace:"))); + if (leaked.length > 0) { + console.error("❌ Unresolved references found in dist manifest:"); + for (const [section, dep, r] of leaked) { + console.error(` ${section}.${dep}: ${r}`); + } + process.exit(1); + }
🧹 Nitpick comments (2)
scripts/write-dist-manifest.mjs (2)
76-120: Workspace pattern handling is limited; warn or support non-trivial globs.Current logic only handles patterns ending with "/" and silently skips others, missing valid configs like "packages//examples/" or a plain directory without "". Add minimal warnings and handle the “no star” directory case to avoid silent drops. Based on learnings.
Apply this diff inside resolveWorkspacePackages:
- for (const pattern of patterns) { - if (!pattern.endsWith("/*")) { - continue; - } - const base = resolve(workspaceRoot.rootDir, pattern.slice(0, -2)); + for (const pattern of patterns) { + // Skip complex nested globs but warn so it's visible in CI + const firstStar = pattern.indexOf("*"); + if (firstStar !== -1 && pattern.slice(firstStar + 1).includes("*")) { + console.warn(`⚠️ Nested workspace pattern not supported by write-dist-manifest: "${pattern}". Skipping.`); + continue; + } + + let base; + if (pattern.endsWith("/*")) { + base = resolve(workspaceRoot.rootDir, pattern.slice(0, -2)); + } else if (!pattern.includes("*")) { + // Allow a concrete directory (e.g., "packages") + base = resolve(workspaceRoot.rootDir, pattern); + } else { + console.warn(`⚠️ Unsupported workspace pattern: "${pattern}". Expected "<dir>/*" or a concrete directory. Skipping.`); + continue; + }
121-159: Broaden workspace: specifier support and guard against nested protocol values.
- Handle workspace:^1.2.3 and workspace:~1.2.3 by normalizing to prefix of the actual workspace version.
- Treat workspace:* and empty specifier explicitly.
- If a catalog value itself contains catalog:/workspace:, treat as unresolved to avoid leaking protocols into the published manifest.
- Optionally emit a warning when pushing to unresolved (helps debugging). Based on learnings.
- if (range.startsWith("catalog:")) { + if (range.startsWith("catalog:")) { const catalogKey = range === "catalog:" ? name : range.slice("catalog:".length); const resolved = catalog?.[catalogKey]; - if (typeof resolved === "string" && resolved.length > 0) { - return [name, resolved]; - } - unresolved.push({ name, range, type: "catalog" }); + if (typeof resolved === "string" && resolved.length > 0) { + // Guard against nested protocols in catalog values + if (resolved.startsWith("catalog:") || resolved.startsWith("workspace:")) { + unresolved.push({ name, range: resolved, type: "catalog-nested" }); + } else { + return [name, resolved]; + } + } else { + unresolved.push({ name, range, type: "catalog" }); + } } - if (range.startsWith("workspace:")) { + if (range.startsWith("workspace:")) { const specifier = range.slice("workspace:".length); const version = workspaceVersions.get(name); if (typeof version === "string" && version.length > 0) { - if (specifier === "^" || specifier === "~") { - return [name, `${specifier}${version}`]; - } - return [name, version]; + // Normalize common forms + if (specifier === "^" || specifier.startsWith("^")) { + return [name, `^${version}`]; + } + if (specifier === "~" || specifier.startsWith("~")) { + return [name, `~${version}`]; + } + if (specifier === "*" || specifier === "") { + return [name, version]; + } + // Fallback: use the workspace version + return [name, version]; } - unresolved.push({ name, range, type: "workspace" }); + unresolved.push({ name, range, type: "workspace" }); } } return [name, range]; }), );Optionally add warnings when recording unresolved:
- unresolved.push({ name, range, type: "catalog" }); + unresolved.push({ name, range, type: "catalog" }); + console.warn(`⚠️ Could not resolve catalog reference: ${name}@${range}`); ... - unresolved.push({ name, range, type: "workspace" }); + unresolved.push({ name, range, type: "workspace" }); + console.warn(`⚠️ Could not resolve workspace reference: ${name}@${range}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/write-dist-manifest.mjs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T12:40:33.718Z
Learnt from: CR
PR: listee-dev/listee-libs#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T12:40:33.718Z
Learning: Applies to packages/*/package.json : Reference shared dependencies via the "catalog:" protocol in package manifests
Applied to files:
scripts/write-dist-manifest.mjs
🔇 Additional comments (2)
scripts/write-dist-manifest.mjs (2)
206-230: Good coverage: all dep sections are normalized.dependencies, devDependencies, peerDependencies, and optionalDependencies are all processed consistently before write. LGTM.
239-246: Verify publishing strategy before applying docs-copy refactor.The concern raised is valid but conditional. Current setup publishes from the package root (standard npm behavior), which correctly includes docs via the
"files"field. The suggested refactor to copy docs intodist/would only be necessary if:
- Publishing is intended to run from
dist/(configured viapublishConfig.directory), or- The
dist/folder is meant to be standalone-publishableThe current
publishConfiglacks a"directory"field, so npm publishes from the root per default. The deletion of the"files"field fromdist/package.jsonis appropriate since the normalized manifest indist/is not itself published directly—docs are included at the root level.However, if the PR's intent is to enable future
dist/-based publishing, the refactor is appropriate and aligns with the stated objective to "include docs in published packages."Check the PR description or release strategy to confirm whether docs should be copied into
dist/as part of this change.
1 participant
Summary
Testing
Summary by CodeRabbit
Chores
Refactor