Skip to content

feat(pm): Package manager agnostic install(), add(), dedupe()#1457

Merged
Tobbe merged 5 commits intomainfrom
tobbe-pm-agnostic-install-add-dedupe
Mar 25, 2026
Merged

feat(pm): Package manager agnostic install(), add(), dedupe()#1457
Tobbe merged 5 commits intomainfrom
tobbe-pm-agnostic-install-add-dedupe

Conversation

@Tobbe
Copy link
Copy Markdown
Member

@Tobbe Tobbe commented Mar 25, 2026

Add helper methods for getting the package manager specific install, add and dedupe commands

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 25, 2026

Deploy Preview for cedarjs canceled.

Name Link
🔨 Latest commit 59d4458
🔍 Latest deploy log https://app.netlify.com/projects/cedarjs/deploys/69c3c50ba33dd10008095dc6

@github-actions github-actions Bot added this to the next-release milestone Mar 25, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 25, 2026

Greptile Summary

This PR makes the Cedar CLI package-manager-agnostic by introducing a set of helper functions (add(), install(), dedupe(), dedupeIsSupported(), installationErrorMessage(), prettyPrintCedarCommand()) in @cedarjs/cli-helpers/packageManager and updating getPackageManager() in @cedarjs/project-config to detect the active package manager from the npm_config_user_agent environment variable before falling back to lock-file detection. Callers throughout the CLI (installHelpers, setupRscHandler, baremetal, and the upgrade command) are migrated away from hardcoded yarn invocations.

Key changes:

  • New PM detection priority: env var (npm_config_user_agent) > lock files > default yarn — the env var tells the CLI which package manager actually invoked the current command.
  • upgrade command: --dedupe option is now hidden for non-yarn users; the dedupeDeps task is correctly skipped for npm/pnpm via the dedupeIsSupported() guard.
  • dedupe() helper returns undefined for non-yarn PMs: it is used in template literals inside dedupeDeps without an explicit null guard — safe today due to control-flow, but fragile (see inline comment).
  • No tests for new cli-helpers/packageManager functions: add(), install(), deduce(), etc. are untested (see inline comment).

Confidence Score: 4/5

  • PR is safe to merge; all previously flagged issues are resolved and the remaining comments are non-blocking style suggestions.
  • The core logic is sound: the dedupeDeps function is correctly gated so it only runs for yarn, prior critical issues (hardcoded yarn dedupe, afterEach env var coercion) have been addressed, and the env-var-first detection strategy is coherent. The remaining P2 items — no null guard on dedupeCmd() in template literals and missing unit tests for the new helpers — are improvements rather than blockers.
  • packages/cli/src/commands/upgrade/upgradeHandler.ts (dedupeCmd null safety), packages/cli-helpers/src/packageManager/tests/index.test.ts (missing tests for new helpers)

Important Files Changed

Filename Overview
packages/project-config/src/packageManager.ts Adds env var (npm_config_user_agent) as the highest-priority signal for package manager detection, falling back to lock files. The default is now set eagerly at the top, and an isPackageManager type guard is introduced. The old multi-return structure is collapsed into a clean if/else chain.
packages/cli-helpers/src/packageManager/index.ts Adds add(), install(), dedupe(), dedupeIsSupported(), installationErrorMessage(), and prettyPrintCedarCommand() helpers. deduce() returns `string
packages/cli/src/commands/upgrade/upgradeHandler.ts Migrates from hardcoded yarn to PM-agnostic helpers throughout. The dedupeDeps function is correctly gated (only runs on yarn) so dedupeCmd() will never return undefined in practice, but the template literal use of a nullable return is fragile. Error messages and task titles are now PM-aware.
packages/cli-helpers/src/lib/installHelpers.ts Correctly replaces hardcoded yarn with getPackageManager() and add(). Minor inconsistency: installPackages hardcodes 'install' instead of calling install(), but this is not a functional issue.
packages/project-config/src/tests/packageManager.test.ts Adds afterEach cleanup and a new test for the env var detection path. The env var restoration correctly handles the undefined case (addressed from a prior review thread).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getPackageManager called] --> B{Cache populated?}
    B -- Yes --> C[Return cached value]
    B -- No --> D[Set default: yarn]
    D --> E{npm_config_user_agent\nenv var set?}
    E -- Yes, valid PM --> F[Use env var PM\ne.g. yarn / npm / pnpm]
    E -- No / unrecognised --> G{yarn.lock exists?}
    G -- Yes --> H[Use yarn]
    G -- No --> I{pnpm-lock.yaml exists?}
    I -- Yes --> J[Use pnpm]
    I -- No --> K{package-lock.json exists?}
    K -- Yes --> L[Use npm]
    K -- No --> M[Fallback: yarn]
    F --> N[Cache & return]
    H --> N
    J --> N
    L --> N
    M --> N
Loading

Reviews (3): Last reviewed commit: "fix CI and review comments" | Re-trigger Greptile

Comment thread packages/cli-helpers/src/packageManager/index.ts
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Mar 25, 2026

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 59d4458

Command Status Duration Result
nx run-many -t build:pack --exclude create-ceda... ✅ Succeeded 9s View ↗
nx run-many -t build ✅ Succeeded 8s View ↗
nx run-many -t test --minWorkers=1 --maxWorkers=4 ✅ Succeeded 1m 25s View ↗
nx run-many -t test:types ✅ Succeeded 10s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-25 11:38:52 UTC

@Tobbe
Copy link
Copy Markdown
Member Author

Tobbe commented Mar 25, 2026

@greptileai please do a full review again

Comment thread packages/cli/src/commands/upgrade/upgradeHandler.ts
Comment thread packages/project-config/src/__tests__/packageManager.test.ts
@Tobbe
Copy link
Copy Markdown
Member Author

Tobbe commented Mar 25, 2026

@greptileai please review once more

Comment thread packages/cli/src/commands/upgrade/upgradeHandler.ts Outdated
@Tobbe Tobbe merged commit 57f7aa3 into main Mar 25, 2026
42 checks passed
@Tobbe Tobbe deleted the tobbe-pm-agnostic-install-add-dedupe branch March 25, 2026 12:08
@github-actions
Copy link
Copy Markdown

The changes in this PR are now available on npm.

Try them out by running yarn cedar upgrade -t 4.0.0-canary.13674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant