Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4bf91db
Migrate from Yarn Classic to pnpm (#2120)
justin808 Nov 25, 2025
2a74aae
Fix pnpm/action-setup version conflict
justin808 Nov 25, 2025
3b98806
Fix CI failures after pnpm migration
justin808 Nov 25, 2025
3ae666b
Fix knip config, ESLint errors, and lefthook scripts
justin808 Nov 25, 2025
0f74293
Add CHANGELOG entry for pnpm migration
justin808 Nov 25, 2025
70cfbf6
Fix knip configuration for pnpm migration
justin808 Nov 25, 2025
149011f
Revert LazyApolloGraphQL.tsx to master version
justin808 Nov 25, 2025
b23dc0d
Reset knip.ts to master version
justin808 Nov 25, 2025
0b91b0a
Update knip.ts for pnpm compatibility
justin808 Nov 25, 2025
6a3d6d3
Fix remaining knip issues for pnpm migration
justin808 Nov 25, 2025
60e75c1
Fix knip config: add worker.ts as entry point
justin808 Nov 25, 2025
05a1852
Add @babel/runtime and mini-css-extract-plugin back to ignoreDependen…
justin808 Nov 25, 2025
1464424
Fix lint workflow to use nps eslint command
justin808 Nov 25, 2025
45f92eb
Fix package.json scripts for pnpm compatibility
justin808 Nov 25, 2025
1768e2e
Add .npmrc to prettierignore
justin808 Nov 25, 2025
10cafc1
Fix markdown formatting for prettier
justin808 Nov 25, 2025
0a26774
Fix lint:scss path for monorepo structure
justin808 Nov 25, 2025
1fa0de9
Fix ESLint no-base-to-string error in LazyApolloGraphQL
justin808 Nov 25, 2025
0b160a5
Fix ESLint no-base-to-string with JSON.stringify
justin808 Nov 25, 2025
33c5127
Update dummy app Babel to ^7.22.0 for pnpm compatibility
justin808 Nov 25, 2025
722b532
Fix Prettier formatting in LazyApolloGraphQL
justin808 Nov 25, 2025
8001b3d
Fix Pro lint workflow Check TypeScript working directory
justin808 Nov 25, 2025
283298b
Fix check-typescript to use pnpm instead of yarn workspace
justin808 Nov 25, 2025
a0f62cd
Fix release.rake to use pnpm instead of yarn
justin808 Nov 25, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions .claude/docs/pr-splitting-strategy.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,16 @@
### Indicators That Splitting Makes Sense

1. **Multiple Independent Test Failures**

- Different test suites failing for different reasons
- Failures span multiple subsystems (integration, unit, Pro package, etc.)
- Each failure requires significant debugging time

2. **Long Git History**

- 50+ commits in the branch
- Multiple feature changes mixed together
- Hard to bisect or identify which commit broke what

3. **Mixed Concerns**

- Infrastructure changes + feature changes
- Multiple unrelated fixes bundled together
- Refactoring mixed with new functionality
Expand Down Expand Up @@ -86,19 +83,16 @@ Group 4 (Monorepo Node Renderer):
**Principle**: Merge least risky changes first

1. **Documentation-only changes** (safest)

- No code changes
- No risk of breaking tests
- Provides value immediately

2. **Bug fixes with tests** (safe if tests pass)

- Clear, focused changes
- Well-tested
- Doesn't change infrastructure

3. **Refactoring with no behavior change** (moderate risk)

- Keep tests passing
- No API changes
- Can be verified by running existing tests
Expand Down Expand Up @@ -392,19 +386,16 @@ This PR has been split into smaller, more focused PRs for easier review and debu
### New PRs (in merge order):

1. **#XXXX: Documentation & Testing Requirements** ✅ READY

- CI failure analysis
- Testing requirement documentation
- Zero risk (docs only)

2. **#YYYY: Build Console Replay Parameter Fix** 🔄 IN REVIEW

- Focused bug fix
- All tests passing
- Addresses one of the three failures

3. **#ZZZZ: Workspace Dependencies Fix** ⏳ DRAFT

- Yarn Classic compatibility
- Small infrastructure fix
- Easy to verify
Expand Down
66 changes: 34 additions & 32 deletions .claude/docs/testing-build-scripts.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

## Why This Matters

- The `prepack`/`prepare` scripts in package.json/package-scripts.yml run during:
- `npm install` / `yarn install` (for git dependencies)
- The `prepack`/`prepare` scripts in package.json run during:
- `npm install` / `pnpm install` (for git dependencies)
- `yalc publish` (critical for local development)
- `npm publish`
- Package manager prepare phase
Expand Down Expand Up @@ -40,7 +40,7 @@ gh pr view --json statusCheckRollup | jq '.statusCheckRollup[] | select(.conclus

## Mandatory Testing After ANY Changes

**If you modify package.json, package-scripts.yml, or build configs:**
**If you modify package.json or build configs:**

### Step 1: ALWAYS Test Clean Install First

Expand All @@ -51,7 +51,7 @@ This is the **MOST CRITICAL** test - it's what CI does first, and installation f
rm -rf node_modules

# Test the exact command CI uses
yarn install --frozen-lockfile
pnpm install --frozen-lockfile

# If this fails, STOP and fix it before testing anything else
```
Expand All @@ -62,7 +62,7 @@ yarn install --frozen-lockfile

```bash
# Build all packages
yarn run build
pnpm run build

# Should succeed without errors
```
Expand All @@ -71,10 +71,10 @@ yarn run build

```bash
# Test prepack/prepare scripts work
yarn nps build.prepack
pnpm run prepack

# Test yalc publish (CRITICAL for local development)
yarn run yalc:publish
pnpm run yalc:publish

# Should publish all workspace packages successfully
```
Expand All @@ -97,48 +97,48 @@ ls -la packages/react-on-rails-pro-node-renderer/lib/ReactOnRailsProNodeRenderer
bundle exec rubocop

# JS/TS formatting
yarn start format.listDifferent
pnpm run format.listDifferent
```

## When Directory Structure Changes

If you rename/move directories that contain build artifacts:

1. **Update ALL path references in package-scripts.yml**
1. **Update ALL path references in package.json**
2. **Test yalc publish BEFORE pushing**
3. **Test in a fresh clone to ensure no local assumptions**
4. **Consider adding a CI job to validate artifact paths**

## Workspace Dependencies: Yarn Classic vs Yarn Berry
## Workspace Dependencies: PNPM

**CRITICAL: This project uses Yarn Classic (v1.x), not Yarn Berry (v2+)**
**CRITICAL: This project uses PNPM (v9+)**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix markdown formatting: convert emphasis to proper heading.

Line 114 uses emphasis (**Workspace Dependencies: PNPM**) where a proper Markdown heading (##) is more semantically correct and improves document navigation.

Apply this diff:

-## Workspace Dependencies: PNPM
+## Workspace Dependencies: PNPM

Per markdownlint (MD036), prefer heading syntax over emphasis formatting for section headers.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

114-114: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🤖 Prompt for AI Agents
In .claude/docs/testing-build-scripts.md around line 114, the section header is
formatted with emphasis (**Workspace Dependencies: PNPM**) instead of a Markdown
heading; replace the emphasized text with an appropriate heading (e.g., prepend
"## " or other heading level) so the line becomes a proper Markdown header and
adheres to markdownlint rule MD036.


Check `package.json` for: `"packageManager": "[email protected]"`
Check `package.json` for: `"packageManager": "[email protected]"`

### Correct Workspace Dependency Syntax

For Yarn Classic workspaces:
For PNPM workspaces:

```json
{
"dependencies": {
"react-on-rails": "*"
"react-on-rails": "workspace:*"
}
}
```

**DO NOT USE:**

- `"workspace:*"` - This is Yarn Berry v2+ syntax, will cause installation errors
- `"*"` - This is Yarn Classic v1.x syntax
- `"file:../react-on-rails"` - This bypasses workspace resolution

### Why `*` Works
### Why `workspace:*` Works

In Yarn Classic workspaces:
In PNPM workspaces:

- `"*"` tells Yarn to resolve to the local workspace package
- Yarn automatically links to the workspace version
- This is the official Yarn v1 workspace syntax
- `"workspace:*"` tells PNPM to resolve to the local workspace package
- PNPM automatically links to the workspace version
- This is the official PNPM workspace syntax

### Testing Workspace Changes

Expand All @@ -149,32 +149,34 @@ When modifying workspace dependencies in package.json:
rm -rf node_modules

# 2. Test CI command - this will fail immediately if syntax is wrong
yarn install --frozen-lockfile
pnpm install --frozen-lockfile

# 3. Verify workspace linking worked
yarn workspaces info
pnpm -r list

# 4. Test that packages can import each other
yarn run build
pnpm run build
```

## Real Examples: What Went Wrong

### Example 1: Path Reference Issue (Sep 2024)

We moved `node_package/` → `packages/react-on-rails/`. The path in
package-scripts.yml was updated to `packages/react-on-rails/lib/ReactOnRails.full.js`.
Later, the structure was partially reverted to `lib/` at root, but package-scripts.yml
package.json was updated to `packages/react-on-rails/lib/ReactOnRails.full.js`.
Later, the structure was partially reverted to `lib/` at root, but package.json
wasn't updated. This broke yalc publish silently for 7 weeks. Manual testing of
`yarn run yalc.publish` would have caught this immediately.
`pnpm run yalc:publish` would have caught this immediately.

### Example 2: Workspace Protocol Issue (Nov 2024)
### Example 2: Workspace Protocol Migration (2024)

Changed workspace dependencies from `"*"` to `"workspace:*"` without testing clean install.
This caused CI to fail with: `Couldn't find any versions for "react-on-rails" that matches "workspace:*"`
When migrating from Yarn to PNPM, workspace dependencies needed to change from `"*"` to `"workspace:*"`.

**Root cause:** Assumed `workspace:*` was standard, but it's only supported in Yarn Berry v2+.
This project uses Yarn Classic v1.x which requires `"*"` for workspace dependencies.
**Root cause:** Different package managers use different workspace protocols:

**Lesson:** ALWAYS test `yarn install --frozen-lockfile` after modifying workspace dependencies.
- Yarn Classic v1.x uses `"*"` for workspace dependencies
- PNPM uses `"workspace:*"` for workspace dependencies
- Yarn Berry v2+ also uses `"workspace:*"`

**Lesson:** ALWAYS test `pnpm install --frozen-lockfile` after modifying workspace dependencies.
Your local node_modules masked the issue - CI starts fresh and caught it immediately.
30 changes: 19 additions & 11 deletions .github/workflows/examples.yml
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ jobs:
fail-fast: false
matrix: ${{ fromJson(needs.setup-matrix.outputs.matrix) }}
env:
SKIP_YARN_COREPACK_CHECK: 0
BUNDLE_FROZEN: ${{ matrix.dependency-level == 'minimum' && 'false' || 'true' }}
runs-on: ubuntu-22.04
steps:
Expand All @@ -117,18 +116,26 @@ jobs:
uses: ./.github/actions/setup-node-with-retry
with:
node-version: 20
# Retry logic now handles V8 crashes automatically
# Tracking: https://github.com/actions/setup-node/issues/1028
cache: yarn
cache-dependency-path: '**/yarn.lock'
- name: Setup pnpm
uses: pnpm/action-setup@v4
- name: Get pnpm store directory
shell: bash
run: echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV
- name: Setup pnpm cache
uses: actions/cache@v4
with:
path: ${{ env.STORE_PATH }}
key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-pnpm-store-
- name: Print system information
run: |
echo "Linux release: "; cat /etc/issue
echo "Current user: "; whoami
echo "Current directory: "; pwd
echo "Ruby version: "; ruby -v
echo "Node version: "; node -v
echo "Yarn version: "; yarn --version
echo "pnpm version: "; pnpm --version
echo "Bundler version: "; bundle --version
- name: run conversion script to support shakapacker v6
if: matrix.dependency-level == 'minimum'
Expand All @@ -140,14 +147,15 @@ jobs:
key: package-app-gem-cache-${{ hashFiles('react_on_rails/Gemfile.lock') }}-ruby${{ matrix.ruby-version }}-${{ matrix.dependency-level }}
- id: get-sha
run: echo "sha=\"$(git rev-parse HEAD)\"" >> "$GITHUB_OUTPUT"
- name: Install Node modules with Yarn for renderer package
- name: Install Node modules with pnpm for renderer package
run: |
yarn install --no-progress --no-emoji ${{ matrix.dependency-level == 'latest' && '--frozen-lockfile' || '' }}
sudo yarn global add yalc
pnpm install ${{ matrix.dependency-level == 'latest' && '--frozen-lockfile' || '--no-frozen-lockfile' }}
pnpm add -g yalc
- name: yalc publish for react-on-rails
# Use yarn workspace script to publish all workspace packages to yalc
# Use pnpm workspace script to publish all workspace packages to yalc
# Runs the "yalc:publish" script defined in each workspace's package.json
run: yarn run yalc:publish
run: pnpm yalc:publish

- name: Install Ruby Gems for package
run: |
cd react_on_rails
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/gem-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ jobs:
echo "Current directory: "; pwd
echo "Ruby version: "; ruby -v
echo "Node version: "; node -v
echo "Yarn version: "; yarn --version
echo "Bundler version: "; bundle --version
- name: run conversion script to use minimum supported dependency versions
if: matrix.dependency-level == 'minimum'
Expand Down
Loading
Loading