Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Expo 52 #584

Draft
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

enigma0Z
Copy link

@enigma0Z enigma0Z commented Dec 12, 2024

This PR is WIP and currently includes changes from PR #583 as that PR is a prerequisite for this PR. Once this is merged, rebase this branch against master

Summary

  • Upgrade to Expo 52
  • Upgrade React Native
  • Upgrade React
  • Fix the things that break as a result of these changes

Rationale

  • A newer version of Expo is required for Chromecast Support, a highly requested feature (and one I intend to see done personally). See issue Add Chromecast Support #16
  • Current expo (46) is ~2 years out of date
  • Current deployed to the IOS store react is out of date (~16)

Details

Package updates

  • Expo 46 -> 52
  • React 18.0 -> 18.3
  • React Native 0.69.9 -> 0.76.5
  • Supported iOS 11+ -> 15.1+
  • Jellyfin Mobile version 1.6.0 -> 1.7.0 (per roadmap and @thornbill)

Code changes

  • A few components were using propTypes and then wrapping the result in forwardRef(), build errors indicated that the pattern should be wrap first, then set prop types
  • Test snapshots updated
  • Three tests disabled due to incompatibilities with dependencies. See details in sections below.
    • ErrorScreen.test
    • NativeShellWebView.test
    • RefreshWebView.test

New architecture

Expo Go will, for SDK 52+ only support the new react-native architecture from React Native 0.76+. Expo 52 itself supports both new and old, but will begin deprecating its support for the previous architecture of react native.

This has been turned on for this project as of the update. The major affected areas are where something in-code calls out to a native method. Nothing (that I can find) in jellyfin-expo does this so it was an easy thing.

Broken tests (OUT OF SCOPE)

Two main unresolvable errors were generated by three test suites after all the updates were said and done:

  • RangeError: Invalid string length
    • Components Affected
      • Anything that use a RefreshControl
      • Components
        • ErrorScreen
        • NativeShellWebView
        • RefreshWebView
    • Detail
      • RefreshControls output of toJSON() when render()ed by the test renderer includes a circular refeference
      • Jest's .toMatchSnapshot()barfs on this generating a string of "invalid length"
      • If you debug on the offending step and wrap this in JSON.stringify() there are some additional clues, essentially that a react internal node references the RefreshControl node and a few layers down wraps back up to the same object
    • Potential cause sources
      • RefreshControl implementation
      • @testing-library/react-native (provides render() method)
      • Changed implementation expectations in jellyfin-expo source code (unlikely)
  • TypeError: _NativeRNCWebView.default.shouldStartLoadWithLockIdentifier is not a function (it is undefined)
    • Components affected
      • Anything that references react-native-webview
      • Components
        • NativeShellWebView
        • RefreshWebView
    • Detail
      • react-native-webview fails to load in Jest when running the test suites. The app is unaffected.
      • An older version of react-native-webview (per the instructions on this bug) fixes this, but creates more issues:
        • Additional Invalid String Length errors
        • Main home screen tab won't render
    • Potential cause sources
      • react-native-webview
      • @testing-library/react-native
      • Jest somwhere (may be a mock is needed??)

Tasks

Upgrade Tasks

  • Update breaking dependencies
    • @react-native-community/masked-view -> @react-native-masked-view/masked-view
      • Make sure podfile is updated or remove this as a dependency
  • Update Expo
  • Fix anything basic broken by the expo upgrade
    • Expo SDK
    • Dependencies
    • Update native projects
    • Release note instructions
    • Broken tests (all of them apparently)
    • Update podfile

Fixes

  • A props object containing a "key" prop is being spread into JSX

Finalization

  • Validate functionality manually

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jan 1, 2025
@thornbill thornbill modified the milestones: vFuture Release, v1.8.0 Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
Development

Successfully merging this pull request may close these issues.

3 participants