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

MHP-2029: Upgrade packages #570

Closed
wants to merge 3 commits into from
Closed

Conversation

OzzieOrca
Copy link
Contributor

  • Upgrade to latest React, React Native, Babel 7
  • Upgrade all other packages
  • Add podfile with Facebook SDK to remove dependency on it being manually downloaded to your Documents folder
  • Commit Pods directory per documentation suggestion and allow other developers to not have to run pod install locally
  • Get running with Xcode 10

I started down this rabbit hole and it took a lot longer than I expected... I got so close. Everything works except the tests. Enzyme doesn't seem to like the new React context API yet... enzymejs/enzyme#1647 There doesn't seem to be much progress on that issue. Guess this might sit for a little while. I didn't commit the snapshot updates yet cuz they'll probably change

The latest React Native requires react@16.6.0-alpha.8af6728. We could wait until that lands in a stable release. But I think the newest React Native is needed to support the Xcode 10 build process.

With Xcode 10, for now you may need to run the commands at:
facebook/react-native#20774 (comment)
Otherwise during the iOS build there will be missing file errors.

Do any of you guys feel comfortable reviewing the iOS stuff? Is there anyone I can ask? I guess I mainly want to see if the podfile setup is correct.

@dsgoers
Copy link

dsgoers commented Oct 17, 2018

Why are there so many files changed??? Lol

@OzzieOrca
Copy link
Contributor Author

I committed the Pods folder... https://guides.cocoapods.org/using/using-cocoapods.html#should-i-check-the-pods-directory-into-source-control suggested it. We could make everyone run pod install on their own machine if you like that better. I generally don't like commiting vendor directories but it was suggested and I figured we probably wouldn't touch the podfile much and it's just another step to get the repo running on a new machine. I think having the FacebookSDK in the podfile is better than manually extracted into Documents.

@OzzieOrca
Copy link
Contributor Author

OzzieOrca commented Oct 17, 2018

I think I started down the podfile route cuz I was getting errors with the Facebook SDK. I don't remember what they were. Idk if moving it to pods was the only solution but seemed like it would be an improvement. Also that's what the new Facebook docs say in their install instructions.

@dsgoers
Copy link

dsgoers commented Oct 17, 2018

That's a great idea. I'm glad you did that. Will it increase the size of our app? Also, could perhaps add that change in a separate commit so it's easier to review?

@OzzieOrca
Copy link
Contributor Author

I moved the Podfile stuff to a separate commit. I didn't really want to reconcile the updates to the ios project (the giant project.pbxproj) from both the pods stuff and the react native upgrade so it's still in the second commit. The Podfile commit won't work on its own (although we could work on configuring it separately if needed).

I meant to check and see if all of these pods are needed. I guess facebook has split it into separate packages. I was definitely getting errors about FBSDKShareKit being missing. Maybe the react native package needs them all.

pod 'FBSDKCoreKit'
pod 'FBSDKLoginKit'
pod 'FBSDKShareKit'

Idk that it'll change the size of our app since this should be the same FB library as before, just imported differently. But I haven't checked it.

- Remove dependency on having to manually downloaded the Facebook SDK to your Documents folder
- Commit Pods directory per documentation suggestion and allow other developers to not have to run pod install locally
- Integration of Podfile into ios project happens in next commit
- Upgrade to latest React, React Native, Babel 7
- Upgrade all other packages
- Integrate Podfile added in last commit into ios project
- Get running with Xcode 10
@OzzieOrca OzzieOrca changed the title Upgrade packages MHP-2029: Upgrade packages Dec 1, 2018
@dsgoers
Copy link

dsgoers commented Apr 4, 2019

Can we close this?

@OzzieOrca OzzieOrca closed this Apr 11, 2019
@OzzieOrca OzzieOrca deleted the MHP-2029-upgrade-packages branch April 11, 2019 18:22
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.

None yet

2 participants