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

iOS Swift Conversion #2527

Merged
merged 22 commits into from
May 19, 2022

Conversation

nickfujita
Copy link
Collaborator

@nickfujita nickfujita commented Nov 4, 2021

This PR is a conversion of the iOS implementation from Objective-c to Swift.

Goals of conversion

  • Make the iOS codebase more accessible to developers coming from JS side to debug and contribute to the iOS side
  • Cleanup codebase by utilizing more modern code patterns in Swift
  • Prep codebase for unit tests by starting to modularize logic into smaller functions
  • Improve code navigation by organize code into smaller files, grouping similar functions

During the conversion process some updates to the code structure were also made

  • Modularize codebase from single file to smaller focused files
  • Untangled large nested IF statements
  • Added more null checks, since Swift is more strict with null pointers
  • Added property to allow for decoding of local video sources with self contained key for offline playback
  • Updates example apps to test react-native 0.63.4 and uses auto native dependency imports for android and ios

Test Instructions

If you have an existing project, please give this branch a try and reply with any issues, and will be happy to have a look.

iOS project update

There is a breaking change in terms of setup, where you will need to add support for static dependency linking in the Podfile in your ios project. For details, please see the example ios project: https://github.com/crunchyroll/react-native-video/blob/ios-swift-conversion/examples/basic/ios/Podfile#L5

Testing in updated basic example apps

yarn xbasic install

iOS Example

yarn xbasic ios

Android Example

yarn xbasic android

Windows Example

yarn xbasic windows

@nickfujita
Copy link
Collaborator Author

@zchwyng @sebjacobs @Fedeorlandau would you mind testing this in your existing project or any of the updated example project in this repo?

@Fedeorlandau
Copy link

Fedeorlandau commented Feb 3, 2022

@zchwyng @sebjacobs @Fedeorlandau would you mind testing this in your existing project or any of the updated example project in this repo?

Hi @nickfujita, just raised it to the management team the need for us to start making PRs to the main repository as well and not only to our fork https://github.com/valtech-sd/react-native-video so we can get the latest fixes from you.
We've created a ticket in our internal tool to document all the features that we added and support the swift migration with the Dolby Atmos fix. Now it's up to the client to approve this and give us green light to update the PRs and stuff.

I'll let you know how it goes and thank you for you work here

@nickfujita
Copy link
Collaborator Author

@Fedeorlandau that's great! was not even aware of the changes your team has been making in that branch. Excited to have a look through and see what we can get back into the main repo. 🙏🏼

@nickfujita
Copy link
Collaborator Author

hey @Fedeorlandau wanted to check back in with you to see if you had some time to test out this PR?

@Fedeorlandau
Copy link

hey @Fedeorlandau wanted to check back in with you to see if you had some time to test out this PR?

Hi there. Not really, I'm not in the project anymore. @adriandragdolby @ShaneMckenna23 could you please check this? Thanks

@nickfujita
Copy link
Collaborator Author

Hi @adriandragdolby @ShaneMckenna23 would either of you be able to perform a test of this Swift conversion? The test application sin the project have been updates, so you can simply run the command yarn xbuild ios to quickly test the player

@hueniverse
Copy link
Contributor

hueniverse commented Apr 20, 2022

@nickfujita I think v6 would be the right time to just go ahead and make the move! If we break stuff, we will get fresh report from the people who upgrade and test it out. Care to update the branch to current master?

It would be best if we can break this PR into smaller pieces as it changes a lot of files including Android that would be much easier to review in smaller chunks.

@hueniverse hueniverse added this to the 6.0.0-alpha1 milestone Apr 20, 2022
@armands-malejevs armands-malejevs added 2 and removed 3 labels May 17, 2022
examples/basic/android/app/proguard-rules.pro Outdated Show resolved Hide resolved
examples/basic/android/app/_BUCK Outdated Show resolved Hide resolved
examples/basic/ios/VideoPlayer/main.m Outdated Show resolved Hide resolved
@nickfujita
Copy link
Collaborator Author

@cobarx The example projects were recreated with a create react native app template, so some of the things were removed, such as comments, rules, etc. Added back the ones that were mentioned above.

@cobarx cobarx self-requested a review May 18, 2022 04:41
@nickfujita nickfujita merged commit 68b9db4 into TheWidlarzGroup:master May 19, 2022
@hueniverse hueniverse removed 2 labels May 20, 2022
@hueniverse
Copy link
Contributor

Fantastic work everyone! Thanks @nickfujita for sticking with it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants