sourcegraph
Minimize state passing through local storage
Changes
Core changes are in the modelsService.ts.
Previously setting selected model was looking like this:
UI -> ModelService::setSelectedModel -> LocalStorage::setModelPreferences -> [ change in local storage] -> LocalStorage::clientStateChanges -> resolvedConfig -> syncModels -> ModelService::modelsChanges -> ModelService::syncPreferencesSubscription ->
LocalStorage::setModelPreferences -> [ change in local storage] -> LocalStorage::clientStateChanges -> resolvedConfig -> syncModels -> ModelService::modelsChanges
WOW, that is a long chain of calls! It is going through local store local store and then through few observables twice. All that to propagate change from ModelService to ModelService.
For the curious readers, when we first enter ModelService::modelsChanges we get the updated list of local/remote models, but without included user preferences (e.g. previous selections which was saved in local store). Then we update it to include user preferences or optional enrolments of new features, save to local store, and then go through update chain again.
After my changes it is still complex but much simpler than before:
UI -> ModelService::setSelectedModel -> LocalStorage::setModelPreferences -> [ change in local storage] -> LocalStorage::clientStateChanges -> resolvedConfig -> syncModels -> ModelService::modelsChanges ->
[[ (discarded because result won't change)
LocalStorage::setModelPreferences -> [ change in local storage] -> LocalStorage::clientStateChanges -> resolvedConfig -> syncModels -> [X]
]]
Main change is that I update modelsChanges including preferences in one step instead of triggering it as action from local store. It still may cause change in the local store, and trigger changes in the observables chain (included in the [[ ]] brackets above), but the models we will receive as an update will be the same as already computed, and due to filtering distinct values it won't be perceived as an update. So we kind of doing loop once instead of twice, and we do not depend on local store change notification which was making an update fragile (not atomic).
Test plan
N/A - this have high coverage through unit and itntegration tests
