thebergamo / react-native-fbsdk-next

MIT License
635 stars 165 forks source link

feat: Limited login and Profile for IOS with type def #18

Closed cocoa-rum closed 3 years ago

cocoa-rum commented 3 years ago

PR for https://github.com/thebergamo/react-native-fbsdk-next/issues/7

Thanks @leolusoli for Profile part

PR contains Authentication Token along side with limited log in: If we use limited login, we lose access to access token, so we need an alternative and Authentication Token is. So from my view point Authentication Token complements limited log in

Also Profile part Related to the commnent

cocoa-rum commented 3 years ago

@bernard-kms I agree. I will improve what you said Speaking about "platform-specific behavior". Log in button contains nonce and loginTracking, which are specifice for IOS too, should I rename them nonceIOS and loginTrackingIOS as well ?

kmsbernard commented 3 years ago

Speaking about "platform-specific behavior". Log in button contains nonce and loginTracking, which are specifice for IOS too, should I rename them nonceIOS and loginTrackingIOS as well ?

I think that would be safer(maybe the safest) way to implement it. When limited login available in android (FB would implement it, wouldn't they?) we can add a new cross-platform one while gradually deprecating the platform-specific one and not making any breaking changes.

cocoa-rum commented 3 years ago

@bernard-kms @leolusoli I think we are ready for review

Also, what the best practice with package. I think, I am doing it a little bit complex way. It's my first time working with external package in such way I added forked repo ( yarn add "local repo path") Into my project. Do edits and paste changes into the package - commit

cocoa-rum commented 3 years ago

@thebergamo I got it. I will fix things that @glenna mentioned for the limited log in part

Also, just a small comment that on reviewing I did not understand why Profile and Authentication Token was added as part of the Limited Login functionality

As for Authentication Token - if we use limited login, we lose access to access token, so we need alternative and Authentication Token is. So from my view point Authentication Token complements limited log in

As for Profile, this commnent And it appeared in this PR. May be we should ask @leolusoli to create separate PR with Profile part? and from this PR we will remove it

glenna commented 3 years ago

@cocoa-rum thank you! yeah, I got the context when I read the issue, but as a reviewer I don't expect to need to read all comments on an issue thread to get the context ;). Just a future reference to add more information to the PR description.

Regarding moving Profile impl to a separate PR, I would leave that up to @thebergamo. (If it were me, I would leave it in, as now it's reviewed and ready to roll -- but update the description so that in the release notes for the version when the PR is linked there's easy to access information)

thebergamo commented 3 years ago

Regarding Profile impl, I believe it's pretty ok having it here, so no biggie from my side. You could just give more context as mentioned by @glenna in the PR description, so it would be much easier to track in the changelog later on :)

leolusoli commented 3 years ago

@glenna @thebergamo I'll explain you tomorrow with some examples of what happening for Profile imageURL field, because there's some particular situation regarding limited/not limited login (i.e. different value returned). I'll also fix the typos in comments and style some code according to the guidelines described in the comments. I think tomorrow will be all pushed and explained :)

Thanks for your time again.

cocoa-rum commented 3 years ago

@thebergamo looks like we fixed all remarks for now You are welcome to review or make some comments for improving PR

glenna commented 3 years ago

@thebergamo did you also want the readme section before merging? If so, I think that's the last thing we are waiting on.

thebergamo commented 3 years ago

Yes, because without that could lead others to some mistakes

glenna commented 3 years ago

@thebergamo agreed! Just wanted to make it clear :)

cocoa-rum commented 3 years ago

Yes, because without that could lead others to some mistakes

Should I and @leolusoli add changes to readme? Or what is next step?

thebergamo commented 3 years ago

Yes, you should.

kmsbernard commented 3 years ago

LGTM, and yes, a proper documentation is important to complete this work to be released.

cocoa-rum commented 3 years ago

Readme is ready. Is it correct?

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 4.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

thebergamo commented 3 years ago

Version released :) v4.2.0

Thank y'all for the effort for make it happen :)