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: wrap initialize sdk function #2

Merged
merged 3 commits into from
Mar 12, 2021

Conversation

rnike
Copy link
Collaborator

@rnike rnike commented Mar 11, 2021

From issue facebookarchive/react-native-fbsdk#840

I wrap the initialize function from native, the original pr is facebookarchive/react-native-fbsdk#841

@rnike rnike changed the title feature: wrap initialize sdk function feat: wrap initialize sdk function Mar 11, 2021
@thebergamo
Copy link
Owner

I looks fine, but I believe it would be good to have some note in the README.md about it that you should do it before start using it.

@thebergamo thebergamo changed the title feat: wrap initialize sdk function feat: wrap initialize sdk function1 Mar 11, 2021
@thebergamo thebergamo changed the title feat: wrap initialize sdk function1 feat: wrap initialize sdk function Mar 11, 2021
@mikehardy
Copy link
Collaborator

Yes! I believe this would stop the crash I saw and worked around by adding an AppDelegate chunk. +1 on a README note as otherwise I would have no idea, maybe even a major version bump

@rnike
Copy link
Collaborator Author

rnike commented Mar 11, 2021

README is added, any suggestions if it's not clear.

@rnike
Copy link
Collaborator Author

rnike commented Mar 11, 2021

@mikehardy I see, that's why you've added the code in didFinishLaunchingWithOptions.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@thebergamo
Copy link
Owner

Besides comments, looks like there is some error in iOS build.
@rnike Anyway, thanks for this PR, I totally forgot about it while just upgrading version.

@mikehardy I agree with you. this could be a major bump.

@rnike rnike force-pushed the feature/wrap-initialize-sdk branch from cdf41ff to 375e7e7 Compare March 12, 2021 05:57
@rnike rnike force-pushed the feature/wrap-initialize-sdk branch from 375e7e7 to 4622827 Compare March 12, 2021 05:58
@rnike
Copy link
Collaborator Author

rnike commented Mar 12, 2021

The AutoInitEnabled default is true if not set on Android. If following GDPR-style we will need to also wrap FacebookSdk.setAutoInitEnabled for Android (reference).

In this pr I will be focused on sdk initialization.

Copy link
Owner

@thebergamo thebergamo left a comment

Choose a reason for hiding this comment

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

I believe now everything looks good to me :)

@rnike rnike removed their assignment Mar 12, 2021
@rnike
Copy link
Collaborator Author

rnike commented Mar 12, 2021

just accidentally unassigned myself, don't mine it

@thebergamo
Copy link
Owner

Fixed :)

@thebergamo
Copy link
Owner

Let's just wait for @mikehardy feedback on it and I'm good to merge and release it.

README.md Outdated Show resolved Hide resolved
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.

I have no problems with the code (it's easy code thankfully!) but I made a suggestion to restructure the document with the idea it will nudge users to be platform-neutral and use the Javascript code. And I expanded the information about GDPR vs non-GDPR flow as I think that is the real decision point, with most having to handle GDPR style flows (voluntarily or by law) so I paid special attention to directing people how to do that so it is the same on both platforms using the platform-neutral javascript call

Words are easy to smush around though, do anything you like (or nothing at all) with the suggestion :-)

@thebergamo thebergamo merged commit a7160da into thebergamo:master Mar 12, 2021
github-actions bot pushed a commit that referenced this pull request Mar 12, 2021
# [4.0.0](v3.1.0...v4.0.0) (2021-03-12)

* Merge pull request #2 from rnike/feature/wrap-initialize-sdk ([a7160da](a7160da)), closes [#2](#2)

### Bug Fixes

* Use correct GITHUB_TOKEN ([041385c](041385c))

### BREAKING CHANGES

* Added the need to initialize the SDK on iOS on developer side.
@github-actions
Copy link

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@thebergamo
Copy link
Owner

As you see, release done :)

@thebergamo
Copy link
Owner

Thank you very much @rnike for your PR and the commitment into it that was wonderful and @mikehardy for reviewing and providing valuable feedback on it as well.

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 released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants