[CLNP-5046] Migrate ThreadProvider to the new state management pattern #1250
Conversation
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| type: ThreadContextActionTypes.GET_CHANNEL_FAILURE, | ||
| payload: error, | ||
| }); | ||
| getChannelFailure(); |
There was a problem hiding this comment.
여기 실패한 에러를 같이 주는건 어떤가요?
| type: ThreadContextActionTypes.GET_PARENT_MESSAGE_FAILURE, | ||
| payload: error, | ||
| }); | ||
| getParentMessageFailure(); |
| type: ThreadContextActionTypes.GET_CHANNEL_FAILURE, | ||
| payload: error, | ||
| }); | ||
| getChannelFailure(); | ||
| }); | ||
| } | ||
| }, [message, sdkInit]); |
There was a problem hiding this comment.
여기 message가 아니라 channelUrl을 주는게 더 나을것같은데 어떻게 생각하시나요?
| url: URL.createObjectURL(file), | ||
| // pending thumbnail message seems to be failed | ||
| // @ts-ignore | ||
| requestState: 'pending', |
There was a problem hiding this comment.
제 기억으로는 requestState는 deprecated 되었고 sendingStatus가 대체한것같아서 확인 후 맞는걸 사용하도록 바꾸는게 좋지 않을까 생각해봤습니다.
| url: URL.createObjectURL(file), | ||
| // pending thumbnail message seems to be failed | ||
| // @ts-ignore | ||
| requestState: 'pending', |
|
|
||
| const toggleReaction = useToggleReactionCallback({ currentChannel }, { logger }); | ||
|
|
||
| const sendMessage = useCallback((props: SendMessageParams) => { |
There was a problem hiding this comment.
여기서 useSendMessageCallback 류의 hook들을 사용하지 않은 이유도 궁금합니다!
There was a problem hiding this comment.
이거 관해서 저희가 내부적으로도 이야기를 한 적이 있습니다. 처음에는 가능한 기존에 사용하던 use~~ �훅을 여기서도 사용하려고 했는데 문제가 조금 있습니다.
일단 579 line의 useMemo()를 기준으로 useMemo() 위에서 �을 사용할 경우 use~~ 훅에서 useThread()를 사용하기 때문에 cycle이 생겨서 훅을 사용할 수 없습니다.
반대로 useMemo() 안에서 훅을 사용할 경우 useMemo 안에서 또다시 훅을 사용하는 케이스가 되므로 리액트 에러가 발생합니다.
이러한 문제들 때문에 기존의 use~~ 훅들의 content를 useThread() 내에 풀어서 옮겨놓았습니다.
There was a problem hiding this comment.
요게 결국 유지보수 비용의 문제인데, 현재의 작은 비용을 아끼기 위해 미래의 큰 비용을 지불할 수 있는 상황으로 볼 수 있지 않을까 생각이 들었습니다. 보통 상호 의존성이 발생하는 경우, 예를 들어 A, B가 서로를 부르는 케이스에서 이렇게 코드의 중복을 통해 해결을 한다면 결국 같은 기능을 하는 2개의 코드가 존재하므로 나중에 휴먼 에러의 가능성이 높아진다고 생각합니다. 그래서 대안으로 중복으로 기능하는 부분을 독립적으로 A, B로부터 따로 분리하고 A, B에서 각각 이들을 사용하도록 디자인하는게 더 낫지 않나 생각해봤습니다. 어떻게 생각하시나요? @AhyoungRyu
There was a problem hiding this comment.
이 케이스에서는 useSendXXXMessageCallback()을 수정해서, useThread()에서 sendMessageStart 등을 꺼내 사용하는게 아니라 hook의 props로 주입하도록 해서 해결하는 방법도 있을것 같습니다.
There was a problem hiding this comment.
저도 Chris 의 의견에 동의합니다. 여기에 기존에 존재하는 useSendXXXMessageCallback 을 풀어서 사용하는 방식을 채택하고 싶다면 기존에 존재하는 hook 들이 없어져야 저희가 관리해야할 로직들이 줄어들거같아요. 근데 또 여기가 너무 방대해지는 문제가 발생할거고 두방식다 득보단 실이 더 많아보이네요 ㅠㅠ
circular dependency 문제가 발생하는것때문에 고민이라면 useSendXXXMessageCallback() 에서라도 useThread 대신 useThreadContext 를 사용하는것도 방법이 될 수 있어보입니다. circular dependency 가 발생하는건 막을 수 있을거같아요. 그럼
const toggleReaction = useToggleReactionCallback({ currentChannel }, { logger });
와 같은 방식으로 기존 useSendXXXMessageCallback hook 들도 모두 재사용 할 수 있을거같구요.
Overview
This PR migrate ThreadProvider and related files to the new state management pattern.
Changelog
ThreadProvideris migrated, anduseThread()hook is introduced.ThreadDispatcherusages in ThreadProvider; it is replaced with the new state-action pattern ofuseThread().PubSubofconfigstill remains. It is out of scope of this PR.Remaining tasks
FurtherConcern
ThreadProvidercontained several custom hooks. Those hooks retrieved state and actions throughuseThreadContext()useThreadContext()to newuseThread()faced a problem. Those hooks �conatinuseThread(),useThread()contains the hooks. So it makes cycle.useThread(), but it looks wrong. Any good way to handle this?