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: Add reauthorizeDataAccess method to LoginManager #204

Merged
merged 3 commits into from
Feb 21, 2022
Merged

feat: Add reauthorizeDataAccess method to LoginManager #204

merged 3 commits into from
Feb 21, 2022

Conversation

neilco
Copy link
Contributor

@neilco neilco commented Feb 21, 2022

This pull request replicates the functionality added in facebookarchive/react-native-fbsdk#720 in order to facilitate the requesting of data access permissions when they have expired. The native Facebook SDK exposes a method on the LoginManager class (reauthorizeDataAccess) that the changes in this pull request exposes to the JS layer.

Test Plan:

We've modified the example app to call the new method. We've also tested this with a separate app to verify the code is behaving as expected.

@neilco neilco changed the title Add reauthorizeDataAccess method to LoginManager feat: Add reauthorizeDataAccess method to LoginManager Feb 21, 2022
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.

Looks generally really good, I'll kick off CI, but I need to understand RCT_REMAP_METHOD prior to merge

@mikehardy mikehardy added the needs more info waiting on original poster to provide details label Feb 21, 2022
@mikehardy
Copy link
Collaborator

[warn] Code style issues found in the above file(s). Forgot to run Prettier?

@neilco
Copy link
Contributor Author

neilco commented Feb 21, 2022

[warn] Code style issues found in the above file(s). Forgot to run Prettier?

You've got to love it when something tells you there's an issue, but no what the actual issue is. Maybe you need to tweak your prettier config?

It's fixed in 8b8e9fc.

@mikehardy
Copy link
Collaborator

[warn] Code style issues found in the above file(s). Forgot to run Prettier?

You've got to love it when something tells you there's an issue, but no what the actual issue is. Maybe you need to tweak your prettier config?

It's fixed in 8b8e9fc.

I think local prettier integrations (like the one in VScode at least, which you indicate you are using?) do look at the local .prettierrc and do tell you - either way because of the way prettier is, you just need to run it and that's that - but this is something that would be best in a pre-commit hook yeah - no reason to wait till it hits CI to see it

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.

looks really good now, happy to approve + merge + release in just a moment, and though it doesn't matter now it appears that the alternative RCT_NNNNN method was just in case there was a symbol collision exporting the method? Interesting - makes sense I guess, just never seen it needed in any of the other repos I've worked on. Learn something every day...

@mikehardy mikehardy added pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. and removed needs more info waiting on original poster to provide details labels Feb 21, 2022
@mikehardy mikehardy merged commit 4aa7ff6 into thebergamo:master Feb 21, 2022
@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 Feb 21, 2022
github-actions bot pushed a commit that referenced this pull request Feb 21, 2022
## [7.2.0](v7.1.0...v7.2.0) (2022-02-21)

### Features

* Add reauthorizeDataAccess method to LoginManager ([#204](#204)) ([4aa7ff6](4aa7ff6))
@github-actions
Copy link

🎉 This PR is included in version 7.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants