Page MenuHomePhabricator
Differential D10933

[lib] avoid modifying state on rehydrate action
AbandonedPublic
Actions

Authored by kamil on Feb 2 2024, 3:07 AM.
Tags
None
Referenced Files
F2098724: D10933.id36582.diff
Mon, Jun 24, 3:19 PM
Unknown Object (File)
Fri, Jun 21, 12:48 AM
Unknown Object (File)
Fri, Jun 14, 5:18 AM
Unknown Object (File)
Tue, Jun 11, 11:48 PM
Unknown Object (File)
Sat, Jun 1, 5:39 PM
Unknown Object (File)
May 23 2024, 10:24 AM
Unknown Object (File)
May 23 2024, 10:24 AM
Unknown Object (File)
May 23 2024, 10:24 AM
View All 16 Files
Subscribers
ashoat
tomek

Details

Reviewers
inka
Summary

According to docs: "Auto merge means if the some piece of substate was modified by your reducer during the REHYDRATE action, it will skip this piece of state." src.

If the logic changes and we modify keyserverStore state on persist/REHYDRATE it could cause skipping the rehydrated state.

Test Plan

Add this line
keyserverStore = { ...keyserverStore }; in if - previously it was causing logging out on each refresh, now it shouldn't

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Feb 2 2024, 3:07 AM
kamil held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 2 2024, 3:07 AM
kamil edited the summary of this revision. (Show Details)Feb 2 2024, 3:07 AM
kamil added a child revision: D10934: [lib] refactor keyserver-related actions in `master-reducer` to ops.Feb 2 2024, 3:09 AM
kamil published this revision for review.Feb 2 2024, 3:29 AM
Harbormaster completed remote builds in B26464: Diff 36582.Feb 2 2024, 3:35 AM
inka requested changes to this revision.Feb 5 2024, 1:57 AM

I don't understand the point of this diff. Do we need to do keyserverStore = { ...keyserverStore }; for some reason?
I know what mechanism you mean, with the REHYDRATE skipping a part of state if it was changed by the reducer, but this code should never change anything on REHYDRATE action, because keyserverStore is not changed by the REHYDRATE action. So the inner ifs (line 102 and 115) will always return false and not execute. And it is very intentional that the keyserver store is not changed on the REHYDRATE action.

If the logic changes and we modify keyserverStore state on persist/REHYDRATE it could cause skipping the rehydrated state.

If the logic changes and we modify keyserverStore state on persist/REHYDRATE the rehydration WILL be skipped for the keyserver store, REGARDELESS of this code here. On the other hand, if we decide to change keyserverStore on REHYDRATE action, then maybe this code should run? Are we sure it shouldn't?

This revision now requires changes to proceed.Feb 5 2024, 1:57 AM
kamil abandoned this revision.Feb 6 2024, 3:14 AM

Discussed this with @inka today in the office - we think this code is confusing but adding action.type !== rehydrateActionType is not making it any better.

Revision Contents
Changeset List

PathSize
lib/
reducers/
9 lines

Diff 36582

View Options

lib/reducers/master-reducer.js

Loading...