Refactor systemTag changes to stop directly mutating note object
#1615
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
Previously we have been directly mutating the note object in the action creator when toggling a note's pinned status, markdown status, publish status, and trash status. This mutation can lead to concurrency defects which expose themselves as inconsistent UI state. This breaks our Redux model which assumes that all UI updates happen atomically. In this patch we're building a new note object when toggling these system tags in order to maintain our consistency. At the same time I have removed the thunks from the action creators since they weren't doing anything and didn't need to exist. This shouldn't have a noticeable impact on the application.
This was referenced Oct 2, 2019
belcherj
approved these changes
Oct 2, 2019
belcherj
reviewed
Oct 2, 2019
| } = note; | ||
| const hasTagAlready = systemTags.includes(systemTag); | ||
|
|
||
| return hasTagAlready !== shouldHaveTag |
There was a problem hiding this comment.
This took me a while to parse. No idea how it could be any simpler but it took me some time to figure out what was going on.
There was a problem hiding this comment.
yeah I don't know the best phrasing. This was the simplest I got.
if ( noteIsAlreadyInAStateWhereThePresenceOrAbsenceOfTheTagMatchesTheExpectationOnceThisFunctionEnds )const tagIsAlreadyInNote = systemTags.includes(systemTag);
const shouldAddTagToNote = ?In my previous work at least there was convention that should_ indicates a desire to enact the thing and that it's boolean. Here in JS land we only have a single type any so the language doesn't help us out like it does in statically-typed languages.
Any recommendation or just noting that it's something that could be better some how?
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.
See #1614
As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.
It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.
Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.
Replaces #1611
Replaces #1613
Previously we have been directly mutating the note object in the action
creator when toggling a note's pinned status, markdown status, publish
status, and trash status.
This mutation can lead to concurrency defects which expose themselves as
inconsistent UI state. This breaks our Redux model which assumes that
all UI updates happen atomically.
In this patch we're building a new note object when toggling these
system tags in order to maintain our consistency.
At the same time I have removed the thunks from the action creators
since they weren't doing anything and didn't need to exist. This
shouldn't have a noticeable impact on the application.