From ad5af984171267bd6d92a560d9d005ea5ebf276e Mon Sep 17 00:00:00 2001 From: infinite-persistence Date: Tue, 5 Oct 2021 15:04:25 +0800 Subject: [PATCH 1/3] makeSelectOthersReactionsForComment: fix memoization `selectState` will always change we when update the state in the reducer, regardless of which field (sub-state) was updated. So, it shouldn't be used as the input for `createSelector`. --- ui/redux/selectors/comments.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ui/redux/selectors/comments.js b/ui/redux/selectors/comments.js index e19e236f4..e009bf4ca 100644 --- a/ui/redux/selectors/comments.js +++ b/ui/redux/selectors/comments.js @@ -5,6 +5,8 @@ import { selectShowMatureContent } from 'redux/selectors/settings'; import { selectBlacklistedOutpointMap, selectFilteredOutpointMap } from 'lbryinc'; import { selectClaimsById, isClaimNsfw, selectMyActiveClaims } from 'lbry-redux'; +type State = { comments: CommentsState }; + const selectState = (state) => state.comments || {}; export const selectCommentsById = createSelector(selectState, (state) => state.commentById || {}); @@ -13,7 +15,7 @@ export const selectIsFetchingCommentsById = createSelector(selectState, (state) export const selectIsFetchingCommentsByParentId = createSelector(selectState, (state) => state.isLoadingByParentId); export const selectIsPostingComment = createSelector(selectState, (state) => state.isCommenting); export const selectIsFetchingReacts = createSelector(selectState, (state) => state.isFetchingReacts); -export const selectOthersReactsById = createSelector(selectState, (state) => state.othersReactsByCommentId); +export const selectOthersReactsById = (state: State) => state.comments.othersReactsByCommentId; export const selectPinnedCommentsById = createSelector(selectState, (state) => state.pinnedCommentsById); export const makeSelectPinnedCommentsForUri = (uri: string) => @@ -191,12 +193,12 @@ export const makeSelectMyReactionsForComment = (commentIdChannelId: string) => }); export const makeSelectOthersReactionsForComment = (commentId: string) => - createSelector(selectState, (state) => { - if (!state.othersReactsByCommentId) { + createSelector(selectOthersReactsById, (othersReactsByCommentId) => { + if (!othersReactsByCommentId) { return {}; } - return state.othersReactsByCommentId[commentId] || {}; + return othersReactsByCommentId[commentId] || {}; }); export const selectPendingCommentReacts = createSelector(selectState, (state) => state.pendingCommentReactions); -- 2.45.2 From 875128fc94f45341a915197fe224e2867f83c2d0 Mon Sep 17 00:00:00 2001 From: infinite-persistence Date: Mon, 4 Oct 2021 18:55:03 +0800 Subject: [PATCH 2/3] Comment: fix memoization for multi-instance selector. Since each comment will have a different key (and on other components, `props.uri` will differ), the selector cache won't work since the inputs keep changing. The suggested solution is to apply a factory method wrap the selector -- this produces a unique memoized selector for each instance. --- ui/component/comment/index.js | 38 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/ui/component/comment/index.js b/ui/component/comment/index.js index 901b8cd3b..8ceae35d7 100644 --- a/ui/component/comment/index.js +++ b/ui/component/comment/index.js @@ -19,24 +19,30 @@ import { selectActiveChannelClaim } from 'redux/selectors/app'; import { selectPlayingUri } from 'redux/selectors/content'; import Comment from './view'; -const select = (state, props) => { - const activeChannelClaim = selectActiveChannelClaim(state); +const makeMapStateToProps = (originalState, originalProps) => { + const activeChannelClaim = selectActiveChannelClaim(originalState); const activeChannelId = activeChannelClaim && activeChannelClaim.claim_id; - const reactionKey = activeChannelId ? `${props.commentId}:${activeChannelId}` : props.commentId; + const reactionKey = activeChannelId ? `${originalProps.commentId}:${activeChannelId}` : originalProps.commentId; + const selectOthersReactionsForComment = makeSelectOthersReactionsForComment(reactionKey); - return { - claim: makeSelectClaimForUri(props.uri)(state), - thumbnail: props.authorUri && makeSelectThumbnailForUri(props.authorUri)(state), - channelIsBlocked: props.authorUri && makeSelectChannelIsMuted(props.authorUri)(state), - commentingEnabled: IS_WEB ? Boolean(selectUserVerifiedEmail(state)) : true, - othersReacts: makeSelectOthersReactionsForComment(reactionKey)(state), - activeChannelClaim, - myChannels: selectMyChannelClaims(state), - playingUri: selectPlayingUri(state), - stakedLevel: makeSelectStakedLevelForChannelUri(props.authorUri)(state), - linkedCommentAncestors: selectLinkedCommentAncestors(state), - totalReplyPages: makeSelectTotalReplyPagesForParentId(props.commentId)(state), + const select = (state, props) => { + const othersReacts = selectOthersReactionsForComment(state); + + return { + claim: makeSelectClaimForUri(props.uri)(state), + thumbnail: props.authorUri && makeSelectThumbnailForUri(props.authorUri)(state), + channelIsBlocked: props.authorUri && makeSelectChannelIsMuted(props.authorUri)(state), + commentingEnabled: IS_WEB ? Boolean(selectUserVerifiedEmail(state)) : true, + othersReacts, + activeChannelClaim, + myChannels: selectMyChannelClaims(state), + playingUri: selectPlayingUri(state), + stakedLevel: makeSelectStakedLevelForChannelUri(props.authorUri)(state), + linkedCommentAncestors: selectLinkedCommentAncestors(state), + totalReplyPages: makeSelectTotalReplyPagesForParentId(props.commentId)(state), + }; }; + return select; }; const perform = (dispatch) => ({ @@ -47,4 +53,4 @@ const perform = (dispatch) => ({ doToast: (options) => dispatch(doToast(options)), }); -export default connect(select, perform)(Comment); +export default connect(makeMapStateToProps, perform)(Comment); -- 2.45.2 From dc961b98ac96600818538c63032ef90933bb1a93 Mon Sep 17 00:00:00 2001 From: infinite-persistence Date: Tue, 5 Oct 2021 15:25:56 +0800 Subject: [PATCH 3/3] CommentReactions: optimize othersReacts selection One solution is to apply the same factory method used in `Comments` to memoize the selector for each key. Unfortunately, I'm not sure how to re-use the same factory for both `Comments` and `CommentReactions`, so we will be memoizing it twice. This feels a bit dumb. Since `CommentReactions` is almost-always** a child of `Comments` and using the same comment key, just get the calculated value from the parent through an optional prop. ** notifications uses it too, but I guess we can be inefficient there since it's not a component that's constantly updating. --- ui/component/comment/view.jsx | 4 +++- ui/component/commentReactions/index.js | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ui/component/comment/view.jsx b/ui/component/comment/view.jsx index b4816303a..0ca6b5405 100644 --- a/ui/component/comment/view.jsx +++ b/ui/component/comment/view.jsx @@ -361,7 +361,9 @@ function Comment(props: Props) { icon={ICONS.REPLY} /> )} - {ENABLE_COMMENT_REACTIONS && } + {ENABLE_COMMENT_REACTIONS && ( + + )} )} diff --git a/ui/component/commentReactions/index.js b/ui/component/commentReactions/index.js index 0bafedfca..cb3a61e8c 100644 --- a/ui/component/commentReactions/index.js +++ b/ui/component/commentReactions/index.js @@ -1,5 +1,5 @@ import { connect } from 'react-redux'; -import Comment from './view'; +import CommentReactions from './view'; import { makeSelectClaimIsMine, makeSelectClaimForUri, doResolveUri } from 'lbry-redux'; import { doToast } from 'redux/actions/notifications'; import { makeSelectMyReactionsForComment, makeSelectOthersReactionsForComment } from 'redux/selectors/comments'; @@ -15,7 +15,7 @@ const select = (state, props) => { claim: makeSelectClaimForUri(props.uri)(state), claimIsMine: makeSelectClaimIsMine(props.uri)(state), myReacts: makeSelectMyReactionsForComment(reactionKey)(state), - othersReacts: makeSelectOthersReactionsForComment(reactionKey)(state), + othersReacts: props.othersReacts || makeSelectOthersReactionsForComment(reactionKey)(state), activeChannelId, }; }; @@ -26,4 +26,4 @@ const perform = (dispatch) => ({ doToast: (params) => dispatch(doToast(params)), }); -export default connect(select, perform)(Comment); +export default connect(select, perform)(CommentReactions); -- 2.45.2