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(android, sdk12)!: update to facebook-android-sdk 12 #127

Merged
merged 11 commits into from
Nov 11, 2021

Conversation

mikehardy
Copy link
Collaborator

@mikehardy mikehardy commented Nov 9, 2021

This PR should be reviewed commit by commit and rebase-merged when ready. I'll keep the commits clean if they need changes, and re-push

Each commit has it's own message and is separate, for easy review

The big idea is:

  • "update all the things", that is: the github workflows, the android SDK, the example app, the javascript dependencies, and then prove android SDK 31 works

Fixes #115
Related #55 (takes care of the android part)
Fixes #102

Test Plan:

I cleanly installed and ran example app on android emulator and ios simulator repeatedly while working through the upgrades, everything worked

yarn was used everywhere else, and using `npm ci` required inclusion
of a package-lock.json which could have different dependencies than yarn.lock

this allows the use of one single yarn.lock, but the --frozen-lockfile argument
on yarn install in combination with purging node_modules emulates `npm ci`
behavior
this way we are in control of the version, npx may pull something in over
the network, which is unwanted in CI / release where we need total control
also improve caching of yarn items by adding a version key, in case
we need to forcibly remove caches in future
@mikehardy mikehardy force-pushed the @mikehardy/android-sdk-12 branch 2 times, most recently from 489cd7f to 8d820e9 Compare November 9, 2021 23:59
@mikehardy
Copy link
Collaborator Author

@thebergamo I'll let this sit for a day or two but it's all green in CI and passes locally so plan is to merge then release a breaking change release since an API was removed (both from iOS and Android SDKs, so removed here as well). It's a major advance for the library though to move forward on the underlying SDKs and update the example so I'm excited to get it out there

@mikehardy mikehardy added the pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. label Nov 10, 2021
@thebergamo
Copy link
Owner

I never seem so well documented PR!
Kudos ❤️
There is just a typo in the example besides it, feel free to move forward

RNFBSDKExample/README.md Outdated Show resolved Hide resolved
@hannojg
Copy link

hannojg commented Nov 10, 2021

(just wanted to add that we are using this PR also for our app and as far as I can tell everything works 👍 Thank you very much for the work put in here!)

@mikehardy
Copy link
Collaborator Author

Thanks very much for the success report! I'm just tidying up the example app generator right now but I can't see anything to change for the actual android forward port (it was really trivial anyway, thankfully) so when this actually merges+releases it won't really be a change for you to switch back to the release version

Cheers

…h-example.sh

This includes all the current configuration items noted in app startup logs / native
integration guides

this improves the developer experience, all things noted or needed while
I was working with the example to update the android SDK
BREAKING CHANGE: upstream SDKs removed updateUserProperties, there is no replacement
this proves Android targetSdk 31 works, when combined with facebook-android-sdk v12
note that we are not compatible with the new flow types, they get stricter
every time. maintenance effort will be spent on typescript, with apologies

feel free to post a PR that brings flow types up to v158 and re-enables
the check in the CI test workflow
@mikehardy mikehardy force-pushed the @mikehardy/android-sdk-12 branch 3 times, most recently from 250d55e to 235d19e Compare November 11, 2021 12:02
this will fix the build in CI for iOS at the same time it ensures
we run CI against the actual development code (vs github master, which
is what yarn installs by default)

pod install will fail on ubuntu (when building android) so allow it to fail
with the `|| true` bit, but we still install the dev code dependencies so
android tests actual dev code
@mikehardy
Copy link
Collaborator Author

Okay finally got this to go 100% green - nothing substantial in the module code has changed.
The main changes in the last day were 100% around getting the example app to configure itself more thoroughly (the underlying SDKs added a lot of configuration elements...) and getting the CI to more thoroughly exercise things while getting the developer experience / documentation lined up all good to go now - I need to release this so I can move on to some pressing work deadlines 🤞 - if something breaks I'll be listening though

@mikehardy mikehardy merged commit 63a6230 into thebergamo:master Nov 11, 2021
@mikehardy mikehardy deleted the @mikehardy/android-sdk-12 branch November 11, 2021 12:37
@mikehardy mikehardy removed the pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. label Nov 11, 2021
@mikehardy
Copy link
Collaborator Author

@thebergamo I think we're ready for a release here but I wanted to coordinate with you to see if you have any thoughts just in case. It should be a major version, and I think this commit message is exactly what is needed to have that work automatically:

cc19463

The ! is supposed to be what triggers it, and then the BREAKING CHANGE notes are supposed to be auto-populated in the changelog / release notes? That's how it is supposed to work, and it has worked for me here:

What do you think? Ready for the release button? :-)

@thebergamo
Copy link
Owner

I don't see any issues with it and I agree with you, it should be the fine as per the commit message.

Go for it tiger 🐯

@mikehardy
Copy link
Collaborator Author

Great 🚀 🤞

@github-actions
Copy link

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehardy
Copy link
Collaborator Author

Great CI setup, it all worked perfectly! https://github.com/thebergamo/react-native-fbsdk-next/releases/tag/v5.0.0
Developers will know exactly how to handle it if it affects them

@mikehardy mikehardy mentioned this pull request Nov 15, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from jcenter to maven CI is a little flaky, needs resiliency added to network activity
3 participants