chore(room-kit): bump devDeps react-native-video-plugin to ^2.0.0-alpha.0#1644
chore(room-kit): bump devDeps react-native-video-plugin to ^2.0.0-alpha.0#1644sapta100ms wants to merge 5 commits into
Conversation
…ha.0 Follow-up to #1643. The example-app bump unblocked the example install, but CI's 'Link react-native-hms to room-kit' step runs npm install in the library directory (packages/react-native-room-kit), not the example. The library still listed video-plugin 1.1.4 / 1.1.0 in devDependencies + additionalDependencies, which peer-deps hms <2.0.0 and re-triggered the ERESOLVE conflict against the locally-packed hms 2.0.0-alpha.1 tarball. Both refs bumped to ^2.0.0-alpha.0. Lockfile regenerated. devDependencies and the non-standard additionalDependencies are not installed by npm for end consumers, so this change only affects our CI.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR ReviewSummaryThis PR correctly addresses the ERESOLVE peer dependency conflict in CI by bumping the video-plugin version in the room-kit library's ObservationsPositive:
Minor Issue:
According to CLAUDE.md: "ALWAYS use npm (not yarn) for consistency" This is outside the scope of this PR (pre-existing), but should be addressed in a separate cleanup PR to align with the documented package manager standards. Verification Questions:
RecommendationApprove - Changes are correct and minimal. The yarn usage is a pre-existing issue that should be tracked separately. Review by Claude Code |
This npm install was the only one in build*.yml that didn't pass --legacy-peer-deps. It ran into a separate peer-dep conflict after the video-plugin one was fixed: peer react-native@"0.81 - 0.85" from react-native-reanimated@4.3.1 peer react-native@">=0.77.3" from @100mslive/react-native-hms@2.0.0-alpha.1 dev react-native@"0.77.3" from the root project The room-kit library's devDep on react-native is still 0.77.3 (pre-alpha example app bump). Bumping the library's whole dev stack is a separate cleanup; for now, match the rest of CI by passing --legacy-peer-deps so the alpha-track install resolves.
PR ReviewThis PR correctly addresses the peer dependency resolution issue that was partially fixed in #1643. The changes are well-scoped and appropriate. SummaryApproved ✓ - The fix properly resolves the CI dependency conflict by updating the library's own dev dependencies to match the alpha version requirements. Positive Observations
Code Quality✓ Version range Minor Observations
No Issues Found
RecommendationMerge when CI passes. This is a straightforward dependency alignment fix that unblocks alpha testing. |
The lockfile change in 2ac036a (regenerated locally with --legacy-peer-deps) appeared to cause an npm internal error ('Cannot read properties of undefined (reading spec)') when CI later ran 'npm install <hms-tarball> --legacy-peer-deps' on top. Letting CI's own 'npm ci --legacy-peer-deps || npm install --legacy-peer-deps' fallback regenerate the lockfile fresh, given the package.json bump.
PR ReviewSummary: This PR correctly addresses the CI ERESOLVE conflict by bumping the video-plugin version in the room-kit library's dev dependencies. The changes align with the PR's stated goal and are a proper follow-up to #1643. ✅ Positives
🔍 Observations
📝 RecommendationsHigh Priority:
Low Priority:
✅ Code Quality
Verdict: Approve pending package-lock.json commit. The core fix is correct. |
Previous attempt with --legacy-peer-deps triggered an npm internal error: 'Cannot read properties of undefined (reading spec)'. This is a known class of npm bugs when --legacy-peer-deps interacts with local tarball installs. --force is more permissive and uses a different resolution path; worth a shot before falling back to manual tarball extraction.
PR ReviewSummaryThis PR completes the fix started in #1643 by updating the video-plugin dependency in the library's (room-kit) package.json. The previous PR only updated the example app, but CI runs from the library directory where devDependencies and additionalDependencies still pinned the old version. Positive Aspects
Issues and Concerns1. --force flag is a code smellThe CI workflow changes add --force to npm install commands. This bypasses conflict resolution and can mask underlying issues. Questions:
Recommendation: Test removing --force in a follow-up. If conflicts persist, investigate the actual peer dep mismatch rather than forcing through it. 2. additionalDependencies is non-standardThe additionalDependencies field is not a standard npm field. What consumes this field? Should it stay in sync with devDependencies? Why were versions different (was 1.1.0 vs 1.1.4 in dev)? Recommendation: Consider adding a comment in package.json explaining this custom field. 3. Alpha dependency in devDependenciesUsing ^2.0.0-alpha.0 means any future alpha releases (alpha.1, alpha.2) will auto-update. For CI stability, you might want to pin the exact version (2.0.0-alpha.0) until the package is stable. 4. Missing package-lock.json in diffThe PR description mentions package-lock.json was regenerated, but its not in the file list. Verify package-lock.json changes are committed. Testing Recommendations
Code Quality
VerdictApprove with minor concerns. The core fix is correct, but the --force flag suggests this might be papering over a deeper peer dependency issue. If CI passes without --force after this PRs changes, remove it in a follow-up commit. |
…force from CI The library's internal tsc needs reanimated 3.x type signatures (HLSPlayerControls.tsx and similar use the pre-v4 'useAnimatedStyle returns ViewStyle' API). The pin existed implicitly via package-lock.json at e1aeea7 (the last-passing CI), but any package.json change that caused npm to re-resolve pulled in reanimated 4.x and broke tsc with TS2559. Making the pin explicit in devDependencies: - Library's tsc continues to compile against reanimated 3 types - Compiled .d.ts files don't expose internal animated-style types, so customers can still use reanimated 4 at runtime - Consumer-facing peerDeps (react-native-reanimated >=3.0.0) unchanged Also reverts the --force flag from build.yml + build-newarch.yml. With package.json + lockfile back in sync, npm install resolves cleanly without --force (which was pulling reanimated 4 against the pin). Verified locally: npm pack + npm install <hms.tgz> + npm run prepack completes successfully with all three bob targets (commonjs, module, typescript).
PR ReviewSummaryThis PR correctly addresses the CI dependency resolution issue by bumping the video-plugin version in the room-kit library's dev dependencies. The changes are minimal and targeted. ✅ Positives
📝 Observations
Minor Suggestions
✅ ApprovalThis PR is ready to merge. It accomplishes exactly what it sets out to do: fix the CI ERESOLVE conflict by aligning video-plugin versions across all dependency fields. No security concerns, no performance impact, and follows the conventional commit format per CLAUDE.md guidelines. |
Follow-up to #1643. The previous PR bumped the example app's video-plugin to
^2.0.0-alpha.0, but CI'sLink react-native-hms to room-kitstep runs from the library directory (packages/react-native-room-kit/), not the example. The library's owndevDependenciesandadditionalDependenciesstill pinned video-plugin1.1.4/1.1.0, which peer-deps hms<2.0.0and re-triggered the same ERESOLVE conflict against the locally-packed hms2.0.0-alpha.1tarball.Two fields updated:
devDependencies["@100mslive/react-native-video-plugin"]:1.1.4→^2.0.0-alpha.0additionalDependencies["@100mslive/react-native-video-plugin"]:1.1.0→^2.0.0-alpha.0package-lock.jsonregenerated accordingly.Consumer impact: none.
devDependenciesand the non-standardadditionalDependenciesare not installed when customers runnpm install @100mslive/react-native-room-kit. This PR affects only our repo's CI + local dev setup.