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

refactor: extract logic by platforms using interface #345

Merged
merged 4 commits into from
Nov 26, 2022

Conversation

bang9
Copy link
Contributor

@bang9 bang9 commented Nov 4, 2022

  • 파일별로 플랫폼별 로직을 분리합니다.
  • 플랫폼별 메소드 인터페이스가 달라서 생기는 문제를, 임시로 오버로드를 통해서 분리하여 해결합니다.
  • 사용하는 프로젝트에서 확장자별 파일을 찾을 수 있도록 package.json 의 main 필드를 업데이트 합니다.

may resolve #337

@YangJonghun
Copy link
Contributor

YangJonghun commented Nov 6, 2022

시간내서 PR만들어주셔서 감사합니다! 바빠서 이슈만 만들어놓고 제가 고칠생각을 못했네요🥲
제가 사용해봤을 때 Issue #337 에서 제기됐던 문제는 해결 되었습니다! 정말 감사해요🙇‍♂️
작성해주신 코드를 살짝 리뷰해봤는데 아래와 같은 코드를 사용하고 계실 분들을 위해 src/index.ts에서 src/types/index.ts를 re-exports할 필요가 있을 것 같습니다.

import type { KakaoOAuthToken } from '@react-native-seoul/kakao-login'

@bang9
Copy link
Contributor Author

bang9 commented Nov 8, 2022

시간내서 PR만들어주셔서 감사합니다! 바빠서 이슈만 만들어놓고 제가 고칠생각을 못했네요🥲 제가 사용해봤을 때 Issue #337 에서 제기됐던 문제는 해결 되었습니다! 정말 감사해요🙇‍♂️ 작성해주신 코드를 살짝 리뷰해봤는데 아래와 같은 코드를 사용하고 계실 분들을 위해 src/index.ts에서 src/types/index.ts를 re-exports할 필요가 있을 것 같습니다.

import type { KakaoOAuthToken } from '@react-native-seoul/kakao-login'

리뷰해주셔서 감사합니다 :) re-exports 는 놓친 부분이었는데 감사합니다.

module suffixes 를 설정해보았는데요! 설정된 순서대로 컴파일 시점에 파일참조 우선순위를 정하는 옵션이다보니
에디터에서도 moduleSuffixes 에 설정된 순서로 참조할 파일이 결정되고, native/web 의 tsconfig 가 분리되지 않은 프로젝트일 경우
모듈의 한쪽 타입만 참조하게되는 문제가 생기는거 같습니다. (moduleSuffixes 에 web 을 우선순위 높게 설정하면, native 에서도 web 모듈 타입만을 참조)
언제 작업이 될지는 모르겠지만, 근본적인 문제인 인터페이스를 통일하는게 제일 좋은방법이지 않을까 합니다!

@bang9 bang9 self-assigned this Nov 8, 2022
@bang9 bang9 requested a review from hyochan November 8, 2022 02:49
@bang9 bang9 marked this pull request as ready for review November 8, 2022 02:49
@bang9 bang9 marked this pull request as draft November 8, 2022 02:50
@bang9 bang9 marked this pull request as ready for review November 8, 2022 03:37
@@ -2,7 +2,7 @@
"name": "@react-native-seoul/kakao-login",
"version": "5.2.4",
"description": "React Native Module for Kakao Login",
"main": "src/index.js",
"main": "src/index",
Copy link
Member

Choose a reason for hiding this comment

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

오우... 이전에 #325 에서 native.js파일을 분류하였다가 인식문제가 있어 다시 병합하였습니다.
이렇게 하면 파일을 분류해도 인식을 잘 하나 보네요.

감사합니다.

@nain93 시간 되실 때 내용 확인해주시면 좋을 것 같습니다.

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.

PR 좋아보입니다. 감사합니다 🙏

@hyochan hyochan merged commit c3d4e67 into crossplatformkorea:main Nov 26, 2022
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.

login함수 리턴타입 처리의 수정이 필요해보입니다
3 participants