Migrate create_comment.jsx to be pure and use Redux #243

QuantumKing · 2017-11-05T07:03:40Z

Summary

Migrate create_comment.jsx to be pure and use Redux

Ticket Link

mattermost/mattermost#7672

Checklist

Added or updated unit tests (required for all new features)
Touches critical sections of the codebase (auth, posting, etc.)

saturninoabril

Awesome, looks good to me, thanks!

jwilander · 2017-11-10T19:21:12Z

actions/views/rhs.jsx

+
+export function updateCommentDraft(rootId, draft) {
+ return (dispatch) => {
+ PostStore.storeCommentDraft(rootId, draft);


This will work but PostStore is really just dispatching to the newly. So in interest in reducing usage of PostStore, can you perform the dispatch directly?

jwilander · 2017-11-10T19:25:28Z

actions/views/rhs.jsx

+
+export function makeOnSubmit(channelId, rootId, latestPostId) {
+ return () => async (dispatch, getState) => {
+ const draft = PostStore.getCommentDraft(rootId);


Similar to comment above, maybe use a storage selector to get the draft? Or create a new selector for it. I'm 0/5 on how it's done, just want to reduce PostStore usage

Ah yeah I see there was a push recently with all this in it. I'll make use of it 👍

@jwilander removed PostStore usage

jwilander

Awesome, lgtm

hmhealey

Looks pretty good, but I have a couple concerns about the mapDispatchToProps and the usage of selectors. Also, scrollToTop might need to be renamed scrollToBottom

hmhealey · 2017-11-13T17:00:59Z

components/create_comment/create_comment.jsx

 this.handleSubmit(e);
 }
 }

 GlobalActions.emitLocalUserTypingEvent(this.props.channelId, this.props.rootId);
 }

+ scrollToTop = () => {


I believe this method actually scrolls to the bottom, not the top

hmhealey · 2017-11-13T17:43:58Z

components/create_comment/create_comment.jsx

- } else {
+ const {draft} = this.props;
+
+ const newMessage = (function getNewMessage() {


I don't think there's anything that needs to be changed here, but I don't know of any other places that we use an inline function in this way for control flow. This could also be done in the following way which might be simpler

if (draft.message === '') { newMessage = ...; } else if (regex.test(draft.message)) { newMessage = ...; } else { newMessage = ...; }

hmhealey · 2017-11-13T21:52:41Z

components/create_comment/index.js

+ return (draft) => updateCommentDraft(rootId, draft);
+}
+
+function mapDispatchToProps(dispatch, ownProps) {


Is mapDispatchToProps called on every store change like mapStateToProps? I've never tried it, but I think using an arrow function or these other functions like makeOnSubmit may cause the CreateComment component to re-render unnecessarily since the actions are changing constantly. If that's the case, we could just pass in the raw actions and have component pass the arguments in itself

I can cache them in makeMapDispatchToProps. Its a pattern I've seen elsewhere in the app.

hmhealey · 2017-11-13T22:03:21Z

selectors/rhs.jsx

+export function makeGetCurrentUsersLatestPost(channelId, rootId) {
+ return createSelector(
+ getCurrentUserId,
+ (state) => state.entities.posts.postsInChannel[channelId] || [],


I think it would be better to just return state.entities.posts.postsInChannel[channelId] and then check if postIds is empty below. Right now, if that part of the state is null, it'll return a new empty array each time this is called, preventing any memoization. Not a likely case, but I think it would be better to be safe here.

hmhealey · 2017-11-13T22:04:49Z

selectors/rhs.jsx

+ return createSelector(
+ getCurrentUserId,
+ (state) => state.entities.posts.postsInChannel[channelId] || [],
+ (state) => (id) => getPost(state, id),


This is going to be returning a new function each time the selector is called, which will also prevent memoization.

It may be easier to just not use a selector here since the overhead and complexity of using a selector might not be worth it when it's unlikely the user will likely go to edit their post more than once. It may actually be slightly worse overall performance to use a selector since it's making a new function each time the channelId or rootId change when the user may not even use this hotkey.

@hmhealey What would you like me to do here?

I can move it to the same place as makeOnEditLatestPost as a util function instead, since this is the only place its used. That way it wont be used in mapStateToProps.

It'll only do all this work when the user wants to edit their latest post.

I think it might still be called whenever mapStateToProps is called for a new thread (ie a different root_id or channel_id) since that'll cause call makeOnEditLatestPost which'll call this in turn. That'll still create a new function here and blow away memoization, but we want that anyway

hmhealey · 2017-11-13T22:10:42Z

selectors/rhs.jsx

+ );
+}
+
+export function makeGetCommentDraft(rootId) {


This looks a bit weird to me since it's not returning a selector here as I'd expect from a makeGetX function, but I assume that's because this is based off the storage helper functions. It's fine to keep as is though.

hmhealey · 2017-11-13T22:11:42Z

tests/components/create_comment/__snapshots__/create_comment.test.jsx.snap

+ className="icon icon--emoji emoji-rhs "
+ dangerouslySetInnerHTML={
+ Object {
+ "__html": "<svg width='15px' height='15px' viewBox='0 0 15 15' version='1.1' xmlns='http:https://www.w3.org/2000/svg' xmlns:xlink='http:https://www.w3.org/1999/xlink'> <g stroke='none' stroke-width='1' fill='inherit' fill-rule='evenodd'> <g transform='translate(-1071.000000, -954.000000)' fill='inherit'> <g transform='translate(25.000000, 937.000000)'> <g transform='translate(1046.000000, 17.000000)'> <path d='M7.5,0.0852272727 C3.405,0.0852272727 0.0852272727,3.405 0.0852272727,7.5 C0.0852272727,11.595 3.405,14.9147727 7.5,14.9147727 C11.595,14.9147727 14.9147727,11.595 14.9147727,7.5 C14.9147727,3.405 11.595,0.0852272727 7.5,0.0852272727 Z M7.5,14.0663436 C3.87926951,14.0663436 0.933656417,11.1207305 0.933656417,7.5 C0.933656417,3.87926951 3.87926951,0.933656417 7.5,0.933656417 C11.1207305,0.933656417 14.0663436,3.87926951 14.0663436,7.5 C14.0663436,11.1207305 11.1207305,14.0663436 7.5,14.0663436 Z'></path> <path d='M11.7732955,8.95397727 C12.0119318,8.90488636 12.2159659,9.11778409 12.1684091,9.35676136 C11.8063636,11.1790909 9.85346591,12.5710227 7.49846591,12.5710227 C5.15096591,12.5710227 3.20284091,11.1877841 2.83193182,9.37397727 C2.78181818,9.129375 2.99267045,8.911875 3.23744318,8.96198864 C4.85369318,9.29232955 10.1786932,9.28142045 11.7732955,8.95397727 Z'></path> <ellipse cx='4.94318182' cy='5.50431818' rx='1' ry='1.06534091'></ellipse> <ellipse cx='10.0568182' cy='5.50431818' rx='1' ry='1.06534091'></ellipse> </g> </g> </g> </g> </svg>",


Ugh, those don't make these tests nice to look at. Good thing we don't manually write these tests.

hmhealey · 2017-11-13T22:14:06Z

Assigning myself so I remember to follow up. Also, Jenkins isn't building because there's merge conflicts

…QuantumKing/mattermost-webapp into migrate-create-comment-to-redux

hmhealey · 2017-11-14T15:20:20Z

components/create_comment/index.js

+ return (draft) => updateCommentDraft(rootId, draft);
+}
+
+function makeMapDispatchToProps() {


I don't actually think we've done this elsewhere. We usually just pass in the action creators into the component, and the component calls it with whatever arguments it needs. This works though, and it shouldn't cause any unnecessary re-renders as far as I can tell.

hmhealey · 2017-11-14T15:25:25Z

@QuantumKing Going to merge this now. Thanks for making those changes

* Update README usage to use try catchs * Add promise example

QuantumKing added 11 commits November 4, 2017 20:44

use mattermost-redux actions

ed11dcf

convert to PureComponent

c2e1770

remove message state

c5f50cb

remove unused submitted state

0527e4d

cleanup

2802789

fix code style

eb701eb

enableAddButton as a prop

06381a2

fileCount

3bfcedc

fileCount

a253cdd

snapshot tests

c034f6b

remove snapshot tests

3c3d30f

jwilander added the 2: Dev Review Requires review by a core commiter label Nov 5, 2017

jwilander requested review from saturninoabril and jwilander November 5, 2017 15:53

QuantumKing added 8 commits November 5, 2017 10:55

swap dispatch order

593612e

use enableAddButton prop

4a199cf

add isRequired for draft prop

5ace7cd

remove emoji store from component

d383be6

added proptype comments

e0fd5fa

fix style issue

1300795

use EmojiMap

b1f7714

added some basic snapshot tests

0997d8f

jasonblais added Work in Progress Not yet ready for review and removed 2: Dev Review Requires review by a core commiter labels Nov 6, 2017

QuantumKing added 6 commits November 5, 2017 20:36

add/remove reaction props

86c8c57

replaced history store with redux

cd69f88

move all submission logic to actions

331acb6

fix unused vars

4cf451f

create unit testable view actions

604070e

add selectors for latest post

bd07ba3

saturninoabril approved these changes Nov 9, 2017

View reviewed changes

coreyhulen requested a review from hmhealey November 10, 2017 19:20

jwilander reviewed Nov 10, 2017

View reviewed changes

QuantumKing added 5 commits November 10, 2017 18:56

removed usage of PostStore

3e0afc3

removed another instance of PostStore

40e4edc

moved actions tests to redux folder

48e4385

Merge branch 'master' into migrate-create-comment-to-redux

a218739

remove AppDispatcher usage

6383f65

jwilander approved these changes Nov 13, 2017

View reviewed changes

mattermost deleted a comment from mattermod Nov 13, 2017

hmhealey suggested changes Nov 13, 2017

View reviewed changes

hmhealey self-assigned this Nov 13, 2017

QuantumKing added 7 commits November 13, 2017 18:52

addressed feedback

fb19ede

Merge branch 'master' into migrate-create-comment-to-redux

7b9badb

another piece of feedback

64fe385

Merge branch 'migrate-create-comment-to-redux' of https://github.com/…

87191d9

…QuantumKing/mattermost-webapp into migrate-create-comment-to-redux

rename rhs to create_comment

a4980af

fix tests

562303c

fix styles

326ab51

hmhealey approved these changes Nov 14, 2017

View reviewed changes

hmhealey removed their assignment Nov 14, 2017

hmhealey removed the 2: Dev Review Requires review by a core commiter label Nov 14, 2017

hmhealey merged commit c6b7946 into mattermost:master Nov 14, 2017

lindalumitchell added the Tests/Not Needed Does not require new release tests label Nov 15, 2017

esethna added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 4, 2017

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Update README usage to use try catchs (#243)

498e71d

* Update README usage to use try catchs * Add promise example

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Update README usage to use try catchs (#243)

2cfd040

* Update README usage to use try catchs * Add promise example

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate create_comment.jsx to be pure and use Redux #243

Migrate create_comment.jsx to be pure and use Redux #243

QuantumKing commented Nov 5, 2017 •

edited

Loading

saturninoabril left a comment

jwilander Nov 10, 2017

jwilander Nov 10, 2017

QuantumKing Nov 10, 2017

QuantumKing Nov 11, 2017

jwilander left a comment

hmhealey left a comment

hmhealey Nov 13, 2017

hmhealey Nov 13, 2017

hmhealey Nov 13, 2017

QuantumKing Nov 13, 2017 •

edited

Loading

hmhealey Nov 13, 2017

QuantumKing Nov 13, 2017

hmhealey Nov 13, 2017

QuantumKing Nov 13, 2017

QuantumKing Nov 14, 2017

QuantumKing Nov 14, 2017 •

edited

Loading

hmhealey Nov 14, 2017

hmhealey Nov 13, 2017

hmhealey Nov 13, 2017

hmhealey commented Nov 13, 2017

hmhealey Nov 14, 2017

hmhealey commented Nov 14, 2017

Migrate create_comment.jsx to be pure and use Redux #243

Migrate create_comment.jsx to be pure and use Redux #243

Conversation

QuantumKing commented Nov 5, 2017 • edited Loading

Summary

Ticket Link

Checklist

saturninoabril left a 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

Choose a reason for hiding this comment

jwilander left a comment

Choose a reason for hiding this comment

hmhealey left a 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

QuantumKing Nov 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

QuantumKing Nov 14, 2017 • edited Loading

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

hmhealey commented Nov 13, 2017

Choose a reason for hiding this comment

hmhealey commented Nov 14, 2017

QuantumKing commented Nov 5, 2017 •

edited

Loading

QuantumKing Nov 13, 2017 •

edited

Loading

QuantumKing Nov 14, 2017 •

edited

Loading