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.

Sign up for GitHub

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

CRS-292 Prevent state updates on unmounted Channel component #566

Merged
merged 3 commits into from
Oct 14, 2020
Merged

CRS-292 Prevent state updates on unmounted Channel component #566

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79dffae
Add ref to keep track of whether component is mounted. Do not trigger…
Oct 9, 2020
324e685
Separate logic into hook and add test
Oct 12, 2020
88eca03
Merge branch 'master' into CRS-292-Channel-unmounted-dispatch
Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/Channel/Channel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ const ChannelInner = ({
const lastRead = useRef(new Date());
const chatContext = useContext(ChatContext);
const online = useRef(true);
const isMounted = useRef(true);
Copy link
Contributor

@vishalnarkhede vishalnarkhede Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have seen this weird issue with our previous class based components as well. LGTM

@ath92 Maybe can we extract it to separate hook?

const isMounted = useIsMounted();

Copy link
Author

Choose a reason for hiding this comment

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

I suppose this hook would basically be a combination of useRef for storing the flag + useEffect for setting it to true/false?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

Copy link
Author

Choose a reason for hiding this comment

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

Just did this, also allowed me to write a test quite easily :)

const { t } = useContext(TranslationContext);

// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -241,7 +242,9 @@ const ChannelInner = ({
channel.on(handleEvent);
}
})();
isMounted.current = true;
return () => {
isMounted.current = false;
if (errored || !done) return;
document.removeEventListener('visibilitychange', onVisibilityChange);
channel.off(handleEvent);
Expand Down Expand Up @@ -270,6 +273,7 @@ const ChannelInner = ({
* @param {import('stream-chat').ChannelState['messages']} messages
*/
(hasMore, messages) => {
if (!isMounted.current) return;
dispatch({ type: 'loadMoreFinished', hasMore, messages });
},
2000,
Expand Down