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

feat: Upgrade SDK to 9 + clean up on old FB stuff. #1

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Conversation

thebergamo
Copy link
Owner

@thebergamo thebergamo commented Mar 9, 2021

Upgrading FB SDK to version 9.
Refers to: facebookarchive/react-native-fbsdk#833
Also some clean up.

  • Upgrade FB SDK Version
  • Renaming Project
  • Define Code of Conduct
  • Finish clean up on old FB stuff
  • Test automations (remove react-native-fbsdk one)
  • Automate release to NPM

@mikehardy
Copy link
Collaborator

I can recommend a fully automated semantic release setup using the Angular conventional-commit guidelines personally if you want to release on every change (which is actually good practice)

react-native-webview is a good example of that style - they do a great job https://github.com/react-native-webview/react-native-webview/blob/099087f8d95fbc600d2a38a11ccb87481e687521/package.json#L20

If you want to still do semantic release (for automated versioning) based on conventional commit messages, but you want to publish only when you want (by hitting an automated button though) then the AnkiDroid github workflow is a good example of automated manual publish, but would need a versioning hook into conventional commit - https://github.com/ankidroid/ankidroiddocs/blob/master/.github/workflows/publish.yml

I can recommend the PR check for conventional commit messages that react-native-firebase uses as a github workflow to make sure versioning will work correctly - https://github.com/invertase/react-native-firebase/blob/master/.github/workflows/pr_title.yml

If you want to publish mostly manually then the react-native-device-info "shipit" command (based on np package) works quite well - https://github.com/react-native-device-info/react-native-device-info/blob/fad55b3c65379331b57b97d6997aaf5fbe6342b5/package.json#L35

I would definitely go for the PR Title check so sem-ver is thought of immediately by all people related to a PR

Until CI was solid I would go for a workflow like react-native-webview's publish workflow but called by a publish.yml that was set as "workflow_dispatch" (which is Github Actions language for: gives you a manual button on the Actions tab to run it when you want, instead of on "push" events fully automated)

Once CI was solid I would go full semantic release like react-native-webview and run the publish any time a push to main branch was made

All just opinions, but hopefully the references to working code in real projects helps :-)

@thebergamo
Copy link
Owner Author

Lots of references! I was totally thinking about it today on to automate the publishing. Having it as semantic versioning will make it easier.
I will try to setup it today or tomorrow within this PR so I can release the first version of it and make It available to others soon.

@thebergamo
Copy link
Owner Author

@mikehardy @rnike I believe now it is in a state that we could release it to public at least to feel this gap of FB SDK 9+.

Later on I plan to remove flow and start migrate to TS. I would really appreciate some help with the e2e tests :)

@thebergamo thebergamo self-assigned this Mar 10, 2021
@thebergamo thebergamo added enhancement New feature or request help wanted Extra attention is needed labels Mar 10, 2021
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Looks really nicely done on the github workflows and release stuff. I've got a lot on my plate right now and there were no E2E tests prior so that certainly shouldn't stop release in my opinion. Hopefully I'll have time in the future to get some automated example construction going (like https://github.com/react-native-device-info/react-native-device-info/blob/master/refresh-example.sh) and the E2E stuff working

@thebergamo thebergamo merged commit 7b3809b into master Mar 11, 2021
@thebergamo thebergamo deleted the sdk-upgrade branch March 11, 2021 08:06
thebergamo added a commit that referenced this pull request Mar 11, 2021
feat: Upgrade SDK to 9 + clean up on old FB stuff.
thebergamo added a commit that referenced this pull request Mar 11, 2021
feat: Upgrade SDK to 9 + clean up on old FB stuff.
@thebergamo
Copy link
Owner Author

released: https://www.npmjs.com/package/react-native-fbsdk-next

@mikehardy
Copy link
Collaborator

mikehardy commented Mar 11, 2021

@thebergamo fantastic!

I test-integrated this in my work project, which uses typescript

