[NO-CDX] Fix breaking React compatibility tests - #56
Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Updates the React compatibility GitHub Actions workflow to adjust how dependencies are installed during the matrix run.
Changes:
- Switched the library dependency install step in the React compat workflow from
npm citonpm i.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| node-version: "22.18.0" | ||
| - name: Install library deps | ||
| run: npm ci | ||
| run: npm i |
There was a problem hiding this comment.
Code Review
This PR fixes the React compat CI workflow by replacing npm ci with npm i --no-audit --no-fund --package-lock=false to resolve installation failures, but the change trades reproducibility for flexibility in a way that may mask the root problem.
Inline comments: 1 discussion added
Overall Assessment:
| node-version: "22.18.0" | ||
| - name: Install library deps | ||
| run: npm ci | ||
| run: npm i --no-audit --no-fund --package-lock=false |
There was a problem hiding this comment.
Important Issue: Replacing npm ci with npm i --package-lock=false undermines CI reproducibility. npm ci exists precisely to enforce a clean, deterministic install from the lockfile — which is what CI should be doing. Using npm i --package-lock=false instead means dependencies can silently resolve to different (potentially breaking or vulnerable) versions on each run.
The root cause here is that npm ci was failing, likely because the package-lock.json is out of sync with package.json, or the lockfile does not exist. The correct fix is to identify and resolve that mismatch rather than bypassing the lockfile altogether.
If the intent is to allow installing the latest compatible versions of deps for testing against different React versions, a more explicit and intentional approach would be to document this decision and use a separate package.json install step that updates only the peer dependency (React) before running npm ci on the rest — or to rely on the already-present npm ci --no-audit --no-fund in the fixture install step (line 39) which already correctly uses npm ci.
Suggested fix: Investigate why npm ci was failing (likely a stale or missing package-lock.json) and fix the lockfile rather than bypassing it. If dynamic resolution is truly required for this step, add a comment explaining the rationale.
No description provided.