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
11 changes: 11 additions & 0 deletions packages/data/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
## 4.3.0 (Unreleased)

### Enhancements

- The `registerStore` function now accepts an optional `initialState` option value.

### 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)

### Enhancements
Expand Down
4 changes: 4 additions & 0 deletions packages/data/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ The `controls` option should be passed as an object where each key is the name o

Refer to the [documentation of `@wordpress/redux-routine`](/packages/redux-routine/README.md) for more information.

### `initialState`

An optional preloaded initial state for the store. You may use this to restore some serialized state value or a state generated server-side.

## Data Access and Manipulation

It is very rare that you should access store methods directly. Instead, the following suite of functions and higher-order components is provided for the most common data access and manipulation needs.
Expand Down
18 changes: 10 additions & 8 deletions packages/data/src/namespace-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import createResolversCacheMiddleware from './resolvers-cache-middleware';
*/
export default function createNamespace( key, options, registry ) {
const reducer = options.reducer;
const store = createReduxStore( reducer, key, registry );
const store = createReduxStore( key, options, registry );

let selectors, actions, resolvers;
if ( options.actions ) {
Expand Down Expand Up @@ -76,21 +76,23 @@ export default function createNamespace( key, options, registry ) {
/**
* Creates a redux store for a namespace.
*
* @param {Function} reducer Root reducer for redux store.
* @param {string} key Part of the state shape to register the
* selectors for.
* @param {Object} registry Registry reference, for resolver enhancer support.
* @return {Object} Newly created redux store.
* @param {string} key Part of the state shape to register the
* selectors for.
* @param {Object} options Registered store options.
* @param {Object} registry Registry reference, for resolver enhancer support.
*
* @return {Object} Newly created redux store.
*/
function createReduxStore( reducer, key, registry ) {
function createReduxStore( key, options, registry ) {
const enhancers = [
applyMiddleware( createResolversCacheMiddleware( registry, key ), promise ),
];
if ( typeof window !== 'undefined' && window.__REDUX_DEVTOOLS_EXTENSION__ ) {
enhancers.push( window.__REDUX_DEVTOOLS_EXTENSION__( { name: key, instanceId: key } ) );
}

return createStore( reducer, flowRight( enhancers ) );
const { reducer, initialState } = options;
return createStore( reducer, initialState, flowRight( enhancers ) );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/data/src/plugins/persistence/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Persistence Plugin

The persistence plugin enhances a registry to enable registered stores to opt in to persistent storage.

By default, persistence occurs by [`localStorage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage). This can be changed using the [`setStorage` function](#api) defined on the plugin. Unless set otherwise, state will be persisted on the `WP_DATA` key in storage.
By default, persistence occurs by [`localStorage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage). In environments where `localStorage` is not available, it will gracefully fall back to an in-memory object storage which will not persist between sessions. You can provide your own storage implementation by providing the [`storage` option](#options). Unless set otherwise, state will be persisted on the `WP_DATA` key in storage.

## Usage

Expand Down
39 changes: 19 additions & 20 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 @@ -34,20 +34,6 @@ const DEFAULT_STORAGE = defaultStorage;
*/
const DEFAULT_STORAGE_KEY = 'WP_DATA';

/**
* Higher-order reducer to provides an initial value when state is undefined.
*
* @param {Function} reducer Original reducer.
* @param {*} initialState Value to use as initial state.
*
* @return {Function} Enhanced reducer.
*/
export function withInitialState( reducer, initialState ) {
return ( state = initialState, action ) => {
return reducer( state, action );
};
}

/**
* Higher-order reducer which invokes the original reducer only if state is
* inequal from that of the action's `nextState` property, otherwise returning
Expand Down Expand Up @@ -178,12 +164,25 @@ export default function( registry, pluginOptions ) {
return registry.registerStore( reducerKey, options );
}

const initialState = persistence.get()[ reducerKey ];
// Load from persistence to use as initial state.
let initialState = persistence.get()[ reducerKey ];
if ( initialState !== 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,
);
}

options = {
...options,
reducer: withInitialState( options.reducer, initialState ),
};
options = { ...options, initialState };
}

const store = registry.registerStore( reducerKey, options );

Expand Down
112 changes: 93 additions & 19 deletions packages/data/src/plugins/persistence/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import plugin, {
createPersistenceInterface,
withInitialState,
withLazySameState,
} from '../';
import objectStorage from '../storage/object';
Expand Down Expand Up @@ -37,6 +36,99 @@ describe( 'persistence', () => {
registry.registerStore( 'test', options );
} );

it( 'should load a persisted value as initialState', () => {
registry = createRegistry().use( plugin, {
storage: {
getItem: () => JSON.stringify( { test: { a: 1 } } ),
setItem() {},
},
} );

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

expect( registry.select( 'test' ).getState() ).toEqual( { a: 1 } );
} );

it( 'should load a persisted subset value as initialState', () => {
const DEFAULT_STATE = { a: null, b: null };

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

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

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 All @@ -46,8 +138,6 @@ describe( 'persistence', () => {
persist: true,
reducer: expect.any( Function ),
} );
// Replaced reducer:
expect( originalRegisterStore.mock.calls[ 0 ][ 1 ].reducer ).not.toBe( options.reducer );
} );

it( 'should not persist if option not passed', () => {
Expand Down Expand Up @@ -208,22 +298,6 @@ describe( 'persistence', () => {
} );
} );

describe( 'withInitialState', () => {
it( 'should return a reducer function', () => {
const reducer = ( state = 1 ) => state;
const enhanced = withInitialState( reducer );

expect( enhanced() ).toBe( 1 );
} );

it( 'should assign a default state by argument', () => {
const reducer = ( state = 1 ) => state;
const enhanced = withInitialState( reducer, 2 );

expect( enhanced() ).toBe( 2 );
} );
} );

describe( 'withLazySameState', () => {
it( 'should call the original reducer if action.nextState differs from state', () => {
const reducer = jest.fn().mockImplementation( ( state, action ) => action.nextState );
Expand Down
3 changes: 2 additions & 1 deletion packages/data/src/test/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ describe( 'createRegistry', () => {
describe( 'registerStore', () => {
it( 'should be shorthand for reducer, actions, selectors registration', () => {
const store = registry.registerStore( 'butcher', {
reducer( state = { ribs: 6, chicken: 4 }, action ) {
reducer( state = {}, action ) {
switch ( action.type ) {
case 'sale':
return {
Expand All @@ -162,6 +162,7 @@ describe( 'createRegistry', () => {

return state;
},
initialState: { ribs: 6, chicken: 4 },
selectors: {
getPrice: ( state, meat ) => state[ meat ],
},
Expand Down