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(CLNP-3879): support mmkv storage and deprecate an async storage #185

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bang9
Copy link
Collaborator

@bang9 bang9 commented Jun 21, 2024

External Contributions

This project is not yet set up to accept pull requests from external contributors.

If you have a pull request that you believe should be accepted, please contact
the Developer Relations team [email protected] with details
and we'll evaluate if we can setup a CLA to allow for the contribution.

For Internal Contributors

CLNP-3879

Description Of Changes

  • Support mmkv storage store

Types Of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply_

  • Bugfix
  • New feature
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance (ex) Prettier)
  • Build configuration
  • Improvement (refactor code)
  • Test

@bang9 bang9 self-assigned this Jun 21, 2024
@bang9 bang9 requested a review from OnestarLee June 21, 2024 04:05
@bang9 bang9 changed the title feat(CLNP-3879): support mmkv storage and deprecate a async storage feat(CLNP-3879): support mmkv storage and deprecate an async storage Jun 21, 2024
@OnestarLee
Copy link
Collaborator

아래와 같은 에러가 발생하면서 ios시뮬레이터에서 실행이 되지 않는데, 디버거 모드시 안된다는 내용같아서 릴리즈로 실행 했는데도? 안되고 있습니다.

Error: Failed to create a new MMKV instance: React Native is not running on-device. MMKV can only be used when synchronous method invocations (JSI) are possible. If you are using a remote debugger (e.g. Chrome), switch to an on-device debugger (e.g. Flipper) instead.

동일한 이슈인것 같은데 mmkv3 베타 버전에서 잘된다고하네요 확인이 필요할것 같습니다
mrousavy/react-native-mmkv#668

@bang9
Copy link
Collaborator Author

bang9 commented Jun 21, 2024

아래와 같은 에러가 발생하면서 ios시뮬레이터에서 실행이 되지 않는데, 디버거 모드시 안된다는 내용같아서 릴리즈로 실행 했는데도? 안되고 있습니다.

Error: Failed to create a new MMKV instance: React Native is not running on-device. MMKV can only be used when synchronous method invocations (JSI) are possible. If you are using a remote debugger (e.g. Chrome), switch to an on-device debugger (e.g. Flipper) instead.

동일한 이슈인것 같은데 mmkv3 베타 버전에서 잘된다고하네요 확인이 필요할것 같습니다 mrousavy/react-native-mmkv#668

엇.. Android, iOS 실행되는걸 모두 체크를 했는데, 혹시 pod install 하셨을까요?

@OnestarLee
Copy link
Collaborator

네 pod install 했는데 일단 저도 다시 확인해보겠습니다

@OnestarLee
Copy link
Collaborator

mmkv 사용시 remote debugging 기능이 불가능한게 맞네요
Android/iOS, Device/Simulate 둘다 디버깅 기능을 활성화 하면 error발생하면서 실행이 안되고, 디버깅 기능 비활성화 하면 문제 없이 실행되고 있습니다.

Flipper 를 사용하여 디버깅 하는것도 가능하다고는 하는데
remote debugging 을 사용하여 디버깅하는 고객들도 있을것 같아, 꼭 변경해야한다면 추가적인 안내를 해야할것 같습니다
image
image

