Skip to content

Conversation

@MartinCupela
Copy link
Contributor

@MartinCupela MartinCupela commented Sep 19, 2022

🎯 Goal

Avoid unnecessary re-renders caused by regenerating a new array of message group style keys (Object.keys() was called directly in the hook dependency array).

oliverlaz
oliverlaz previously approved these changes Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #1761 (f51f2a6) into develop (aeff06a) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1761      +/-   ##
===========================================
- Coverage    83.58%   83.55%   -0.04%     
===========================================
  Files          256      256              
  Lines         6239     6239              
  Branches      1675     1675              
===========================================
- Hits          5215     5213       -2     
- Misses         869      871       +2     
  Partials       155      155              
Impacted Files Coverage Δ
...mponents/ChannelList/hooks/usePaginatedChannels.ts 88.23% <ø> (ø)
.../components/MessageList/VirtualizedMessageList.tsx 64.17% <100.00%> (ø)
...omponents/MessageList/hooks/useEnrichedMessages.ts 86.36% <100.00%> (ø)
...components/Reactions/hooks/useProcessReactions.tsx 100.00% <100.00%> (ø)
src/components/Channel/Channel.tsx 80.45% <0.00%> (-0.66%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@petyosi petyosi left a comment

Choose a reason for hiding this comment

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

I believe that we had a bug where processedMessages were incorrectly rebuilt with a new object identity at some point, hence the .length usage (which, of course, is not correct but we are not aware of any issues because of it).
Given that, I would say that this change should not touch on that.

@MartinCupela
Copy link
Contributor Author

I believe that we had a bug where processedMessages were incorrectly rebuilt with a new object identity at some point, hence the .length usage (which, of course, is not correct but we are not aware of any issues because of it). Given that, I would say that this change should not touch on that.

@petyosi I have reverted the changes and added the explanatory comment for future reference.

petyosi
petyosi previously approved these changes Sep 19, 2022
arnautov-anton
arnautov-anton previously approved these changes Sep 19, 2022
@MartinCupela MartinCupela dismissed stale reviews from arnautov-anton and petyosi via f51f2a6 September 19, 2022 10:30
@MartinCupela MartinCupela merged commit 41d1d67 into develop Sep 19, 2022
@MartinCupela MartinCupela deleted the fix/remove-unmemoized-dep-in-VML-hook branch September 19, 2022 10:40
@MartinCupela MartinCupela mentioned this pull request Sep 19, 2022
github-actions bot pushed a commit that referenced this pull request Sep 19, 2022
# [10.1.0](v10.0.2...v10.1.0) (2022-09-19)

### Bug Fixes

* **VirtualizedMessageList:** use memoized values as hook dependencies directly ([#1761](#1761)) ([41d1d67](41d1d67))

### Features

* provide close callback to app menu ([#1754](#1754)) ([5202a5f](5202a5f))
@petyosi
Copy link
Contributor

petyosi commented Sep 19, 2022

🎉 This PR is included in version 10.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants