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: enhanced iOS installation + update Examples #55

Merged
merged 14 commits into from
Oct 8, 2019
Merged

feat: enhanced iOS installation + update Examples #55

merged 14 commits into from
Oct 8, 2019

Conversation

bang9
Copy link
Contributor

@bang9 bang9 commented Aug 7, 2019

@hyochan 컨플릭트 때문에 다시 PR 생성해서 보냅니다. 감사합니다!

@bang9
Copy link
Contributor Author

bang9 commented Aug 7, 2019

개인적인 궁금증인데, podspec의 파일명과 내부 name을 맞춰서 root로 옮기는게 0.60 아래 버전에 어떤 영향을 미칠 수 있나요?

@bang9 bang9 changed the title chore : podspec info from package.json WIP: enhanced iOS installation Aug 22, 2019
@bang9
Copy link
Contributor Author

bang9 commented Aug 22, 2019

  • podspec 추가
  • KakaoOpenSDK Pods로 이동
  • Example 0.60.x 업데이트
  • Readme.md 업데이트

@sunkibaek
Copy link
Contributor

sunkibaek commented Aug 27, 2019

@bang9 혹시 이 PR은 어떻게 되어 가시나요~? 혹시 도와드릴 수 있는 부분 있으시면 알려주세요~

@bang9
Copy link
Contributor Author

bang9 commented Aug 28, 2019

@sunkibaek Example 업데이트 하고 빌드 수정중에 있습니다. 다른 일들때문에 아직 진행중이네요 ^^;

@hyochan
Copy link
Member

hyochan commented Aug 28, 2019

개인적인 궁금증인데, podspec의 파일명과 내부 name을 맞춰서 root로 옮기는게 0.60 아래 버전에 어떤 영향을 미칠 수 있나요?

안녕하세요 해당 이슈에 대응이 늦어 죄송합니다 ㅜ 저도 관련 이슈가 있어서 영향이 미칠까 했는데 언제부턴가 root로 옮겨야 되는 것 같더라고요.

Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

준비되시면 알려주세요~

@bang9
Copy link
Contributor Author

bang9 commented Aug 29, 2019

@hyochan 확인 한번 부탁드릴게요! :)

- docs: update ios document
@bang9 bang9 changed the title WIP: enhanced iOS installation feat: enhanced iOS installation + update Examples Aug 29, 2019
<Text>{`id : ${id}`}</Text>
<Text>{`email : ${email}`}</Text>
</View>
<View style={ styles.content}>
Copy link
Contributor

Choose a reason for hiding this comment

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

예제코드에서 컨벤션이 안맞는 부분이 있는거같은데 확인하시고 통일 해주실 수 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heyman333 eslint 적용했습니다!

Copy link
Contributor

@heyman333 heyman333 Sep 28, 2019

Choose a reason for hiding this comment

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

@bang9 감사합니다. 빠른 시간내에 테스트 해보고 이상없으면 머지 할 수 있도록 하겠습니다.
다만, 한가지 걸리는 부분이 현재 안드로이드 SDK version을 1.15.X로 쓰고 있는데
v1.23.0 (https://developers.kakao.com/docs/sdk) 까지 나와있는 상황에서 적용해서 배포 하는게 좋지 않을까 싶긴하네요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heyman333 실시간 프로필 때문에 어제 SDK가 배포가 되었었네요, 양쪽 플랫폼 둘 다 최신버전 SDK로 targeting 하도록 변경하겠습니다!

@hyochan
Copy link
Member

hyochan commented Sep 27, 2019

@heyman333 메인테이너분께서 자유롭게 판단해서 해주시면 됩니다~!

root: true,
extends: '@react-native-community',
Copy link
Member

Choose a reason for hiding this comment

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

There is other one @dooboo/eslint-config

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@heyman333
Copy link
Contributor

heyman333 commented Oct 3, 2019

@bang9
RN0.60 을 기준으로 모두 테스트 해봤을때 0.60이상 에서는 autolink도 모두 잘 되고 필요한 기능 대부분 동작하는 것 확인했습니다!

한가지 iOS에서 로그아웃 시 다음과 같은 에러가 발생하네요
sdk version을 올리면서 관련 API 사용법이 바뀐거 같긴한데 확인 부탁 드리겠습니다~

스크린샷 2019-10-03 오후 8 25 34

추가적으로 위에 설치 메뉴얼 관련해서 몇가지 코멘트를 달긴 했는데
결국 안드로이드 sdk version을 올리면서 android supportLib을 사용하고 있는 앱에서는 빌드가 안되는 문제가 있어서 0.60v 미만 사용자들을 위해 설치 문서를 따로 정리하고 Manul link방법도 업데이트 할 예정이니 너무 신경 안쓰셔도 될것 같습니다.

마지막으로 0.59에서 iOS cocoapod 설치시 다음과 같은 에러가 나는데 이것도 확인 가능할까요?ㅠㅠ

Installing KakaoOpenSDK (1.16.0)
Installing React (0.11.0)
Installing react-native-kakao-logins (1.3.7)
[!] The 'Pods-kakaosample22' target has transitive dependencies that include statically linked binaries: (/Users/zero/kakaosample22/ios/Pods/KakaoOpenSDK/KakaoCommon.framework, /Users/zero/kakaosample22/ios/Pods/KakaoOpenSDK/KakaoLink.framework, /Users/zero/kakaosample22/ios/Pods/KakaoOpenSDK/KakaoMessageTemplate.framework, /Users/zero/kakaosample22/ios/Pods/KakaoOpenSDK/KakaoNavi.framework, and /Users/zero/kakaosample22/ios/Pods/KakaoOpenSDK/KakaoOpenSDK.framework)

@bang9
Copy link
Contributor Author

bang9 commented Oct 4, 2019

@heyman333 리뷰 감사합니다!
리뷰 해주신 내용들 확인해서 수정하도록 하겠습니다 :)
수고 많으셨습니다.