Here is what I had to do to get it to work vs the old one:

  • switch module yarn remove react-native-fbsdk && yarn add react-native-fbsdk-next
  • purge old types: yarn remove @types/react-native-fbsdk
  • stub out new types: mkdir @types/react-native-fbsdk-next && echo "declare module \"react-native-fbsdk-next\"" > @types/react-native-fbsdk-next/index.d.ts
  • alter all imports in my source code of course, to new package name
  • check/alter android/[app/]build.gradle where I had an android sdk version of 8.2.0, to 9.0.0
  • check/alter ios/Podfile to make sure you don't have the ios version pinned somehow
  • cd ios && pod install && ..
  • avoid ios crash by initializing correctly, edit ./ios/<APP NAME>/AppDelegate.m and add this in didFinishLaunchingWithOptions method:
  /** Test for FBSDK 9+ **/
  [[FBSDKApplicationDelegate sharedInstance] application:application
                           didFinishLaunchingWithOptions:launchOptions];
  /** End of FBSDK **/

After that, for my use cases (just a login button and simple graph request) both Android (using fbsdk 9.0.0) and iOS (using fbsdk pod 9.0.1) worked

I think the AppDelegate.m change might be avoidable if there was a "swizzler" - like https://github.com/invertase/react-native-firebase/blob/master/packages/messaging/ios/RNFBMessaging/RNFBMessaging%2BAppDelegate.m#L46

@rnike
Copy link
Collaborator

rnike commented Mar 11, 2021

@mikehardy About initializing the sdk, do you know what's the difference between calling [FBSDKApplicationDelegate initializeSDK:nil]; and add codes below in didFinishLaunchingWithOptions as you mention

[[FBSDKApplicationDelegate sharedInstance] application:application
                           didFinishLaunchingWithOptions:launchOptions];

Following the facebook-ios-sdk v9.0.0 changeLog it actually tells us to call initializeSDK , I'm confused about it.

@mikehardy
Copy link
Collaborator

@rnike I would assume what you are doing is correct, and what I am doing is a gross hack in the spirit of "let's see this thing run" vs "I know what I'm doing" 😅 - the firebase sdk docs I linked do tell you to do what I did, but I do not know exactly what it's doing there vs the initializeSDK - I do know the docs mentioned that some work could be deferred etc., so perhaps the initializeSDK is the thing to do if deferring? I'm sorry I have nothing authoritative. The one solid fact I had is that a graph request crashed my app with an "SDK is not initialized yet" type error until I sliced that AppDelegate.m stanza in, but with the stanza it worked. I did not try the alternative of running initializeSDK

@thebergamo
Copy link
Owner Author

thebergamo commented Mar 11, 2021

I completely missed this conversation. :(
Now everything in the other PR makes much more sense hehe
I will try to make it run in my working app as well, as I hadn't much time today for it, but will integrate and check it following yours steps and probably update the docs towards this change as well. Now I'm thinking if we shouldn't remove this current version and release just a working one after @rnike PR is integrated in master.
Would like to know your thoughts.

@mikehardy
Copy link
Collaborator

@thebergamo - I dunno, it does work ? and it is clearly the first release - I'd leave it and just bump major ver after it is integrated, but every maintainer has their own tolerance for messiness. I tolerate a little :-), some tolerate zero, if you're doing the work any stance you take is valid of course :-)

@rnike
Copy link
Collaborator

rnike commented Mar 12, 2021

@mikehardy - I've run into the api Reference, at application:didFinishLaunchingWithOptions: it says As part of SDK initialization basic auto logging of app events will occur, as my understanding of the document, this is why it works in your circumstances.

I think the +initializeSDK: is a better option if we need a full initialization.

@mikehardy
Copy link
Collaborator

Yes, I agree with you

@iduuck
Copy link
Contributor

iduuck commented Mar 12, 2021

So, for me my app is stating me that I need to install the Facebook-iOS-SDK and not the separate ones. So this package still uses the old (?) packages?

Also, when installing this package and checking the "Event Debugger" no events are even showing. But also didn't show before.

@thebergamo
Copy link
Owner Author

@iduuck would you mind to have it as an issue instead of posting it in the merged PR?
Thanks for that :)

jimincutrell referenced this pull request in 6DegreeLabs/react-native-fbsdk-next Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants