Skip to content

chore(room-kit): bump devDeps react-native-video-plugin to ^2.0.0-alpha.0#1644

Open
sapta100ms wants to merge 5 commits into
mainfrom
chore/room-kit-video-plugin-alpha-bump
Open

chore(room-kit): bump devDeps react-native-video-plugin to ^2.0.0-alpha.0#1644
sapta100ms wants to merge 5 commits into
mainfrom
chore/room-kit-video-plugin-alpha-bump

Conversation

@sapta100ms
Copy link
Copy Markdown
Contributor

Follow-up to #1643. The previous PR bumped the example app's video-plugin to ^2.0.0-alpha.0, but CI's Link react-native-hms to room-kit step runs from the library directory (packages/react-native-room-kit/), not the example. The library's own devDependencies and additionalDependencies still pinned video-plugin 1.1.4 / 1.1.0, which peer-deps hms <2.0.0 and re-triggered the same ERESOLVE conflict against the locally-packed hms 2.0.0-alpha.1 tarball.

Two fields updated:

  • devDependencies["@100mslive/react-native-video-plugin"]: 1.1.4^2.0.0-alpha.0
  • additionalDependencies["@100mslive/react-native-video-plugin"]: 1.1.0^2.0.0-alpha.0

package-lock.json regenerated accordingly.

Consumer impact: none. devDependencies and the non-standard additionalDependencies are not installed when customers run npm install @100mslive/react-native-room-kit. This PR affects only our repo's CI + local dev setup.

…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.
@sapta100ms sapta100ms requested a review from ygit as a code owner May 15, 2026 06:01
@trunk-io
Copy link
Copy Markdown

trunk-io Bot commented May 15, 2026

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

PR Review

Summary

This PR correctly addresses the ERESOLVE peer dependency conflict in CI by bumping the video-plugin version in the room-kit library's devDependencies and additionalDependencies. The change is a logical follow-up to #1643.

Observations

Positive:

  • ✅ Correctly identifies and fixes the root cause - the library's own dependencies, not just the example app
  • ✅ Updates both devDependencies (line 133) and additionalDependencies (line 175) consistently
  • ✅ Clear PR description explaining the impact and scope
  • ✅ Zero consumer impact - these fields don't affect package consumers
  • ✅ Uses caret range ^2.0.0-alpha.0 for appropriate version flexibility
  • ✅ package-lock.json properly regenerated

Minor Issue:
⚠️ package.json uses yarn but CLAUDE.md mandates npm

  • Line 192: "packageManager": "^yarn@1.22.15"
  • Line 35: "example": "yarn --cwd example"
  • Line 36: "bootstrap": "yarn example && yarn install"

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:

  • Have you tested that npm install runs cleanly in packages/react-native-room-kit/ after these changes?
  • Does the CI "Link react-native-hms to room-kit" step now pass?

Recommendation

Approve - 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.
@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

PR Review

This PR correctly addresses the peer dependency resolution issue that was partially fixed in #1643. The changes are well-scoped and appropriate.

Summary

Approved ✓ - The fix properly resolves the CI dependency conflict by updating the library's own dev dependencies to match the alpha version requirements.

Positive Observations

  • Root Cause Fix: Correctly identified that chore(example/room-kit): bump react-native-video-plugin to ^2.0.0-alpha.0 #1643 only updated the example app, while CI operates from the library directory
  • Minimal Scope: Only touches necessary fields (devDependencies + additionalDependencies)
  • Consumer Safety: Changes are isolated to dev/build environment, zero customer impact
  • CI Alignment: Adding --legacy-peer-deps to all workflow steps provides consistent resolution strategy

Code Quality

✓ Version range ^2.0.0-alpha.0 is appropriate for alpha testing
✓ package-lock.json regenerated correctly
✓ Consistent changes across both workflow files (build.yml, build-newarch.yml)
✓ Follows conventional commit format per CLAUDE.md

Minor Observations

  1. --legacy-peer-deps flag: This is added to 4 workflow steps. While it resolves the immediate issue, it's worth noting:

    • It bypasses peer dependency validation entirely
    • Consider removing this once video-plugin reaches stable 2.0.0 and peer deps align naturally
    • Not a blocker, but a tech debt marker for future cleanup
  2. additionalDependencies: This is a non-standard package.json field. While not an issue for this PR, it may warrant documentation about its purpose in the repo (appears to be for example apps or tooling)

