chore: replace Nx workspace scripts with pnpm recursive commands#1220
chore: replace Nx workspace scripts with pnpm recursive commands#1220danieliser wants to merge 4 commits into
Conversation
Remove unused Nx tooling and run package scripts via pnpm -r instead.
WalkthroughNx configuration is removed and root scripts/devDependencies are migrated from Nx to pnpm recursive commands; workspace packages replace ChangesNx to pnpm Toolchain Migration
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Replace per-package prepublishOnly hooks with a root postinstall that runs pnpm recursive TypeScript builds topologically. prepublishOnly ran in parallel during install and caused components to compile before utils.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 702386bf17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| "scripts": { | ||
| "preinstall": "npx -y only-allow pnpm", | ||
| "postinstall": "pnpm run build:tsc && pnpm run build:types", |
There was a problem hiding this comment.
Keep postinstall from requiring devDependencies
This new lifecycle hook makes production installs fail: postinstall now runs the TypeScript builds, but typescript/tsc is only in devDependencies, and the pnpm 10 install docs state that --prod skips devDependencies while scripts still run unless --ignore-scripts is set. In any environment that does pnpm install --prod or otherwise omits dev deps, install will complete dependency resolution and then fail at this hook before the package can be used.
Useful? React with 👍 / 👎.
| }, | ||
| "scripts": { | ||
| "prepublishOnly": "pnpm run build:tsc && pnpm run build:types", | ||
| "build:tsc": "tsc", |
There was a problem hiding this comment.
Restore the per-package publish build hook
Removing prepublishOnly means a direct pnpm publish/pnpm -r publish from a clean checkout no longer builds this package before packing it; pnpm's publish lifecycle still includes prepublishOnly, and this package's main, types, and files all expect ignored build/build-types artifacts to exist. The custom bin/bump-and-publish-packages.js path builds first, but any other publish path can now ship a tarball with missing entrypoints.
Useful? React with 👍 / 👎.
Skip postinstall package builds when TypeScript is unavailable (e.g. pnpm install --prod). Restore publish-time builds via prepack instead of prepublishOnly to avoid install-time race conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 103: The postinstall script calls execSync('pnpm run build:tsc && pnpm
run build:types') in bin/postinstall-packages.js without protecting failures, so
a failing build aborts pnpm install; wrap that execSync call in a try/catch
around the code path that runs when require.resolve('typescript') succeeds, and
in the catch log the error (use console.error or the existing logger) and exit
with code 0 (or otherwise return/continue) so postinstall does not fail the
install when build:tsc or build:types errors; keep the existing behavior for the
require.resolve('typescript') absent case.
🪄 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: 100abd3e-e42a-4c3d-b9d9-ebb40dc50509
📒 Files selected for processing (14)
bin/postinstall-packages.jspackage.jsonpackages/components/package.jsonpackages/core-data/package.jsonpackages/cta-admin/package.jsonpackages/cta-editor/package.jsonpackages/data/package.jsonpackages/fields/package.jsonpackages/i18n/package.jsonpackages/layout/package.jsonpackages/registry/package.jsonpackages/types/package.jsonpackages/use-query-params/package.jsonpackages/utils/package.json
✅ Files skipped from review due to trivial changes (2)
- bin/postinstall-packages.js
- packages/data/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/i18n/package.json
- packages/use-query-params/package.json
- packages/layout/package.json
- packages/types/package.json
- packages/core-data/package.json
- packages/utils/package.json
| }, | ||
| "scripts": { | ||
| "preinstall": "npx -y only-allow pnpm", | ||
| "postinstall": "node bin/postinstall-packages.js", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the postinstall helper exists and inspect its TS-availability guard
fd -t f 'postinstall-packages.js' bin
echo '--- contents ---'
fd -t f 'postinstall-packages.js' bin --exec cat {}Repository: PopupMaker/Popup-Maker
Length of output: 649
Fix postinstall failure handling when TypeScript is present but the build fails
bin/postinstall-packages.js correctly skips the build when typescript is missing (require.resolve('typescript') + process.exit(0)), but when TypeScript exists it runs execSync('pnpm run build:tsc && pnpm run build:types') with no try/catch—so a failing build:tsc/build:types will still fail the postinstall step and can break pnpm install.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 103, The postinstall script calls execSync('pnpm run
build:tsc && pnpm run build:types') in bin/postinstall-packages.js without
protecting failures, so a failing build aborts pnpm install; wrap that execSync
call in a try/catch around the code path that runs when
require.resolve('typescript') succeeds, and in the catch log the error (use
console.error or the existing logger) and exit with code 0 (or otherwise
return/continue) so postinstall does not fail the install when build:tsc or
build:types errors; keep the existing behavior for the
require.resolve('typescript') absent case.
Summary
@nx/*dev dependencies from the core plugin workspace.nx run-manypackage scripts with nativepnpm -rcommands (--sortfor builds,--parallelfor lint/format).nx.jsonand refresh the lockfile.Test plan
pnpm installpnpm run build:tscpnpm run packages:lintpnpm run packages:formatSummary by CodeRabbit