[CLNP-5046] Migrate ThreadProvider to the new state management pattern #1250

git-babel · 2024-11-12T05:06:10Z

Overview

This PR migrate ThreadProvider and related files to the new state management pattern.

Changelog

ThreadProvider is migrated, and useThread() hook is introduced.
Removed ThreadDispatcher usages in ThreadProvider; it is replaced with the new state-action pattern of useThread().
PubSub of config still remains. It is out of scope of this PR.

Remaining tasks

Add unit tests and integration tests.

FurtherConcern

Handling hook
- The previous ThreadProvider contained several custom hooks. Those hooks retrieved state and actions through useThreadContext()
- Due to that, replacing useThreadContext() to new useThread() faced a problem. Those hooks �conatin useThread(), useThread() contains the hooks. So it makes cycle.
- For now, I moved all functionality of the hooks to the useThread(), but it looks wrong. Any good way to handle this?

netlify · 2024-11-12T05:06:30Z

✅ Deploy Preview for sendbird-uikit-react ready!

Name	Link
🔨 Latest commit	`63bc58a`
🔍 Latest deploy log	https://app.netlify.com/sites/sendbird-uikit-react/deploys/67482f606c3b2c0008057487
😎 Deploy Preview	https://deploy-preview-1250--sendbird-uikit-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/modules/Thread/components/ThreadMessageInput/index.tsx

chrisallo · 2024-11-14T19:29:05Z

src/modules/Thread/context/hooks/useGetChannel.ts

-            type: ThreadContextActionTypes.GET_CHANNEL_FAILURE,
-            payload: error,
-          });
+          getChannelFailure();


여기 실패한 에러를 같이 주는건 어떤가요?

사실 기존의 dispatcher에서도 payload로 넘어오는 error를 consume하는 곳이 없었습니다. 그래서 이번에 리팩토링을 하면서 사용되지 않는 변수를 남겨두기보단 일단 없애기로 결정했습니다.

chrisallo · 2024-11-14T19:29:24Z

src/modules/Thread/context/hooks/useGetParentMessage.ts

-            type: ThreadContextActionTypes.GET_PARENT_MESSAGE_FAILURE,
-            payload: error,
-          });
+          getParentMessageFailure();


여기도 에러 같이주면 좋을것 같습니다.

위와 같습니다.

chrisallo · 2024-11-14T19:39:06Z

src/modules/Thread/context/hooks/useGetChannel.ts

+          getChannelFailure();
        });
    }
  }, [message, sdkInit]);


여기 message가 아니라 channelUrl을 주는게 더 나을것같은데 어떻게 생각하시나요?

diff에는 보이지 않는데, 해당 파일의 50번 라인부터 아래의 주석이 달려있습니다.

/** * We don't use channelUrl here, * because Thread must operate independently of the channel. */

위의 주석 때문에 기존의 dependecy list를 바꾸지 않았습니다.

chrisallo · 2024-11-14T19:41:38Z

src/modules/Thread/context/hooks/useSendFileMessage.ts

+            url: URL.createObjectURL(file),
+            // pending thumbnail message seems to be failed
+            // @ts-ignore
+            requestState: 'pending',


제 기억으로는 requestState는 deprecated 되었고 sendingStatus가 대체한것같아서 확인 후 맞는걸 사용하도록 바꾸는게 좋지 않을까 생각해봤습니다.

수정했습니다.

chrisallo · 2024-11-14T19:41:53Z

src/modules/Thread/context/hooks/useSendVoiceMessageCallback.ts

+          url: URL.createObjectURL(file),
+          // pending thumbnail message seems to be failed
+          // @ts-ignore
+          requestState: 'pending',


여기두요!

수정했습니다.

chrisallo · 2024-11-14T19:44:57Z

src/modules/Thread/context/useThread.ts