No Issues Found

  • No security concerns
  • No performance impact
  • No test coverage needed (build/CI configuration only)
  • No code duplication
  • No refactoring opportunities

Recommendation

Merge 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.
@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

PR Review

Summary: 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

  1. Problem correctly identified: The PR description clearly explains the root cause - the library's devDependencies still had the old video-plugin version with incompatible peer deps
  2. Minimal, focused changes: Only updates what's necessary (package.json fields + workflow flags)
  3. Zero consumer impact: Correctly notes that devDependencies don't affect downstream users
  4. Workflow fix: Adding --legacy-peer-deps to CI steps is a pragmatic solution during the alpha phase

🔍 Observations

  1. Missing package-lock.json: The PR description mentions "package-lock.json regenerated accordingly" but it's not in the changed files. This should be committed to ensure deterministic installs in CI and local dev.

  2. additionalDependencies field: This is a non-standard field (not recognized by npm). If it's used by custom tooling in your monorepo, that's fine, but worth documenting why both this and devDependencies need the same update.

  3. --legacy-peer-deps in CI: While pragmatic for unblocking CI during alpha, consider documenting:

    • Why it's needed (peer dep conflicts during alpha transition)
    • Plan to remove it (once video-plugin + hms stable versions align)
    • Adding a TODO comment or GitHub issue to track removal

📝 Recommendations

High Priority:

  • Regenerate and commit packages/react-native-room-kit/package-lock.json with npm install after the package.json changes

Low Priority:

  • Add a comment in the workflow files explaining the --legacy-peer-deps flag is temporary for alpha versions
  • Consider creating a follow-up issue to remove --legacy-peer-deps once stable 2.0.0 releases are available

✅ Code Quality

  • Changes follow conventional commit format ✓
  • Consistent version bumps across both fields ✓
  • Clear PR description with context ✓

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.
@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

PR Review

Summary

This 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

  • Correct root cause fix: Addresses the actual CI failure point (library directory, not just example)
  • Clear PR description: Excellent explanation of why this follow-up was needed
  • Zero consumer impact: Changes only affect CI/dev dependencies, not published package
  • Consistent versioning: Both fields updated to the same version range (^2.0.0-alpha.0)

Issues and Concerns

1. --force flag is a code smell

The CI workflow changes add --force to npm install commands. This bypasses conflict resolution and can mask underlying issues.

Questions:

  • Is --force actually needed after this PRs package.json changes?
  • Have you tested whether CI passes without --force after the version bumps?
  • If force is truly required, this should be documented with a comment explaining why

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-standard

The 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 devDependencies

Using ^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 diff

The PR description mentions package-lock.json was regenerated, but its not in the file list. Verify package-lock.json changes are committed.

Testing Recommendations

  1. Verify CI passes on this PR
  2. Test whether CI would pass without the --force flags
  3. Confirm no package-lock.json drift in local testing

Code Quality

  • Follows conventional commit format
  • Minimal, focused change
  • Clear PR description
  • Consider documenting the --force workaround if truly necessary

Verdict

Approve 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).
@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

PR Review

Summary

This 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

  • Well-scoped fix: Only updates the necessary fields to resolve the peer dependency conflict
  • Zero consumer impact: Changes are limited to devDependencies and additionalDependencies
  • Proper alpha versioning: Uses ^2.0.0-alpha.0 consistently
  • Lock file updated: package-lock.json correctly reflects the changes

📝 Observations

  1. Version consistency: The package.json shows the room-kit version bumped to 2.0.0-alpha.0 (line 8 in diff), which aligns with the alpha release cycle mentioned in the PR context.

  2. react-native-reanimated addition: The diff shows react-native-reanimated moved from peer to dev dependency (now listed explicitly). This appears unrelated to the stated PR goal but is likely a side effect of lock file regeneration. Consider mentioning this in the PR description if intentional.

  3. Peer dependency update: The lock file shows peer dependency for hms updated from 1.12.1 to 2.0.0-alpha.1, which is consistent with the local tarball reference - this is expected for alpha testing.

Minor Suggestions

  • The additionalDependencies field is noted as "non-standard" in the PR description. Consider adding a brief comment in package.json explaining what this field is used for (appears to be room-kit specific).

✅ Approval

This 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.

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