Improve PersistOptions merging (#3913)

xh · Jan 31, 2025 · 16786f6 · 16786f6
1 parent f8d655b
commit 16786f6
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,8 @@
 ### 🐞 Bug Fixes
 
 * Fixed Role grid losing view state on refresh.
+* Fixed bug when merging `PersistOptions` with conflicting implicit provider types.
+* Fixed bug where explicit `persistGrouping` options were not being respected by `GridModel`.
 
 ## v72.0.0 - 2025-01-27
 

diff --git a/cmp/filter/FilterChooserModel.ts b/cmp/filter/FilterChooserModel.ts
@@ -415,7 +415,9 @@ export class FilterChooserModel extends HoistModel {
     }: FilterChooserPersistOptions) {
         if (persistValue) {
             const status = {initialized: false},
-                persistWith = isObject(persistValue) ? persistValue : rootPersistWith;
+                persistWith = isObject(persistValue)
+                    ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistValue)
+                    : rootPersistWith;
             PersistenceProvider.create({
                 persistOptions: {
                     path: `${path}.value`,
@@ -432,7 +434,9 @@ export class FilterChooserModel extends HoistModel {
         }
 
         if (persistFavorites) {
-            const persistWith = isObject(persistFavorites) ? persistFavorites : rootPersistWith,
+            const persistWith = isObject(persistFavorites)
+                    ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistFavorites)
+                    : rootPersistWith,
                 provider = PersistenceProvider.create({
                     persistOptions: {
                         path: `${path}.favorites`,

diff --git a/cmp/grid/impl/InitPersist.ts b/cmp/grid/impl/InitPersist.ts
@@ -24,7 +24,9 @@ export function initPersist(
     }: GridModelPersistOptions
 ) {
     if (persistColumns) {
-        const persistWith = isObject(persistColumns) ? persistColumns : rootPersistWith;
+        const persistWith = isObject(persistColumns)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistColumns)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.columns`,
@@ -39,7 +41,9 @@ export function initPersist(
     }
 
     if (persistSort) {
-        const persistWith = isObject(persistSort) ? persistSort : rootPersistWith;
+        const persistWith = isObject(persistSort)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistSort)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.sortBy`,
@@ -55,7 +59,9 @@ export function initPersist(
     }
 
     if (persistGrouping) {
-        const persistWith = isObject(persistSort) ? persistSort : rootPersistWith;
+        const persistWith = isObject(persistGrouping)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistGrouping)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.groupBy`,

diff --git a/cmp/grouping/GroupingChooserModel.ts b/cmp/grouping/GroupingChooserModel.ts
@@ -293,7 +293,9 @@ export class GroupingChooserModel extends HoistModel {
         ...rootPersistWith
     }: GroupingChooserPersistOptions) {
         if (persistValue) {
-            const persistWith = isObject(persistValue) ? persistValue : rootPersistWith;
+            const persistWith = isObject(persistValue)
+                ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistValue)
+                : rootPersistWith;
             PersistenceProvider.create({
                 persistOptions: {
                     path: `${path}.value`,
@@ -308,7 +310,9 @@ export class GroupingChooserModel extends HoistModel {
         }
 
         if (persistFavorites) {
-            const persistWith = isObject(persistFavorites) ? persistFavorites : rootPersistWith,
+            const persistWith = isObject(persistFavorites)
+                    ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistFavorites)
+                    : rootPersistWith,
                 provider = PersistenceProvider.create({
                     persistOptions: {
                         path: `${path}.favorites`,

diff --git a/cmp/zoneGrid/impl/InitPersist.ts b/cmp/zoneGrid/impl/InitPersist.ts
@@ -24,7 +24,9 @@ export function initPersist(
     }: ZoneGridModelPersistOptions
 ) {
     if (persistMappings) {
-        const persistWith = isObject(persistMappings) ? persistMappings : rootPersistWith;
+        const persistWith = isObject(persistMappings)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistMappings)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.mappings`,
@@ -39,7 +41,9 @@ export function initPersist(
     }
 
     if (persistGrouping) {
-        const persistWith = isObject(persistGrouping) ? persistGrouping : rootPersistWith;
+        const persistWith = isObject(persistGrouping)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistGrouping)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.groupBy`,
@@ -54,7 +58,9 @@ export function initPersist(
     }
 
     if (persistSort) {
-        const persistWith = isObject(persistSort) ? persistSort : rootPersistWith;
+        const persistWith = isObject(persistSort)
+            ? PersistenceProvider.mergePersistOptions(rootPersistWith, persistSort)
+            : rootPersistWith;
         PersistenceProvider.create({
             persistOptions: {
                 path: `${path}.sortBy`,

diff --git a/core/HoistBase.ts b/core/HoistBase.ts
@@ -261,8 +261,7 @@ export abstract class HoistBase {
         PersistenceProvider.create({
             persistOptions: {
                 path: property,
-                ...this.persistWith,
-                ...options
+                ...PersistenceProvider.mergePersistOptions(this.persistWith, options)
             },
             owner: this,
             target: {

diff --git a/core/HoistBaseDecorators.ts b/core/HoistBaseDecorators.ts
@@ -81,7 +81,10 @@ function createPersistDescriptor(
         // codeValue undefined if no initial in-code value provided, otherwise call to get initial value.
         ret = codeValue?.call(this);
 
-        const persistOptions = {path: property, ...this.persistWith, ...options};
+        const persistOptions = {
+            path: property,
+            ...PersistenceProvider.mergePersistOptions(this.persistWith, options)
+        };
         PersistenceProvider.create({
             persistOptions,
             owner: this,

diff --git a/core/persist/PersistenceProvider.ts b/core/persist/PersistenceProvider.ts
@@ -8,12 +8,14 @@
 import {logDebug, logError, throwIf} from '@xh/hoist/utils/js';
 import {
     cloneDeep,
+    compact,
     debounce as lodashDebounce,
     get,
     isEmpty,
     isNumber,
     isString,
     isUndefined,
+    omit,
     set,
     toPath
 } from 'lodash';
@@ -91,6 +93,35 @@ export abstract class PersistenceProvider<S = any> {
         }
     }
 
+    /**
+     * Merge PersistOptions, respecting provider types, with later options overriding earlier ones.
+     */
+    static mergePersistOptions(
+        defaults: PersistOptions,
+        ...overrides: PersistOptions[]
+    ): PersistOptions {
+        const TYPE_RELATED_KEYS = [
+            'type',
+            'prefKey',
+            'localStorageKey',
+            'sessionStorageKey',
+            'dashViewModel',
+            'viewManagerModel',
+            'getData',
+            'setData'
+        ];
+        return compact(overrides).reduce(
+            (ret, override) =>
+                TYPE_RELATED_KEYS.some(key => override[key])
+                    ? {
+                          ...omit(ret, ...TYPE_RELATED_KEYS),
+                          ...override
+                      }
+                    : {...ret, ...override},
+            defaults
+        );
+    }
+
     /** Read persisted state at this provider's path. */
     read(): PersistableState<S> {
         const state = get(this.readRaw(), this.path);