+
+  const toggleReaction = useToggleReactionCallback({ currentChannel }, { logger });
+
+  const sendMessage = useCallback((props: SendMessageParams) => {


여기서 useSendMessageCallback 류의 hook들을 사용하지 않은 이유도 궁금합니다!

이거 관해서 저희가 내부적으로도 이야기를 한 적이 있습니다. 처음에는 가능한 기존에 사용하던 use~~ �훅을 여기서도 사용하려고 했는데 문제가 조금 있습니다.
일단 579 line의 useMemo()를 기준으로 useMemo() 위에서 �을 사용할 경우 use~~ 훅에서 useThread()를 사용하기 때문에 cycle이 생겨서 훅을 사용할 수 없습니다.
반대로 useMemo() 안에서 훅을 사용할 경우 useMemo 안에서 또다시 훅을 사용하는 케이스가 되므로 리액트 에러가 발생합니다.
이러한 문제들 때문에 기존의 use~~ 훅들의 content를 useThread() 내에 풀어서 옮겨놓았습니다.

요게 결국 유지보수 비용의 문제인데, 현재의 작은 비용을 아끼기 위해 미래의 큰 비용을 지불할 수 있는 상황으로 볼 수 있지 않을까 생각이 들었습니다. 보통 상호 의존성이 발생하는 경우, 예를 들어 A, B가 서로를 부르는 케이스에서 이렇게 코드의 중복을 통해 해결을 한다면 결국 같은 기능을 하는 2개의 코드가 존재하므로 나중에 휴먼 에러의 가능성이 높아진다고 생각합니다. 그래서 대안으로 중복으로 기능하는 부분을 독립적으로 A, B로부터 따로 분리하고 A, B에서 각각 이들을 사용하도록 디자인하는게 더 낫지 않나 생각해봤습니다. 어떻게 생각하시나요? @AhyoungRyu

이 케이스에서는 useSendXXXMessageCallback()을 수정해서, useThread()에서 sendMessageStart 등을 꺼내 사용하는게 아니라 hook의 props로 주입하도록 해서 해결하는 방법도 있을것 같습니다.

저도 Chris 의 의견에 동의합니다. 여기에 기존에 존재하는 useSendXXXMessageCallback 을 풀어서 사용하는 방식을 채택하고 싶다면 기존에 존재하는 hook 들이 없어져야 저희가 관리해야할 로직들이 줄어들거같아요. 근데 또 여기가 너무 방대해지는 문제가 발생할거고 두방식다 득보단 실이 더 많아보이네요 ㅠㅠ

circular dependency 문제가 발생하는것때문에 고민이라면 useSendXXXMessageCallback() 에서라도 useThread 대신 useThreadContext 를 사용하는것도 방법이 될 수 있어보입니다. circular dependency 가 발생하는건 막을 수 있을거같아요. 그럼

const toggleReaction = useToggleReactionCallback({ currentChannel }, { logger });

와 같은 방식으로 기존 useSendXXXMessageCallback hook 들도 모두 재사용 할 수 있을거같구요.

Chris께서 제안주신 모양으로 리팩토링 진행했습니다. 기존에는 useXXX() hook들의 로직을 전부 useThread() 내부로 옮긴 후 기존의 hook을 삭제하려 하였으나, 팀 내부에서 상의한 결과 말씀하신대로 기존 custom hook들의 파라미터로 action들을 넘겨주는 방식으로 결정되었습니다.
다만 이 경우 특정 custom hook에서 사용하려는 action이 아직 정의되지 않는 문제가 있을 수 있어 action에 들어갈 함수들을 에러가 나지 않도록 순서에 맞게 정의한 후 action 객체에 spread로 넣도록 하였습니다.

…-provider-migration

#1250) ### Overview This PR migrate ThreadProvider and related files to the new state management pattern. ### Changelog * `ThreadProvider` is migrated, and `useThread()` hook is introduced. * Removed `ThreadDispatcher` usages in ThreadProvider; it is replaced with the new state-action pattern of `useThread()`. * `PubSub` of `config` still remains. It is out of scope of this PR. ### Remaining tasks * Add unit tests and integration tests. ### FurtherConcern * Handling hook * The previous `ThreadProvider` contained several custom hooks. Those hooks retrieved state and actions through `useThreadContext()` * Due to that, replacing `useThreadContext()` to new `useThread()` faced a problem. Those hooks �conatin `useThread()`, `useThread()` contains the hooks. So it makes cycle. * For now, I moved all functionality of the hooks to the `useThread()`, but it looks wrong. Any good way to handle this?

git-babel added 2 commits November 12, 2024 11:20

Migrate ThreadProvider with the new convention

73299c8

Move hook functions into the useThread()

aae70ba

Add ts-ignore

b11d66c

git-babel requested review from AhyoungRyu and HoonBaek November 12, 2024 05:22

AhyoungRyu requested a review from chrisallo November 13, 2024 04:37

chrisallo reviewed Nov 14, 2024

View reviewed changes

AhyoungRyu assigned git-babel Nov 18, 2024

AhyoungRyu added the provider_migration label Nov 18, 2024

git-babel and others added 8 commits November 27, 2024 13:08

Apply the feedback and write basic unit test

493aaba

Change custom hook convention

c945c81

Fix

050d886

Fix manual test bugs

bba9a90

Merge branch 'feat/state-mgmt-migration-1' into feat/CLNP-5046/thread…

183f92e

…-provider-migration

Apply feedback and fix build error

3c10a05

Remove dux entirely

83ac4ea

Fix initial state

63bc58a

AhyoungRyu approved these changes Nov 28, 2024

View reviewed changes

AhyoungRyu merged commit 86958c7 into feat/state-mgmt-migration-1 Nov 28, 2024
10 checks passed

AhyoungRyu deleted the feat/CLNP-5046/thread-provider-migration branch November 28, 2024 12:48


		const toggleReaction = useToggleReactionCallback({ currentChannel }, { logger });

		const sendMessage = useCallback((props: SendMessageParams) => {

[CLNP-5046] Migrate ThreadProvider to the new state management pattern #1250

[CLNP-5046] Migrate ThreadProvider to the new state management pattern #1250

Uh oh!

Conversation

git-babel commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changelog

Remaining tasks

FurtherConcern

Uh oh!

netlify bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for sendbird-uikit-react ready!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisallo Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

git-babel commented Nov 12, 2024 •

edited

Loading

netlify bot commented Nov 12, 2024 •

edited

Loading

chrisallo Nov 20, 2024 •

edited

Loading