chatSDK = SendbirdChat.init({
...chatInitParams,
appId,
newInstance: true,
modules: [new GroupChannelModule(), new OpenChannelModule()],
localCacheEnabled: true,
useAsyncStorageStore: internalStorage as never,
useMMKVStorageStore: isMMKVStorage ? (localCacheStorage as never) : undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 혹시 as never 로 사용하는 이유가 있을까요?
as MMKV 가 더 명확한것 같아서요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

타입이 맞지 않아서, 강제로 지정하기 위한 용도라서 딱히 없습니다! 변경하도록 하게씁니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bang9
Copy link
Collaborator Author

bang9 commented Jun 25, 2024

mmkv 사용시 remote debugging 기능이 불가능한게 맞네요 Android/iOS, Device/Simulate 둘다 디버깅 기능을 활성화 하면 error발생하면서 실행이 안되고, 디버깅 기능 비활성화 하면 문제 없이 실행되고 있습니다.

Flipper 를 사용하여 디버깅 하는것도 가능하다고는 하는데 remote debugging 을 사용하여 디버깅하는 고객들도 있을것 같아, 꼭 변경해야한다면 추가적인 안내를 해야할것 같습니다

아 remote debugging 을 키고 계셨군요, 릴리즈때 변경사항에 포함하는건 어떨까요?

@@ -0,0 +1,3 @@
import { MMKV } from 'react-native-mmkv';

export const mmkv = new MMKV();
Copy link
Collaborator

Choose a reason for hiding this comment

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

리모트 디버깅 사용시 chatOptions 에서 AsyncStorage 로 변경해도 MMKV 인스턴스가 생성되며 Excpetion이 발생하고 있습니다. 아래와 같이 변경하는건 어떨까요?

import { MMKV } from 'react-native-mmkv';

import { LocalCacheStorage } from '@sendbird/uikit-react-native';

class DevLocalCacheStorage {
  getString(_key: string): string | undefined {
    return undefined;
  }

  set(_key: string, _value: boolean | string | number | Uint8Array): void {}

  delete(_key: string): void {}
  getAllKeys(): string[] {
    return [];
  }
}

const createLocalCacheStorageInstance = (): LocalCacheStorage => {
  if (__DEV__) {
    return new DevLocalCacheStorage();
  }
  return new MMKV();
};

export const localCacheStorage = createLocalCacheStorageInstance();

@OnestarLee
Copy link
Collaborator

mmkv 사용시 remote debugging 기능이 불가능한게 맞네요 Android/iOS, Device/Simulate 둘다 디버깅 기능을 활성화 하면 error발생하면서 실행이 안되고, 디버깅 기능 비활성화 하면 문제 없이 실행되고 있습니다.
Flipper 를 사용하여 디버깅 하는것도 가능하다고는 하는데 remote debugging 을 사용하여 디버깅하는 고객들도 있을것 같아, 꼭 변경해야한다면 추가적인 안내를 해야할것 같습니다

아 remote debugging 을 키고 계셨군요, 릴리즈때 변경사항에 포함하는건 어떨까요?

네 포함 시키면 좋을것 같습니다. 다만 걱정되는건 remote debugging 사용하는 고객들이 mmkv 적용된 버전 사용하고나서 저희쪽으로 이슈 문의를 계속 하지않을까? 라는 걱정이 듭니다.
AsyncStorage depreacted 관련 히스토리를 찾아보면 react-native 기본 패키지에서 -> community 로 변경되었다가 지금은 @react-native-async-storage/async-storage 로 변경되면서 deprecated 되지 않고 지원을 계속 하는걸로 보이는데요
mmkv 성능상 유리하다고 어필하는건 보긴했는데, 이거 꼭 mmkv로 변경해야할까요?

@bang9
Copy link
Collaborator Author

bang9 commented Jun 26, 2024

네 포함 시키면 좋을것 같습니다. 다만 걱정되는건 remote debugging 사용하는 고객들이 mmkv 적용된 버전 사용하고나서 저희쪽으로 이슈 문의를 계속 하지않을까? 라는 걱정이 듭니다. AsyncStorage depreacted 관련 히스토리를 찾아보면 react-native 기본 패키지에서 -> community 로 변경되었다가 지금은 @react-native-async-storage/async-storage 로 변경되면서 deprecated 되지 않고 지원을 계속 하는걸로 보이는데요 mmkv 성능상 유리하다고 어필하는건 보긴했는데, 이거 꼭 mmkv로 변경해야할까요?

요게 AsyncStorage 의 블록 사이즈(6mb) 이슈 때문에 Android 에서 읽기에 실패하는 경우가 있어서 Chat SDK 에서 AsyncStorage 가 deprecated 하고 MMKV 로 변경을 한것이라서, 안따라가기에는 조금 애매할 것 같습니다. 고객한테도 계속 warning 이 뜰것이구요.
react-native 에서도 remote debugging 은 이제 사용을 지양하는 분위기여서, 아래 deprecation 링크를 함께 포함시키면 괜찮을 것 같습니다.
react-native-community/discussions-and-proposals#734

new debugger: react-native-community/discussions-and-proposals#733

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 58 lines in your changes missing coverage. Please review.

Project coverage is 13.01%. Comparing base (3836c10) to head (eb9c491).

Files Patch % Lines
...react-native/src/libs/InternalLocalCacheStorage.ts 0.00% 50 Missing ⚠️
...t-native/src/containers/SendbirdUIKitContainer.tsx 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
- Coverage   13.07%   13.01%   -0.07%     
==========================================
  Files         334      334              
  Lines        7434     7470      +36     
  Branches     1919     1942      +23     
==========================================
  Hits          972      972              
- Misses       6461     6497      +36     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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