Skip to content

Conversation

zmzlois
Copy link
Contributor

@zmzlois zmzlois commented Jul 3, 2025

@artem-sht reported pnpm --filter=./apps/* doesn't work

Copy link

cla-assistant bot commented Jul 3, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

claude bot commented Jul 3, 2025

Claude finished @zmzlois's task —— View job


PR Review Complete

Todo List:

  • Get PR diff information
  • Analyze the pnpm workspace configuration changes
  • Review for correctness, security, and best practices
  • Provide structured review feedback with severity table
  • Complete comprehensive review

Purpose: This PR fixes a pnpm workspace filtering issue by adding proper workspace configuration and updating the dev script to use recursive execution.

📋 Review Findings
Severity File/Line(s) Issue Recommendation
High package.json:9,12 Inconsistent command usage - other scripts still use --filter=./apps/* which reportedly doesn't work Update all scripts to use consistent approach: either -r with workspace filtering or fix the filter syntax
Medium package.json:7 Behavior change - pnpm -r runs on ALL workspace packages, not just apps/* Consider using pnpm -r --filter='./apps/*' to maintain original intent
Low pnpm-workspace.yaml:2 Missing workspace configuration options Consider adding workspaces field to package.json for npm compatibility
🔍 Detailed Analysis

Correctness & Hidden Bugs:

  • pnpm-workspace.yaml is correctly formatted
  • ⚠️ Inconsistency: Only the dev script was updated, but build-remotes, start, and start-remotes still use the problematic --filter=./apps/* syntax
  • ⚠️ Behavior change: pnpm -r runs commands in ALL workspace packages, not just those matching ./apps/*

Code Quality:

  • ✅ Configuration follows pnpm best practices
  • ✅ Clean, minimal workspace configuration
  • ⚠️ Inconsistent command patterns across scripts

Suggested fixes:

{
  "scripts": {
    "dev": "pnpm -r --filter='./apps/*' --parallel run dev",
    "build-remotes": "pnpm -r --filter='./apps/*' --filter='!airbnb-react-host' --parallel run build",
    "start": "pnpm -r --filter='./apps/*' --parallel run start",  
    "start-remotes": "pnpm -r --filter='./apps/*' --filter='!airbnb-react-host' --parallel run start"
  }
}

Or if the current filter syntax is the issue, consider using workspace names:

{
  "scripts": {
    "dev": "pnpm -r --parallel run dev",
    "build-remotes": "pnpm run --parallel --filter='!airbnb-react-host' build",
    "start": "pnpm -r --parallel run start",
    "start-remotes": "pnpm run --parallel --filter='!airbnb-react-host' start"
  }
}

Overall Quality Score: 6/10

Top 3 Next Steps:

  1. Fix script inconsistency - Update all scripts to use the same approach that works
  2. Test the change - Verify that pnpm -r doesn't unintentionally run commands in the root package
  3. Consider workspace naming - If using -r without filters, ensure all packages have consistent script names

The core fix (adding pnpm-workspace.yaml) is correct, but the inconsistent script updates create potential issues. The change should work for the dev script but other scripts may still fail.

Copy link

cla-assistant bot commented Jul 3, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@zmzlois
Copy link
Contributor Author

zmzlois commented Jul 3, 2025

@claude please review

@zmzlois zmzlois closed this Jul 3, 2025
@zmzlois zmzlois reopened this Jul 3, 2025
@zmzlois zmzlois merged commit 3ccb0e5 into main Jul 4, 2025
9 checks passed
@zmzlois zmzlois deleted the lois/add-workspace-file-airbnb branch July 4, 2025 11:12
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.

2 participants