Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

MM-13432 Remove unused _REQUEST, _SUCCESS and _FAILURE actions #748

Merged
merged 5 commits into from Jan 4, 2019
Merged

MM-13432 Remove unused _REQUEST, _SUCCESS and _FAILURE actions #748

merged 5 commits into from Jan 4, 2019

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Dec 26, 2018 •
edited

Summary

This PR removes any unused status states and requests from the following entities

Channels
Posts
teams
Users
Files
Emoji

This is part of the performance improvement for channel switch/ postList mount and update. PR does not completely remove all of the request entities as most of the flags left does not impact the performance for the channel switch. Moreover moving away from storing flags might not be al that possible and needed in few cases so, we might have to circle back again to a similar pattern. Moving the flags to react components can cause problems as state is lost when a component is unmounted.

Going forward we have to make sure that we don't introduce these state flags for request if we don't use

Ticket Link

MM-13544
related mobile PR: mattermost/mattermost-mobile#2490

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit tests passed
  • Ran make flow to ensure type checking passed
  • Added or updated unit tests (required for all new features)

@sudheerDev sudheerDev added the 2: Dev Review Requires review by a core commiter label Dec 26, 2018
@sudheerDev sudheerDev added this to the v5.8.0 milestone Dec 26, 2018
@sudheerDev sudheerDev added Awaiting Submitter Action Blocked on the author and removed 2: Dev Review Requires review by a core commiter labels Dec 27, 2018
lieut-data
lieut-data approved these changes Jan 2, 2019
View reviewed changes
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

This is terrific! Fully agree with the summary as well: do we want to raise this at the next developer's meeting and capture this pattern going forward?

src/actions/helpers.js
@@ -62,29 +62,37 @@ export function requestFailure(type: ActionType, error: Client4Error) {
* @param {...Array<any>} args
* @returns {ActionFunc} ActionFunc
*/
export function bindClientFunc(clientFunc: () => Promise<mixed>, request: ActionType,
success: ActionType | Array<ActionType>, failure: ActionType, ...args: Array<any>): ActionFunc {
export function bindClientFunc({
Copy link
Member

Choose a reason for hiding this comment

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

Should we update the @param comments here? Do we need to capture any flow types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I will update the params here. I need to push another commit as i saw few unused flags in mobile repo. So will be removing them and updating this PR along with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lieut-data done 👍 Have a look at the flow commit changes. Let me know if you have any comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah forgot to change the comment of @param here :(

@jwilander
Copy link
Member

@sudheerDev is this ready for review?

@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter and removed Awaiting Submitter Action Blocked on the author labels Jan 3, 2019
@sudheerDev
Copy link
Contributor Author

@jwilander Forgot to change the label back. It is up for review

jwilander
jwilander approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Awesome. Only question I have is: the mobile app uses the request state in some cases, will merging this require changes in the mobile app to address that?

@sudheerDev
Copy link
Contributor Author

Yes both mobile and webapp do still use few flags.
This PR removes the flags from few files which effect the perf and not at all used in both repos.

There is a related mobile PR in i removed few unused status flags so, i removed them as well and removed them from redux files in the last commit here.

Rest of the flags left here in the PR for the files above are being used and luckily they are not all that frequent actions.

@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Jan 3, 2019
@sudheerDev
MM-13432 Eliminate _REQUEST, _SUCCESS and _FAILURE actions
d54adcc 
  * Remove request status from posts
  * Remove request status from channels
  * Remove request status from teams
  * Remove request status from files
  * Remove emojis requests

  * Change bindClientFunc to accept obj instead or arguments
@sudheerDev
Fix flow errors
7e91e2a
@sudheerDev
Remove unused mobile repo state flags
ff3adf0
@sudheerDev
Update jsdoc annotations for client helper func
413efad
@sudheerDev sudheerDev merged commit e6a49c9 into mattermost:master Jan 4, 2019
@sudheerDev sudheerDev deleted the MM-13432 branch January 4, 2019 13:00
@sudheerDev sudheerDev mentioned this pull request Jan 8, 2019
Merged
1 task
DSchalla pushed a commit to DSchalla/mattermost-redux that referenced this pull request Apr 1, 2019
@sudheerDev @DSchalla
MM-13432 Remove unused _REQUEST, _SUCCESS and _FAILURE actions (matte…
0bce178 
…rmost#748)

* MM-13432 Eliminate _REQUEST, _SUCCESS and _FAILURE actions

  * Remove request status from posts
  * Remove request status from channels
  * Remove request status from teams
  * Remove request status from files
  * Remove emojis requests
  * Change bindClientFunc to accept obj instead or arguments
  * Fix flow errors
  * Remove unused mobile repo state flags
  * Update jsdoc annotations for client helper func
  * Fix rebase issue
This was referenced Oct 1, 2019
Closed
Closed
Closed
This was referenced Oct 1, 2019
Closed
Closed
Closed
Closed
This was referenced Nov 8, 2019
Closed
Closed
@0xVolodya 0xVolodya mentioned this pull request Dec 9, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lieut-data lieut-data lieut-data approved these changes

@jwilander jwilander jwilander approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v5.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sudheerDev @jwilander @lieut-data