Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data: Fix persistence initial state merging behavior #13951

Merged
merged 8 commits into from
Feb 19, 2019
Prev Previous commit
Next Next commit
Data: Deeply merge into persistence default value
  • Loading branch information
aduth committed Feb 19, 2019
commit ba66c856b3c07de39a5c7946cc45f99902fa4e67
1 change: 1 addition & 0 deletions packages/data/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Bug Fix

- Resolves issue in the persistence plugin where passing `persist` as an array of reducer keys would wrongly replace state values for the unpersisted reducer keys.
- Restores a behavior in the persistence plugin where a default state provided as an object will be deeply merged as a base for the persisted value. This allows for a developer to include additional new keys in a persisted value default in future iterations of their store.

## 4.2.0 (2019-01-03)

Expand Down
20 changes: 11 additions & 9 deletions packages/data/src/plugins/persistence/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { flow } from 'lodash';
import { flow, merge, isPlainObject } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -167,16 +167,18 @@ export default function( registry, pluginOptions ) {
// Load from persistence to use as initial state.
let initialState = persistence.get()[ reducerKey ];
if ( initialState !== undefined ) {
// When persisting only a subset of keys from the reducer
// result, ensure that other keys are left intact when
// restoring from persistence.
if ( Array.isArray( options.persist ) ) {
initialState = {
...options.reducer( undefined, {
// For object-like persistence, ensure that:
// - Other keys are left intact when persisting only a subset
// of keys.
// - New keys in what would otherwise be provided as a default
// state are deeply merged as a base for the persisted value.
if ( isPlainObject( initialState ) ) {
initialState = merge(
options.reducer( undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should ensure that this also returns a plain object or undefined to avoid a corrupted persisted state from being loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the expected behavior if we had a plain-object persisted state and a non-plain-object default initial state?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a1469ff, I went with: If there's a mismatch of object-likeness between initial and persisted states, it defers always to the value produced by the original reducer.

But immediately upon pushing the change, I'm second-guessing myself, particularly for the following scenarios:

  • Why defer to the default implementation for only the mismatch on this type, when if they were both non-objects we'd be fine to use the persisted value, regardless whether they're different types (array vs. string vs. number vs. etc)
  • Are there cases where we'd want to allow some "empty" initial state, but still prefer the persisted value? Take, for example, the changed test case "should load a persisted value as initialState" in a1469ff where the reducer returns null as the default initial state

With consideration of these points, maybe it should be that the persisted value always "wins", and merges with the default implementation value if and only if both it and the persisted value are objects.

The merging behavior in general feels a bit odd to me, though I understand the need for it in introducing new keys to something like a preferences object over time. The only other ideas that came to mind to support this were things more explicitly defined as "upgrading" behaviors†. There's some overhead in trying to manage this, which I think the auto-merging handles reasonably well on its own.

† Things like: Handling the "persistence restore" action type from the reducer, checking the value and adding missing keys. Or defining a validation function which returns whether a persisted value should be considered valid for use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also was thinking to defer to the reducer value and consider the persisted one as broken but on a second though reducers can return different shapes so I guess it's fine to override the state entirely with the persisted value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also was thinking to defer to the reducer value and consider the persisted one as broken but on a second though reducers can return different shapes so I guess it's fine to override the state entirely with the persisted value.

And should a need arise that we need to explicitly forget some persisted state, I think at that point we could consider introducing one of the other options I mentioned in my previous comment about validating or overriding the persisted input.

type: '@@WP/PERSISTENCE_RESTORE',
} ),
...initialState,
};
initialState,
);
}

// Since it's otherwise not possible to assign an initial state
Expand Down
53 changes: 53 additions & 0 deletions packages/data/src/plugins/persistence/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,59 @@ describe( 'persistence', () => {
expect( registry.select( 'test' ).getState() ).toEqual( { a: 1, b: null } );
} );

it( 'should merge persisted value with default if object-like', () => {
const DEFAULT_STATE = { preferences: { useFoo: true, useBar: true } };

registry = createRegistry().use( plugin, {
storage: {
getItem: () => JSON.stringify( {
test: {
preferences: {
useFoo: false,
},
},
} ),
setItem() {},
},
} );

registry.registerStore( 'test', {
persist: [ 'preferences' ],
reducer: ( state = DEFAULT_STATE ) => state,
selectors: {
getState: ( state ) => state,
},
} );

expect( registry.select( 'test' ).getState() ).toEqual( {
preferences: {
useFoo: false,
useBar: true,
},
} );
} );

it( 'should be reasonably tolerant to a non-object persisted state', () => {
registry = createRegistry().use( plugin, {
storage: {
getItem: () => JSON.stringify( {
test: 1,
} ),
setItem() {},
},
} );

registry.registerStore( 'test', {
persist: true,
reducer: ( state = null ) => state,
selectors: {
getState: ( state ) => state,
},
} );

expect( registry.select( 'test' ).getState() ).toBe( 1 );
} );

it( 'override values passed to registerStore', () => {
const options = { persist: true, reducer() {} };

Expand Down