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

CRNS-264: Fixing issues around network recovery #544

Merged
merged 19 commits into from
Mar 21, 2021

Conversation

vishalnarkhede
Copy link
Contributor

@vishalnarkhede vishalnarkhede commented Mar 6, 2021

Submit a pull request

Dependent on #626

CLA

  • I have signed the Stream CLA (required).
  • The code changes follow best practices
  • Code changes are tested (add some information if not applicable)

Description of the pull request

@vishalnarkhede vishalnarkhede changed the title Fixing issues around network recovery CRNS-264: Fixing issues around network recovery Mar 6, 2021
@@ -197,6 +197,10 @@ export type ChannelPreviewMessengerProps<
'channel' | 'latestMessagePreview'
>;

export const MemoizedChannelPreviewMessengerWithContext = React.memo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saves some re-renders!!

Comment on lines 140 to 151
const rightContent = useMemo(() => {
return RightContent &&
<View style={styles.rightContainer}>
<RightContent />
</View>
}, []);

const leftContent = useMemo(() => {
return LeftContent &&
<LeftContent />
}, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes the image flickering issue on sample app!!

Copy link
Contributor

@nhannah nhannah left a comment

Choose a reason for hiding this comment

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

Mostly minor comments, but one def needs changing

};
}, []);

const handleAppStateChange = async (nextAppState: AppStateStatus) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be an issue; if you get a phone call lets say, the app state will go from active -> inactive (no connection closed), then you end call and in app, app state will go from inactive -> active (new connection), now 2x connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now handled on js client level :)

Copy link
Contributor

Choose a reason for hiding this comment

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

we should still get the logic right tho? I.e. if we only close when going to background only open when coming from background??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If app is in inactive state for too long, websocket may still be dropped. So I think it's just better to call openConnection anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually curious if that happens, do you know?

@vishalnarkhede vishalnarkhede force-pushed the vishal/network-recovery-fix branch from 1a71ed9 to 02a9926 Compare March 12, 2021 13:36
@vishalnarkhede
Copy link
Contributor Author

vishalnarkhede commented Mar 12, 2021

@nhannah Updated PR with few more changes.

  • Added prop closeConnectionOnbackground (boolean), to be able to disable AppState listener
  • Moved function-props on FlatList to separate functions, instead defining them inline.
  • The things that discussed about multiple calls to client.openConnection - we have not handled it on js client level. So all good there.
  • I have removed those image flickering related changes from this PR. I will handle them separately.
  • Took loading indicator out of FlatList, it causes some issues with flatlist scroll.

@vishalnarkhede vishalnarkhede force-pushed the vishal/network-recovery-fix branch from 42ea831 to d1c630a Compare March 12, 2021 13:46
Copy link
Contributor

@nhannah nhannah left a comment

Choose a reason for hiding this comment

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

Just had a few comments that should be easy to answer, if it's all working think were good to go though.

<HeaderComponent loadingMore={onStartReachedInProgress} />
);

const renderEmptyStateIndicator = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused with the change here of moving the loading state outside the flat list? If I recall correctly the main reason to have loading in the empty state is performance related to rendering the entire list possibly repeatedly.

Copy link
Contributor Author

@vishalnarkhede vishalnarkhede Mar 13, 2021

Choose a reason for hiding this comment

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

So rendering empty state is still in flatlist, I have just separated out inline fuction. Inline functions can cause more re-renders. React documentation only gives headsup about this for ref callback function (inline vs separate function), it doesn't say any such thing about indicators. But from the discussions I can see, it's still better not have inline functions in FlatList.

For loading indicator, I had to take it out of FlatList because

  • when you are in network recovery state, we show loading indicator.
  • after recovery, when new results are ready, scroll doesn't start at bottom of the list if loading indicator is part of FlatList (within ListEmptyComponent).

And this seemed like a confusing UX to me.

@vishalnarkhede vishalnarkhede requested a review from nhannah March 15, 2021 15:35
…ream-chat-react-native into vishal/network-recovery-fix
…e into vishal/network-recovery-fix

# Conflicts:
#	examples/TypeScriptMessaging/yarn.lock
#	src/components/MessageList/MessageList.tsx
@vishalnarkhede vishalnarkhede merged commit ca6f167 into master Mar 21, 2021
@delete-merged-branch delete-merged-branch bot deleted the vishal/network-recovery-fix branch March 21, 2021 20:44
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.

2 participants