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

fix(expo): update scheme injection #504

Conversation

andrejpavlovic
Copy link
Contributor

Currently expo plugin injects scheme into all intent filters causing auto verify to break for deep links. See #329 issue for more info.

The fix is to follow latest instructions for configuring Facebook Login for Android essentially removing existing injection of scheme into intent filters and adding <activity android:name="com.facebook.FacebookActivity" .. />

Test Plan:
Updated unit test to check that new <activity android:name="com.facebook.FacebookActivity" .. /> has been added to the manifest.

Fixes #329

What this really does is remove support for Android WebViews as per Deprecating Facebook Login support on Android WebViews which should be fine since we are well beyond Facebook SDK 8.2.0 at this point.

@andrejpavlovic andrejpavlovic marked this pull request as draft March 13, 2024 23:12
@andrejpavlovic andrejpavlovic changed the title fix - expo plugin - update scheme injection fix(expo): update scheme injection Mar 13, 2024
@andrejpavlovic andrejpavlovic marked this pull request as ready for review March 14, 2024 03:16
@JuanRdBO
Copy link

@mikehardy please merge 🙏

@andrejpavlovic
Copy link
Contributor Author

I haven't tested this with a fresh expo project and I'm not sure if custom tabs need extra configuration as per https://developer.chrome.com/docs/android/custom-tabs/guide-get-started . It's working for me in an existing project just fine though.

@JuanRdBO
Copy link

I haven't tested this with a fresh expo project and I'm not sure if custom tabs need extra configuration as per https://developer.chrome.com/docs/android/custom-tabs/guide-get-started . It's working for me in an existing project just fine though.

works for me too

@andrejpavlovic
Copy link
Contributor Author

I tried it with a new app and it's working fine there as well - so it should be fine to merge this at some point.

@mikehardy
Copy link
Collaborator

Hey @andrejpavlovic 👋 - thanks for your patience

This looks close to working but I was unable to push to your branch (looks like you didn't grant maintainers rights to push?) so I can't fix the trivial problems

Two things:

  • you need to either rebase upstream/master into your PR and repush it or merge upstream/master in to get CI changes which will clear the iOS build issue
  • you need to run yarn validate:prettier:fix and commit the result for it to pass formatting checks

With those two items done it should clear CI then I can merge this - both are trivial / mechanical - the PR itself looks fine and since I don't use Expo myself I'll trust you - if you test it and it works, that works for me

Thanks!

@mikehardy mikehardy added question Further information is requested pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. labels Apr 10, 2024
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.

almost good to go - just needs changes as described

@mikehardy
Copy link
Collaborator

Hope you don't mind - I didn't want to delay further since I'd prefer to get all the work done in this repo in a single batch then let it sit again for a long while - so I opened #512 that contains this as well as the rebase + format fix I mentioned

Should do the trick!

Copy link

🎉 This issue has been resolved in version 12.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@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 Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expo plugin breaks auto-verification of Android deep linking
3 participants