Add approval controller count state #306
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rekmarks
force-pushed
the
approval-controller-count-state
branch
from
9de5b4d to
2a49858
Compare
Merged
|
Why is the count needed in state? The count methods should already suffice I'd think. This introduces the possibility that it could get out-of-sync with the underlying list. |
Gudahtt
reviewed
Nov 13, 2020
| [id]: info, | ||
| }, | ||
| [APPROVAL_COUNT_STORE_KEY]: this.state[APPROVAL_COUNT_STORE_KEY] + 1, |
There was a problem hiding this comment.
As an example of the count getting out of sync, this could happen here if id is already in state. Then this would replace that entry, and falsely inflate the count by 1 per additional call.
There was a problem hiding this comment.
If consumers respect the interface of the controller and only use its public methods, it is impossible for that to happen (see _validateAddParams).
I decided to add the approval count to the state of this controller in order to make it consumable via our usual state subscription logic.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#304 added the methods. This adds the state. Also properly initializes this controller, which wasn't happening previously. It's clearly not always necessary, but the constructor should always call
BaseController.initialize().