From 3962937f3d45962ffe1064a39133681ad1078484 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Thu, 18 May 2017 11:38:03 -0700 Subject: [PATCH] State: opt into persistence (#13359) --- client/state/test/utils.js | 225 +++++++++++++++++++++++++++++++++++ client/state/utils.js | 113 +++++++++++++++++- docs/our-approach-to-data.md | 55 +++++++-- 3 files changed, 383 insertions(+), 10 deletions(-) diff --git a/client/state/test/utils.js b/client/state/test/utils.js index b2bcd2364a5c5..85e6ca71a0839 100644 --- a/client/state/test/utils.js +++ b/client/state/test/utils.js @@ -27,6 +27,8 @@ describe( 'utils', () => { let keyedReducer; let reducer; let withSchemaValidation; + let combineReducersWithPersistence; + let isValidStateWithSchema; useMockery( ( mockery ) => { mockery.registerMock( 'lib/warn', noop ); @@ -36,6 +38,8 @@ describe( 'utils', () => { extendAction, keyedReducer, withSchemaValidation, + combineReducersWithPersistence, + isValidStateWithSchema, } = require( 'state/utils' ) ); } ); @@ -357,7 +361,9 @@ describe( 'utils', () => { describe( '#withSchemaValidation', () => { const load = { type: DESERIALIZE }; + const write = { type: SERIALIZE }; const normal = { type: 'NORMAL' }; + const grow = { type: 'GROW' }; const schema = { type: 'number', minimum: 0, @@ -368,6 +374,33 @@ describe( 'utils', () => { ? state + 1 : state; + const date = ( state = new Date( 0 ), action ) => { + switch ( action.type ) { + case 'GROW': + return new Date( state.getTime() + 1 ); + case SERIALIZE: + return state.getTime(); + case DESERIALIZE: + if ( isValidStateWithSchema( state, schema ) ) { + return new Date( state ); + } + return new Date( 0 ); + default: + return state; + } + }; + date.hasCustomPersistence = true; + + it( 'should return initial state without a schema on SERIALIZE', () => { + const validated = withSchemaValidation( null, age ); + expect( validated( 5, write ) ).to.equal( 0 ); + } ); + + it( 'should return initial state without a schema on DESERIALIZE', () => { + const validated = withSchemaValidation( null, age ); + expect( validated( 5, load ) ).to.equal( 0 ); + } ); + it( 'should invalidate DESERIALIZED state', () => { const validated = withSchemaValidation( schema, age ); @@ -385,5 +418,197 @@ describe( 'utils', () => { expect( validated( 5, load ) ).to.equal( 5 ); } ); + + it( 'actions work as expected with schema', () => { + const validated = withSchemaValidation( schema, age ); + expect( validated( 5, grow ) ).to.equal( 6 ); + } ); + + it( 'actions work as expected without schema', () => { + const validated = withSchemaValidation( null, age ); + expect( validated( 5, grow ) ).to.equal( 6 ); + } ); + + it( 'supports reducers with custom handlers', () => { + const validated = withSchemaValidation( null, date ); + expect( validated( 44, load ).getTime() ).to.equal( 44 ); + expect( validated( -5, load ).getTime() ).to.equal( 0 ); + expect( validated( new Date( 24 ), write ) ).to.equal( 24 ); + expect( validated( new Date( 24 ), grow ).getTime() ).to.equal( 25 ); + } ); + } ); + + describe( '#combineReducersWithPersistence', () => { + const load = { type: DESERIALIZE }; + const write = { type: SERIALIZE }; + const grow = { type: 'GROW' }; + const schema = { + type: 'number', + minimum: 0, + }; + + const age = ( state = 0, action ) => + 'GROW' === action.type + ? state + 1 + : state; + age.schema = schema; + + const height = ( state = 160, action ) => + 'GROW' === action.type + ? state + 1 + : state; + const count = ( state = 1, action ) => + 'GROW' === action.type + ? state + 1 + : state; + + const date = ( state = new Date( 0 ), action ) => { + switch ( action.type ) { + case 'GROW': + return new Date( state.getTime() + 1 ); + case SERIALIZE: + return state.getTime(); + case DESERIALIZE: + if ( isValidStateWithSchema( state, schema ) ) { + return new Date( state ); + } + return new Date( 0 ); + default: + return state; + } + }; + date.hasCustomPersistence = true; + + let reducers; + + beforeEach( () => { + reducers = combineReducersWithPersistence( { + age, + height + } ); + } ); + + const appState = deepFreeze( { + age: 20, + height: 171 + } ); + + it( 'should return initial state on init', () => { + const state = reducers( undefined, write ); + expect( state ).to.eql( { age: 0, height: 160 } ); + } ); + + it( 'should not persist height, because it is missing a schema', () => { + const state = reducers( appState, write ); + expect( state ).to.eql( { age: 20, height: 160 } ); + } ); + + it( 'should not load height, because it is missing a schema', () => { + const state = reducers( appState, load ); + expect( state ).to.eql( { age: 20, height: 160 } ); + } ); + + it( 'should validate age', () => { + const state = reducers( { age: -5 }, load ); + expect( state ).to.eql( { age: 0, height: 160 } ); + } ); + + it( 'actions work as expected', () => { + const state = reducers( appState, grow ); + expect( state ).to.eql( { age: 21, height: 172 } ); + } ); + + it( 'nested reducers work on load', () => { + reducers = combineReducersWithPersistence( { + age, + height, + date + } ); + const nested = combineReducersWithPersistence( { + person: reducers, + count + } ); + const valid = nested( { person: { age: 22, date: 224 } }, load ); + expect( valid ).to.eql( { person: { age: 22, height: 160, date: new Date( 224 ) }, count: 1 } ); + + const invalid = nested( { person: { age: -5, height: 100, date: -5 } }, load ); + expect( invalid ).to.eql( { person: { age: 0, height: 160, date: new Date( 0 ) }, count: 1 } ); + } ); + + it( 'nested reducers work on persist', () => { + reducers = combineReducersWithPersistence( { + age, + height, + date + } ); + const nested = combineReducersWithPersistence( { + person: reducers, + count + } ); + const valid = nested( { person: { age: 22, date: new Date( 224 ) } }, write ); + expect( valid ).to.eql( { person: { age: 22, height: 160, date: 224 }, count: 1 } ); + + const invalid = nested( { person: { age: -5, height: 100, date: new Date( -500 ) } }, write ); + expect( invalid ).to.eql( { person: { age: -5, height: 160, date: -500 }, count: 1 } ); + } ); + + it( 'deeply nested reducers work on load', () => { + reducers = combineReducersWithPersistence( { + age, + height, + date + } ); + const nested = combineReducersWithPersistence( { + person: reducers, + } ); + const veryNested = combineReducersWithPersistence( { + bob: nested, + count + } ); + const valid = veryNested( { bob: { person: { age: 22, date: 224 } }, count: 122 }, load ); + expect( valid ).to.eql( { bob: { person: { age: 22, height: 160, date: new Date( 224 ) } }, count: 1 } ); + + const invalid = veryNested( { bob: { person: { age: -5, height: 22, date: -500 } }, count: 123 }, load ); + expect( invalid ).to.eql( { bob: { person: { age: 0, height: 160, date: new Date( 0 ) } }, count: 1 } ); + } ); + + it( 'deeply nested reducers work on persist', () => { + reducers = combineReducersWithPersistence( { + age, + height, + date + } ); + const nested = combineReducersWithPersistence( { + person: reducers, + } ); + const veryNested = combineReducersWithPersistence( { + bob: nested, + count + } ); + const valid = veryNested( { bob: { person: { age: 22, date: new Date( 234 ) } }, count: 122 }, write ); + expect( valid ).to.eql( { bob: { person: { age: 22, height: 160, date: 234 } }, count: 1 } ); + + const invalid = veryNested( { bob: { person: { age: -5, height: 22, date: new Date( -5 ) } }, count: 123 }, write ); + expect( invalid ).to.eql( { bob: { person: { age: -5, height: 160, date: -5 } }, count: 1 } ); + } ); + + it( 'deeply nested reducers work with reducer with a custom handler', () => { + reducers = combineReducersWithPersistence( { + height, + date + } ); + const nested = combineReducersWithPersistence( { + person: reducers, + } ); + const veryNested = combineReducersWithPersistence( { + bob: nested, + count + } ); + const valid = veryNested( { bob: { person: { date: new Date( 234 ) } }, count: 122 }, write ); + expect( valid ).to.eql( { bob: { person: { height: 160, date: 234 } }, count: 1 } ); + + const invalid = veryNested( { bob: { person: { height: 22, date: new Date( -5 ) } }, count: 123 }, write ); + expect( invalid ).to.eql( { bob: { person: { height: 160, date: -5 } }, count: 1 } ); + } ); } ); } ); diff --git a/client/state/utils.js b/client/state/utils.js index bc45a0babde8a..a65278b6e33a3 100644 --- a/client/state/utils.js +++ b/client/state/utils.js @@ -2,7 +2,8 @@ * External dependencies */ import validator from 'is-my-json-valid'; -import { merge, flow, partialRight } from 'lodash'; +import { merge, flow, partialRight, reduce } from 'lodash'; +import { combineReducers } from 'redux'; /** * Internal dependencies @@ -228,7 +229,7 @@ export function createReducer( initialState = null, customHandlers = {}, schema } /** - * Creates schema-validating reducer + * Creates a schema-validating reducer * * Use this to wrap simple reducers with a schema-based * validation check when loading the initial state from @@ -255,12 +256,38 @@ export function createReducer( initialState = null, customHandlers = {}, schema * age( -5, { type: DESERIALIZE } ) === 0 * age( 23, { type: DESERIALIZE } ) === 23 * + * If no schema is provided, the reducer will return initial state on SERIALIZE + * and DESERIALIZE + * + * @example + * const schema = { type: 'number', minimum: 0 } + * export const age = withSchemaValidation( null, age ) + * + * ageReducer( -5, { type: SERIALIZE } ) === -5 + * age( -5, { type: SERIALIZE } ) === 0 + * age( 23, { type: SERIALIZE } ) === 0 + * age( 23, { type: DESERIALIZE } ) === 0 + * * @param {object} schema JSON-schema description of state * @param {function} reducer normal reducer from ( state, action ) to new state - * @returns {function} wrapped reducer handling validation on DESERIALIZE + * @returns {function} wrapped reducer handling validation on DESERIALIZE and + * returns initial state if no schema is provided on SERIALIZE and DESERIALIZE. */ export const withSchemaValidation = ( schema, reducer ) => ( state, action ) => { + const shouldDispatchAction = reducer.hasSchema || reducer.hasCustomPersistence; + + if ( SERIALIZE === action.type ) { + return schema || shouldDispatchAction ? reducer( state, action ) : reducer( undefined, { type: '@@calypso/INIT' } ); + } if ( DESERIALIZE === action.type ) { + if ( shouldDispatchAction ) { + return reducer( state, action ); + } + + if ( ! schema ) { + return reducer( undefined, { type: '@@calypso/INIT' } ); + } + return state && isValidStateWithSchema( state, schema ) ? state : reducer( undefined, { type: '@@calypso/INIT' } ); @@ -268,3 +295,83 @@ export const withSchemaValidation = ( schema, reducer ) => ( state, action ) => return reducer( state, action ); }; + +/** + * Returns a single reducing function that ensures that persistence is opt-in. + * If you don't need state to be stored, simply use this method instead of + * combineReducers from redux. This function uses the same interface. + * + * To mark that a reducer's state should be persisted, add the related JSON + * schema as a property on the reducer. + * + * @example + * const age = ( state = 0, action ) => + * GROW === action.type + * ? state + 1 + * : state + * const height = ( state = 150, action ) => + * GROW === action.type + * ? state + 1 + * : state + * const schema = { type: 'number', minimum: 0 }; + * + * age.schema = schema; + * + * const combinedReducer = combineReducersWithPersistence( { + * age, + * height + * } ); + * + * combinedReducer( { age: -5, height: -5 } ), { type: DESERIALIZE } ); // { age: 0, height: 150 }; + * combinedReducer( { age: -5, height: 123 } ), { type: DESERIALIZE } ); // { age: 0, height: 150 }; + * combinedReducer( { age: 6, height: 123 } ), { type: DESERIALIZE } ); // { age: 6, height: 150 }; + * combinedReducer( { age: 6, height: 123 } ), { type: SERIALIZE } ); // { age: 6, height: 150 }; + * combinedReducer( { age: 6, height: 123 } ), { type: GROW } ); // { age: 7, height: 124 }; + * + * If the reducer explicitly handles the SERIALIZE and DESERIALZE actions, set + * the hasCustomPersistence property to true on the reducer. + * + * @example + * const date = ( state = new Date( 0 ), action ) => { + * switch ( action.type ) { + * case 'GROW': + * return new Date( state.getTime() + 1 ); + * case SERIALIZE: + * return state.getTime(); + * case DESERIALIZE: + * if ( isValidStateWithSchema( state, schema ) ) { + * return new Date( state ); + * } + * return new Date( 0 ); + * default: + * return state; + * } + * }; + * date.hasCustomPersistence = true; + * + * const combinedReducer = combineReducersWithPersistence( { + * date, + * height + * } ); + * + * combinedReducer( { date: -5, height: -5 } ), { type: DESERIALIZE } ); // { date: new Date( 0 ), height: 150 }; + * combinedReducer( { date: -5, height: 123 } ), { type: DESERIALIZE } ); // { date: new Date( 0 ), height: 150 }; + * combinedReducer( { date: 6, height: 123 } ), { type: DESERIALIZE } ); // { date: new Date( 6 ), height: 150 }; + * combinedReducer( { date: new Date( 6 ), height: 123 } ), { type: SERIALIZE } ); // { date: 6, height: 150 }; + * combinedReducer( { date: new Date( 6 ), height: 123 } ), { type: GROW } ); // { date: new Date( 7 ), height: 124 }; + * + * @param {object} reducers - object containing the reducers to merge + * @returns {function} - Returns the combined reducer function + */ +export function combineReducersWithPersistence( reducers ) { + const [ validatedReducers, validatedReducersHasSchema ] = reduce( reducers, ( [ validated, hasSchema ], next, key ) => { + const { schema } = next; + return [ + { ...validated, [ key ]: withSchemaValidation( schema, next ) }, + hasSchema || !! schema || next.hasSchema || next.hasCustomPersistence + ]; + }, [ {}, false ] ); + const combined = combineReducers( validatedReducers ); + combined.hasSchema = validatedReducersHasSchema; + return combined; +} diff --git a/docs/our-approach-to-data.md b/docs/our-approach-to-data.md index feb3925adb49b..1f20d89445f21 100644 --- a/docs/our-approach-to-data.md +++ b/docs/our-approach-to-data.md @@ -332,9 +332,9 @@ export const items = createReducer( defaultState, { [DESERIALIZE]: state => fromJS( state ) } ); ``` -If your reducer state is already a plain object, you may choose to omit `SERIALIZE` and `DESERIALIZE` handlers, or -simply define them as returning the current state. However, please note that the subtree can still see errors from -changing data shapes, as described below. +If your reducer state can be serialized by the browser without additional work (eg a plain object, string or boolean), +the `SERIALIZE` and `DESERIALIZE` handlers are not needed. However, please note that the subtree can still see errors +from changing data shapes, as described below. #### Problem: Data shapes change over time ( [#3101](https://github.com/Automattic/wp-calypso/pull/3101) ) @@ -380,6 +380,7 @@ export const itemsSchema = { A JSON Schema must be provided if the subtree chooses to persist state. If we find that our persisted data doesn't match our described data shape, we should throw it out and rebuild that section of the tree with our default state. + When using `createReducer` util you can pass a schema as a third param and all that will be handled for you. ```javascript export const items = createReducer( defaultState, { @@ -387,9 +388,9 @@ export const items = createReducer( defaultState, { }, itemsSchema ); ``` -When you are not satisfied with the default handling you are also encouraged to implement your own `SERIALIZE` and -`DESERIALIZE` action handlers in your reducers to customize data persistence. Always consider using schema to avoid -errors when data shape changes. +If you are not satisfied with the default handling, it is possible to implement your own `SERIALIZE` and +`DESERIALIZE` action handlers in your reducers to customize data persistence. Always use a schema with your custom +handlers to avoid data shape errors. ### Not persisting data @@ -398,10 +399,50 @@ values are persisted we will not be able to reliably tell when the application i If persisting state causes application errors, opting out of persistence is straightforward: in the `createReducer` util provide only the default state value as a first param and don't provide a schema as a third param. Behind the scenes -data is never going to be persisted and always regenerated with default value. In this example, it happens to be `'CHECKING'` +data is never going to be persisted and is always regenerated with default value. In this example, it happens to be `'CHECKING'` ```javascript export const connectionState = createReducer( 'CHECKING', { [CONNECTION_LOST]: () => 'OFFLINE', [CONNECTION_RESTORED]: () => 'ONLINE' } ); ``` + +### Opt-in to Persistence ( [#13359](https://github.com/Automattic/wp-calypso/pull/13359) ) + +Currently reducers are persisted by default if no handlers are given for `SERIALIZE` and `DESERIALIZE`. This is a major +problem since many people may not realize that this is happening, and we can run into the data shape errors as +noted above. + +In the **future** we can opt-in to persistence by adding a schema as a property on the reducer. We do this by combining +all of our reducers using `combineReducersWithPersistence` at every level of the tree instead of [combineReducers](http://redux.js.org/docs/api/combineReducers.html). +Each reducer is then wrapped with `withSchemaValidation` which returns a wrapped reducer that validates on `DESERIALZE` +if a schema is present and returns initial state on both `SERIALIZE` and `DESERIALZE` if a schema is not present. + +To opt-out of persistence we combine the reducers without any attached schema. +```javascript +return combineReducersWithPersistence( { + age, + height, +} ); +``` + +To persist, we add the schema as a property on the reducer: +```javascript +age.schema = ageSchema; +return combineReducersWithPersistence( { + age, + height, +} ); +``` + +For a reducer that has custom handlers (needs to perform transforms), we assume the reducer is checking the schema already, +on `DESERIALIZE` so all we need to do is set a boolean bit on the reducer, to ensure that we don't return initial state +incorrectly from the default handling provided by `withSchemaValidation`. +```javascript +date.hasCustomPersistence = true; +return combineReducersWithPersistence( { + age, + height, + date, +} ); +```