@bang9
Copy link
Contributor Author

bang9 commented Oct 6, 2019

@heyman333 확인한번 부탁드릴게요! :)

  • iOS installation 문서 수정
  • iOS logout 수정
  • iOS transitive dependencies issue(use_framework! 사용시 발생) 수정

@heyman333
Copy link
Contributor

heyman333 commented Oct 7, 2019

@bang9
정말 꼼꼼하게 작업 해주셨네요 감사합니다.
그럼데 혹시 계정으로했을떄 로그아웃 되셨나요??

logoutAndCloseWithCompletionHandler의 첫번째 parameter(success)로 true값이 넘오긴 하는데
js파일 RNKakaoLogins.logout((err, result)의 첫번째 parameter(err)로 [Error: logged out]와 같이 Null값이 아니라 에러클래스 값이 넘어오네요

아래 예제 프로젝트 LOG를 보시면 이해 되실것 같습니다~

RCT_EXPORT_METHOD(logout:(RCTResponseSenderBlock)callback) {
    KOSession *session = [KOSession sharedSession];
    
    [session logoutAndCloseWithCompletionHandler:^(BOOL success, NSError *error) {
        if(success){
            NSLog(@"Log out success in objective-c");
            callback(@[[NSNull null], @"logged out"]);
        } else {
            RCTLogInfo(@"Error=%@", error);
            callback(@[@"Logout failed.\n", [NSNull null]]);
        }
    }];
}
const kakaoLogout = () => {
    logCallback('Logout Start', setLogoutLoading(true));

    RNKakaoLogins.logout((err, result) => {
      if (err) {
        console.log('logout in javascript error', err);
        return logCallback(
          `Logout Failed:${err.toString()}`,
          setLogoutLoading(false),
        );
      }
      setToken(TOKEN_EMPTY);
      setProfile(PROFILE_EMPTY);
      logCallback(`Logout Finished:${result}`, setLogoutLoading(false));
    });
  };
2019-10-08 02:18:07.705 [info][tid:com.facebook.react.JavaScript] Logout Start
2019-10-08 02:18:07.760737+0900 newkakao[7021:166969] Log out success in objective-c
2019-10-08 02:18:07.770 [info][tid:com.facebook.react.JavaScript] 'logout in javascript error', { [Error: logged out]
  line: 95834,
  column: 26,
  sourceURL: 'http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false' }
2019-10-08 02:18:07.775 [info][tid:com.facebook.react.JavaScript] Logout Failed:Error: logged out

Copy link
Contributor

@heyman333 heyman333 left a comment

Choose a reason for hiding this comment

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

추가적으로 이 예제코드도 수정 부탁 드리겠습니다!!

KakaoLoginExample/App.js Outdated Show resolved Hide resolved
@bang9
Copy link
Contributor Author

bang9 commented Oct 8, 2019

@heyman333 #57 로그아웃 문제는 해당 이슈와 관련된 것 같네요, 아무래도 변수 선언 없이 바로 스트링을 전달하면 에러로 떨어지는 그런 문제가 있는것 아닌가 의심이 됩니다.

RN iOS Native Module 문서를 보니 Callback쪽에는 Best Practice가 없어 more Expremiental 할 수 있다고 나와있는데 이참에 Promise 형태로 전환하는건 어떨까요?

다른 소셜 로그인들도 Promise 형태로 지원하고 있고, 저도 KakaoLogin 모듈을 쓸때는 다른 Promise들과 함께 처리하다 보니, 한번 프로미스로 래핑해서 쓰고있었거든요!

의견 부탁드릴게요 👍

@heyman333
Copy link
Contributor

heyman333 commented Oct 8, 2019

@bang9 아 네 그렇네요 그러고 보니 이 로그아웃 문제는 기존에도 가지고 있던 이슈였던 것 같습니다.
저희와 비슷한 라이브러리도 Promise로 형태로 하고 있고, 그게 더 안정적인 것 같습니다 😀😀😀

그렇다면 혹시 이부분도 작업 가능할까요??🙇‍♂️

@bang9
Copy link
Contributor Author

bang9 commented Oct 8, 2019

@heyman333 PR 이 길어지는 것 같아서 일단 콜백쪽 문제 수정후에, 따로 작업해서 PR 요청 드리겠습니다!

@heyman333
Copy link
Contributor

@bang9 감사합니다👍👍👍👍👍👍👍👍

@bang9
Copy link
Contributor Author

bang9 commented Oct 8, 2019

@heyman333 resolve #57 Native Module에서 반환된 result가 string 형태일경우 index.js의 JSON.parse 과정에서 에러를 발생시키는 문제였네요 일단 logout result에 null을 반환시키도록 수정했습니다.

이 문제를 해결하기 위해서 추후 PR에서
Native에서 바로 오브젝트 형식으로 넘어오도록 JSON 형태를 다음과 같이 변경하겠습니다.
Android JSON -> Map
iOS NSString -> NSDictionary

@heyman333
Copy link
Contributor

@bang9 아 그쪽에 문제가 있던 거였군요.. 감사합니다

@heyman333
Copy link
Contributor

heyman333 commented Oct 8, 2019

@bang9 다음 Promise로 작업 하실때 #57 #46 이슈 고려해서 작업 해주시면 감사하겠습니다~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants