CRS-384 Upgrade react-virtuoso to v1 #694

ath92 · 2021-02-05T10:37:41Z

Submit a pull request

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

Upgrades react-virtuoso to the latest version available. A refactor of VirtualizedMessageList was required to adapt to the breaking changes introduced with this new version. In particular, some extra work was needed to handle the new way virtuoso deals with prepending messages.

Improvements:

VirtualMessageList works well with Message and FixedHeightMessage
Margins in the item content are handled
New reactions keep the list at the bottom (if already there)

Not included:

Images with no specified sizing displace the timeline. The same happens in non-virtualized mode.

Squash the commits before merging, a lot of back and forth.

DanC5 · 2021-02-05T14:34:58Z

src/components/MessageList/VirtualizedMessageList.js

          }
        />
      );
    },
-    [MessageDeleted, customMessageRenderer, shouldGroupByUser],
+    [
+      MessageDeleted,


can probably remove MessageDeleted from dep array, it shouldn't change

Not sure if I'd remove this. Even if it shouldn't change this, it technically could, and it doesn't really hurt to have it in the dependencies array.

I think this raises a larger question of how we handle dependency arrays. On the react native side we only include the data that should actually trigger a new render during an app session, so we don't include things like prop UI components that don't change mid-session. Obviously native is a little different, but we definitely saw performance improvements and decreased re-renders upon getting stricter with the dependency arrays. @mahboubii do you have thoughts on whether or not to keep UI components in dependency arrays?

There could be customers who change the UI props conditionally in their app but the performance gain from removing this dependency check is worth it IMO and we should move toward that path.
In the meantime since changing these effect dependencies are potentially a breaking change, let's leave it as is until the next major release.
we should make it clear for customers that UI prop overrides should not change after the first render of the app

DanC5 · 2021-02-05T14:35:14Z

src/components/MessageList/VirtualizedMessageList.js

+      Footer,
+    };
+  }, [
+    EmptyStateIndicator,


same with EmptyState and Typing

DanC5 · 2021-02-05T14:40:00Z

@ath92 I'll admit I don't have much background on this feature but it looks pretty good, will wait for Amin to check it out too. Though in the meantime can you replace @ts-ignore with @ts-expect-error? This will help us when we get to converting this file and will notify you if there's not a type error to follow.

src/components/MessageList/VirtualizedMessageList.js

mahboubii · 2021-02-08T07:05:44Z

looks like the new scrollTo function also accepts smooth behavior

codecov-io · 2021-02-08T16:51:25Z

Codecov Report

Merging #694 (7d79567) into master (d936e30) will decrease coverage by 0.07%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
- Coverage   85.87%   85.79%   -0.08%     
==========================================
  Files         154      155       +1     
  Lines        3108     3140      +32     
  Branches      746      754       +8     
==========================================
+ Hits         2669     2694      +25     
- Misses        327      332       +5     
- Partials      112      114       +2

Impacted Files	Coverage Δ
...c/components/MessageList/VirtualizedMessageList.js	`50.00% <48.64%> (+3.06%)`	⬆️
.../components/MessageList/usePrependMessagesCount.js	`89.47% <89.47%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d936e30...7d79567. Read the comment docs.

github-actions · 2021-02-09T11:01:59Z

Size Change: +36 kB (2%)

Total Size: 1.54 MB

Filename	Size	Change
`./dist/browser.full-bundle.js`	736 kB	+20.8 kB (2%)
`./dist/browser.full-bundle.min.js`	408 kB	+13.4 kB (3%)
`./dist/css/index.css`	28.6 kB	+7 B (0%)
`./dist/css/index.min.css`	23.8 kB	+7 B (0%)
`./dist/index.es.js`	164 kB	+882 B (0%)
`./dist/index.js`	166 kB	+896 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`./dist/css/index.js`	21 B	0 B
`./dist/i18n/en.json`	902 B	0 B
`./dist/i18n/fr.json`	1.38 kB	0 B
`./dist/i18n/hi.json`	1.56 kB	0 B
`./dist/i18n/it.json`	1.33 kB	0 B
`./dist/i18n/nl.json`	1.31 kB	0 B
`./dist/i18n/ru.json`	1.62 kB	0 B
`./dist/i18n/tr.json`	1.32 kB	0 B

_{compressed-size-action}

ath92 · 2021-02-09T11:12:21Z

I think I've addressed the comments in this review so far, but I'm running into an issue: sometimes the startReached callback is called twice (one after the other, so after the first promise resolves, the function is called again to load more items). This causes the list to jump to the latest set of items that has been loaded (because firstItemIndex is changed when the last set of messages is loaded).

I'm not sure why this happens - as far as I can tell we're not doing anything different from the examples provided on the Virtuoso website. Perhaps @petyosi has some idea on why this could be happening? (let me know if it makes more sense to try and create a more minimal example of the problem and open an issue in the virtuoso repo instead)

petyosi · 2021-02-09T11:15:47Z

I think I've addressed the comments in this review so far, but I'm running into an issue: sometimes the startReached callback is called twice (one after the other, so after the first promise resolves, the function is called again to load more items). This causes the list to jump to the latest set of items that has been loaded (because firstItemIndex is changed when the last set of messages is loaded).

I'm not sure why this happens - as far as I can tell we're not doing anything different from the examples provided on the Virtuoso website. Perhaps @petyosi has some idea on why this could be happening? (let me know if it makes more sense to try and create a more minimal example of the problem and open an issue in the virtuoso repo instead)

I have been struggling with this problem in this issue: petyosi/react-virtuoso#281 - check if debouncing the callback handle won't resolve it. If not, a repro will help me. Notice that the OS matters - if you reproduce it on Mac, then you are probably facing something different.

ath92 · 2021-02-12T16:16:37Z

I have been struggling with this problem in this issue: petyosi/react-virtuoso#281 - check if debouncing the callback handle won't resolve it. If not, a repro will help me. Notice that the OS matters - if you reproduce it on Mac, then you are probably facing something different.

Got some time to look into this again: The issue I was seeing was unrelated to the issue you mentioned here (and unrelated to virtuoso), but caused by how we handle prepending items to our channel state.

In short: the promise that is returned by loadMore contains the amount of new messages, but those new messages aren't added to the channel state at exactly the moment the promise resolves: the actual state update is performed by a debounced loadMoreFinished function in the Channel component. In some cases, this caused the list to be re-rendered with new messages, while firstItemIndex was not yet updated, or the other way around (firstItemIndex already updated, but no new messages yet).

I pushed a workaround for this that doesn't depend on the timing of the loadMore promise resolving, but I'm not super happy with it because it's kinda complex. I don't immediately see an easier way to fix it though, but open to suggestions.

petyosi · 2021-02-23T08:41:56Z

@mahboubii @DanC5 @virdesai please take a look

src/components/MessageList/hooks/usePrependMessagesCount.js

src/components/MessageList/VirtualizedMessageList.js

petyosi · 2021-02-24T07:46:27Z

@DanC5 let's give this a review when possible

…new version

examples/messaging/src/App.js

kainosnoema · 2021-03-15T16:45:24Z

src/components/MessageList/VirtualizedMessageList.js

  scrollSeekPlaceHolder,
-  Message = FixedHeightMessage,
+  Message = DefaultMessage,


Seeing that a fixed height message might not be required anymore, is it possible to use the MessageTeam component in the virtualized list now?

Looks like this Message property requires a type with FixedHeightMessageProps still, so not sure how to do it...

@petyosi can you speak to the feasibility of using our other Message UI components in the VL now?

@kainosnoema indeed, using "any" messages in the virtual list is something which we are looking into, but not yet ready to introduce as the default configuration. The part which does not work as smoothly as we wish is messages which contain img elements without a specified size. Such posts may cause the virtual list to "jump" when the image is loaded.

This being said, you can ignore the typing for now and try passing the plain message - we would love to get your feedback on the performance of that.

@petyosi thanks for the update. We're building our own custom message component and will be having photo galleries with a fixed height (when attachments are present), so don't think we'll run into the jumping issue. We'll give this a shot!

ath92 requested review from mahboubii and DanC5 February 5, 2021 13:41

DanC5 reviewed Feb 5, 2021

View reviewed changes

src/components/MessageList/VirtualizedMessageList.js Outdated Show resolved Hide resolved

mahboubii reviewed Feb 8, 2021

View reviewed changes

src/components/MessageList/VirtualizedMessageList.js Show resolved Hide resolved

src/components/MessageList/VirtualizedMessageList.js Outdated Show resolved Hide resolved

src/components/MessageList/VirtualizedMessageList.js Outdated Show resolved Hide resolved

ath92 force-pushed the upgrade-virtuoso branch from 0df4861 to 6d04f8c Compare February 12, 2021 16:09

petyosi marked this pull request as draft February 18, 2021 16:46

petyosi force-pushed the upgrade-virtuoso branch from 6d04f8c to 6ecde4b Compare February 22, 2021 08:09

petyosi changed the title ~~CRS-384 Upgrade react-virtuoso~~ CRS-384 Upgrade react-virtuoso to v1 Feb 22, 2021

petyosi requested review from mahboubii, virdesai and DanC5 February 22, 2021 13:59

petyosi self-assigned this Feb 22, 2021

petyosi force-pushed the upgrade-virtuoso branch from 3ec1946 to b7aaee5 Compare February 23, 2021 08:18

petyosi marked this pull request as ready for review February 23, 2021 08:39

mahboubii approved these changes Feb 23, 2021

View reviewed changes

src/components/MessageList/hooks/usePrependMessagesCount.js Outdated Show resolved Hide resolved

src/components/MessageList/hooks/usePrependMessagesCount.js Outdated Show resolved Hide resolved

src/components/MessageList/VirtualizedMessageList.js Outdated Show resolved Hide resolved

petyosi mentioned this pull request Feb 24, 2021

CRS-323 Virtualized dynamic heights #611

Closed

3 tasks

mahboubii mentioned this pull request Feb 24, 2021

Chore/upgrade virtuoso GetStream/mml-react#41

Merged

3 tasks

petyosi marked this pull request as draft February 24, 2021 10:53

mahboubii mentioned this pull request Feb 24, 2021

chore: upgrade mml-react #728

Merged

Tom Hutman and others added 4 commits February 24, 2021 18:31

upgrade react-virtuoso, refactor VirtualizedMessageList to work with …

a845d23

…new version

Make followOutput behavior configurable

7eb0c47

Address review comments

120c7f0

Reactions keep list at bottom

e786509

petyosi force-pushed the upgrade-virtuoso branch from b52d973 to e786509 Compare February 24, 2021 16:32

petyosi marked this pull request as ready for review February 24, 2021 16:32

Update snapshot

961afe7

DanC5 reviewed Feb 24, 2021

View reviewed changes

examples/messaging/src/App.js Outdated Show resolved Hide resolved

DanC5 approved these changes Feb 24, 2021

View reviewed changes

Revert example fiddling

d645285

DanC5 merged commit 18cdba0 into master Feb 24, 2021

delete-merged-branch bot deleted the upgrade-virtuoso branch February 24, 2021 17:29

DanC5 mentioned this pull request Feb 24, 2021

scroll does not stick to bottom on message height change. #603

Closed

kainosnoema reviewed Mar 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRS-384 Upgrade react-virtuoso to v1 #694

CRS-384 Upgrade react-virtuoso to v1 #694

ath92 commented Feb 5, 2021 •

edited by petyosi

Loading

DanC5 Feb 5, 2021

ath92 Feb 8, 2021

DanC5 Feb 8, 2021

mahboubii Feb 8, 2021

DanC5 Feb 5, 2021

DanC5 commented Feb 5, 2021

mahboubii commented Feb 8, 2021

codecov-io commented Feb 8, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading

ath92 commented Feb 9, 2021

petyosi commented Feb 9, 2021

ath92 commented Feb 12, 2021

petyosi commented Feb 23, 2021

petyosi commented Feb 24, 2021

kainosnoema Mar 15, 2021

DanC5 Mar 15, 2021

petyosi Mar 16, 2021

kainosnoema Mar 16, 2021

CRS-384 Upgrade react-virtuoso to v1 #694

CRS-384 Upgrade react-virtuoso to v1 #694

Conversation

ath92 commented Feb 5, 2021 • edited by petyosi Loading

Submit a pull request

CLA

Description of the pull request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanC5 commented Feb 5, 2021

mahboubii commented Feb 8, 2021

codecov-io commented Feb 8, 2021 • edited Loading

Codecov Report

github-actions bot commented Feb 9, 2021 • edited Loading

ath92 commented Feb 9, 2021

petyosi commented Feb 9, 2021

ath92 commented Feb 12, 2021

petyosi commented Feb 23, 2021

petyosi commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ath92 commented Feb 5, 2021 •

edited by petyosi

Loading

codecov-io commented Feb 8, 2021 •

edited

Loading

github-actions bot commented Feb 9, 2021 •

edited

Loading