Moved @tryghost framework pins to pnpm catalog#27533
Conversation
- @tryghost/debug (7 workspaces) -> catalog: - @tryghost/errors (ghost/core + root pnpm.overrides) -> catalog: - @tryghost/logging (ghost/core, e2e, root pnpm.overrides) -> catalog: - one source of truth for three @tryghost/* packages that were pinned identically across the monorepo; future framework bumps become a single-line catalog edit rather than seven (debug) or three (logging) coordinated package.json changes - extends the pattern already in use for eslint in pnpm-workspace.yaml - set up before wave 4 (@tryghost/errors 3.1.0 + @tryghost/logging 4.1.0) so that wave lands cleanly - root pnpm.overrides can reference catalog entries too (pnpm 10.x), so the override for errors tightens from ^1.3.7 to the catalog's 1.3.13 — no behaviour change since every consumer was already resolving to 1.3.13 - patch-bumped the four published apps whose package.json was touched: portal 2.68.16, comments-ui 1.4.9, signup-form 0.3.17, sodo-search 1.8.14
WalkthroughAdded a pnpm dependency catalog with explicit versions for Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
The comment implied catalog entries and renovate's grouping rule had to stay in sync; they don't. Renovate groups by package name and picks up any @tryghost/* dep regardless of where it's declared, and the concern (does this package belong in the renovate group?) applies to every @tryghost/* pin anywhere in the repo, not just catalog ones.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/sodo-search/package.json`:
- Line 17: The package.json contains a pnpm-only dependency specifier
"@tryghost/debug": "catalog:" which will break npm publish; update the
publishing flow so the ship script (the "ship" npm script) performs pnpm publish
(or pnpm pack) after committing the version bump or rewrites package.json to
replace "catalog:" specifiers before calling npm publish. Concretely, modify the
ship pipeline to run "pnpm publish" for the package (or run a manifest rewrite
step to convert "catalog:" deps to real versions) after the commit, ensuring
"@tryghost/debug" is not left as "catalog:" when published.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74a18ac0-5390-44f1-94c9-2a47eaf9f61f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/comments-ui/package.jsonapps/portal/package.jsonapps/signup-form/package.jsonapps/sodo-search/package.jsone2e/package.jsonghost/core/package.jsonghost/i18n/package.jsonpackage.jsonpnpm-workspace.yaml
| }, | ||
| "dependencies": { | ||
| "@tryghost/debug": "2.1.0", | ||
| "@tryghost/debug": "catalog:", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package manager declared at the repo root:"
if [ -f package.json ]; then
jq -r '.packageManager // "(no packageManager field)"' package.json
fi
echo
echo "Catalog entry for `@tryghost/debug`:"
rg -n -C2 '@tryghost/debug|catalog:' pnpm-workspace.yaml apps/sodo-search/package.json
echo
echo "Release script publish/pack calls:"
if [ -f .github/scripts/release-apps.js ]; then
rg -n -C4 '\b(pnpm|npm|yarn)\b|pack|publish|spawn|exec|execa' .github/scripts/release-apps.js
else
echo ".github/scripts/release-apps.js not found"
fiRepository: TryGhost/Ghost
Length of output: 3094
🏁 Script executed:
wc -l .github/scripts/release-apps.js
cat -n .github/scripts/release-apps.js | tail -20Repository: TryGhost/Ghost
Length of output: 894
🏁 Script executed:
fd -e yml -e yaml | xargs rg -l "publish|sodo-search" .github/workflows/Repository: TryGhost/Ghost
Length of output: 169
🏁 Script executed:
cat -n .github/workflows/release-apps.yml 2>/dev/null | head -60Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
rg -r "release-apps" .github/Repository: TryGhost/Ghost
Length of output: 1586
🏁 Script executed:
find . -name "*.md" -type f | xargs rg -l "release|publish|sodo-search" | head -5Repository: TryGhost/Ghost
Length of output: 268
🏁 Script executed:
cat -n apps/sodo-search/package.jsonRepository: TryGhost/Ghost
Length of output: 3471
🏁 Script executed:
rg -B5 -A5 "publishConfig|npm publish|pnpm publish" apps/sodo-search/package.jsonRepository: TryGhost/Ghost
Length of output: 269
Verify that publishing uses pnpm publish or rewrites catalog: dependencies.
The @tryghost/debug dependency at line 17 uses catalog:, which is valid in pnpm workspaces. However, catalog: is not understood by npm and must be removed before publishing. The custom ship script (line 34) commits the version change but does not call pnpm publish, pnpm pack, or rewrite the manifest. Confirm the actual publishing step (after the commit) uses pnpm publish (which automatically removes catalog: specifiers) or performs an equivalent manifest rewrite before npm publication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/sodo-search/package.json` at line 17, The package.json contains a
pnpm-only dependency specifier "@tryghost/debug": "catalog:" which will break
npm publish; update the publishing flow so the ship script (the "ship" npm
script) performs pnpm publish (or pnpm pack) after committing the version bump
or rewrites package.json to replace "catalog:" specifiers before calling npm
publish. Concretely, modify the ship pipeline to run "pnpm publish" for the
package (or run a manifest rewrite step to convert "catalog:" deps to real
versions) after the commit, ensuring "@tryghost/debug" is not left as "catalog:"
when published.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27533 +/- ##
=======================================
Coverage 73.00% 73.00%
=======================================
Files 1556 1556
Lines 125898 125898
Branches 15244 15244
=======================================
+ Hits 91909 91911 +2
+ Misses 33029 33028 -1
+ Partials 960 959 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- ghost-cli installs Ghost outside the pnpm workspace, where
catalog: specifiers can't resolve - pnpm fails with
ERR_PNPM_SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER
- two places were leaking catalog: refs into the tarball:
1) component tarballs packed with `npm pack` kept the workspace
package.json verbatim; switched to `pnpm pack` which
substitutes workspace: and catalog: with concrete versions
2) the root pnpm.overrides merged into the packaged package.json
still contained catalog: entries; resolve them via
pnpm-workspace.yaml before merging
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/scripts/pack.js (2)
102-117: Parsepnpm pack's stdout to obtain the actual tarball filename instead of reconstructing it.The script hand-constructs
tgzNameaskey.replaceAll('@','').replaceAll('/','-') + '-' + version + '.tgz'and uses it to reference the packed component in the deploypackage.json. Whilepnpm packdoes produce this filename (matching npm pack's behavior for scoped packages), relying on reconstructed filenames is fragile if pnpm's behavior changes. Sincepnpm packprints the tarball path to stdout, capture and use it as the source of truth instead. Alternatively, add an assertionfs.existsSync(path.join(componentsDir, tgzName))after packing to catch filename mismatches early rather than failing silently at install time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/scripts/pack.js` around lines 102 - 117, The pack script currently hand-constructs tgzName from key and depPkg.version and trusts that pnpm produced that filename; change the execFileSync call for 'pnpm pack' in the pack.js block (where depPkg, slug and tgzName are defined) to capture pnpm's stdout (or use execFileSync with encoding to get stdout) and parse the printed tarball path to derive the actual filename to assign into pkg.dependencies[key] (instead of using the reconstructed tgzName); as a safety fallback, after packing assert the file exists with fs.existsSync(path.join(componentsDir, actualTgzName)) and throw/log a clear error if not found so mismatches are detected early.
37-49: Catalog resolver doesn't support named catalogs or selector-based overrides.The function reads only
workspaceYaml.catalog(the default catalog) and ignoresworkspaceYaml.catalogs(named catalogs). In pnpm's catalog protocol:
catalog:/catalog:defaultreferences the default catalog (under top-levelcatalog:key)catalog:<name>references a named catalog (under top-levelcatalogs:key, keyed by catalog name)The condition
spec === \catalog:${name}`is semantically incorrect:nameis a package name (e.g.,@tryghost/errors) or selector (e.g.,react>some-dep), not a catalog name. This branch can never match for valid pnpm specs. If any override uses a>selector with a catalog reference (e.g.,"react>some-dep": "catalog:"`), the lookup will throw.Not a blocker for current overrides (all simple names, default catalog only), but tighten to prevent breaking on future named-catalog or selector-based usage.
♻️ Suggested refactoring
const workspaceYaml = yaml.load(fs.readFileSync(path.join(ROOT_DIR, 'pnpm-workspace.yaml'), 'utf8')); -const catalog = workspaceYaml.catalog || {}; +const defaultCatalog = workspaceYaml.catalog || {}; +const namedCatalogs = workspaceYaml.catalogs || {}; function resolveCatalogSpec(name, spec) { - if (spec === 'catalog:' || spec === `catalog:${name}` || spec === 'catalog:default') { - const resolved = catalog[name]; - if (!resolved) { - throw new Error(`Catalog reference for ${name} not found in pnpm-workspace.yaml`); - } - return resolved; - } - return spec; + if (typeof spec !== 'string' || !spec.startsWith('catalog:')) { + return spec; + } + const catalogName = spec.slice('catalog:'.length); + const source = (!catalogName || catalogName === 'default') + ? defaultCatalog + : (namedCatalogs[catalogName] || {}); + const resolved = source[name]; + if (!resolved) { + throw new Error(`Catalog reference ${spec} for ${name} not found in pnpm-workspace.yaml`); + } + return resolved; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/scripts/pack.js` around lines 37 - 49, The resolver wrongly compares spec to `catalog:${name}` and only reads `workspaceYaml.catalog`; change resolveCatalogSpec to detect any spec that starts with "catalog:" (use prefix check), extract the catalog key after the colon (empty or "default" -> use workspaceYaml.catalog, otherwise treat it as a named catalog and look it up under workspaceYaml.catalogs[catalogKey]), and throw a clear error if the named catalog is missing; update references to `workspaceYaml.catalogs` and keep the existing behavior for plain specs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/scripts/pack.js`:
- Around line 102-117: The pack script currently hand-constructs tgzName from
key and depPkg.version and trusts that pnpm produced that filename; change the
execFileSync call for 'pnpm pack' in the pack.js block (where depPkg, slug and
tgzName are defined) to capture pnpm's stdout (or use execFileSync with encoding
to get stdout) and parse the printed tarball path to derive the actual filename
to assign into pkg.dependencies[key] (instead of using the reconstructed
tgzName); as a safety fallback, after packing assert the file exists with
fs.existsSync(path.join(componentsDir, actualTgzName)) and throw/log a clear
error if not found so mismatches are detected early.
- Around line 37-49: The resolver wrongly compares spec to `catalog:${name}` and
only reads `workspaceYaml.catalog`; change resolveCatalogSpec to detect any spec
that starts with "catalog:" (use prefix check), extract the catalog key after
the colon (empty or "default" -> use workspaceYaml.catalog, otherwise treat it
as a named catalog and look it up under workspaceYaml.catalogs[catalogKey]), and
throw a clear error if the named catalog is missing; update references to
`workspaceYaml.catalogs` and keep the existing behavior for plain specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25f1210a-e90f-448d-8f2e-d2adaa610844
📒 Files selected for processing (1)
ghost/core/scripts/pack.js
Reverts #27533. The catalog resolutions do not work because `catalog:` does not work outside of a workspace, so `npm i` is broken for these packages. This largely should not matter, because anyone working on these should be doing so by cloning the TryGhost/Ghost repo, which is a workspace, and the built assets are also unaffected. I will follow up with more holistic changes to adopt the pnpm catalog after dependencies are brought more up to date.
- wave 4 of the @tryghost/framework catch-up, scoped to logging only - @tryghost/logging source diff vs old version is entirely formatting; transitive deps (bunyan-loggly, fs-extra, lodash) get patch bumps and framework-sibling packages logging depends on (elasticsearch, http-stream, pretty-stream, request, validator) move to their current v2/v3/v5 majors internally - none of that surface crosses into ghost/core's app code - @tryghost/errors intentionally not bumped. the override in pnpm.overrides forces the tree to errors@1 because errors@3 is node-only (imports Node's crypto module) and @tryghost/limit-service drags it into admin-x-settings' browser bundle via a runtime import in use-limiter.tsx, breaking the vite build. dropping the override fragments the tree into five errors versions and breaks instanceof checks in ghost/core unit tests. proper fix is in @tryghost/limit- service or admin-x-settings, not in @tryghost/errors - FRAMEWORK-DEPS-FOLLOWUPS.md captures this plus the catalog issue from the earlier reverted attempt (#27533 / #27535) so a future dedicated effort has the full picture
1 participant
Summary
Moves three
@tryghost/*packages topnpm-workspace.yaml'scatalog:entry so the version is declared in one place instead of being mirrored across up to seven workspaces andpnpm.overrides.@tryghost/debugghost/core,ghost/i18n,e2e, + four public apps)catalog:@tryghost/errorsghost/corepin + rootpnpm.overridescatalog:@tryghost/loggingghost/core,e2e, + rootpnpm.overridescatalog:Extends the pattern already used for
eslintinpnpm-workspace.yaml. The catalog comment points to the matching.github/renovate.json5group so the two stay in sync.Why now
Framework-repo catch-up is still in flight. Wave 4 is
@tryghost/errors→ 3.1.0 and@tryghost/logging→ 4.1.0 — with this migration in place first, Wave 4 becomes three version edits (one per catalog entry) instead of eight coordinated package.json changes. It also prevents silent drift (e.g.ghost/coreat 2.1.0 while an app lingers at an older pin).Mechanical notes
debug2.1.0,errors1.3.13,logging2.5.5).pnpm.overridesentry for@tryghost/errorstightens from the range^1.3.7to the catalog's exact1.3.13. Since every consumer was already at 1.3.13, this is a behaviour-neutral tightening.package.jsontouched (via thedebugpin swap), so patch-bumped each: portal 2.68.16, comments-ui 1.4.9, signup-form 0.3.17, sodo-search 1.8.14.matchPackageNamesin.github/renovate.json5) targets package names, so it keeps working regardless of where the dep is declared. Worth watching on the next renovate run for this grouping to confirm PR shape is unchanged.Test plan
pnpm install— clean, 21-line lockfile diff (all specifier updates, zero package adds/removes)cd ghost/core && pnpm test:unit— 6141 passingcd ghost/core && pnpm test:integration— 244 passingcd ghost/core && pnpm test:e2e— 1573 passingcd ghost/core && pnpm test:legacy— 451 passingcd ghost/core && pnpm lint— cleanpnpm --filter @tryghost/i18n test— 30 passingpnpm --filter @tryghost/comments-ui test:unit— 202 passingpnpm --filter @tryghost/portal test— 507 passingpnpm --filter @tryghost/sodo-search test— 2 files passingpnpm --filter @tryghost/signup-form run build— clean