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

BREAKING CHANGE: storage migration #11

Merged
merged 8 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,12 @@ function* handleFetchLandAmountRequest(action: FetchLandAmountRequestAction) {

## Storage

The storage module allows you to save parts of the redux store in localStorage to make them persistent.
This module is required to use other modules like `Transaction` and `Translation`.
The storage module allows you to save parts of the redux store in localStorage to make them persistent and migrate it from different versions without loosing it.
This module is required to use other modules like `Transaction`, `Translation`, `Wallet` and `Storage`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Storage module is irrelevant in this enumeration (this is already the Storage module)


### Installation

You need to add a middleware and a two reducers to your dApp.
You need to add a middleware and two reducers to your dApp.

**Middleware**:

Expand All @@ -305,15 +305,17 @@ You will need to create a `storageMiddleware` and add apply it along with your o
// store.ts
import { applyMiddleware, compose, createStore } from 'redux'
import { createStorageMiddleware } from 'decentraland-dapps/dist/modules/storage/middleware'
import { migrations } from './migrations'

const composeEnhancers =
(window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose

const { storageMiddleware, loadStorageMiddleware } = createStorageMiddleware(
'storage-key', // this is the key used to save the state in localStorage (required)
[], // array of paths from state to be persisted (optional)
[] // array of actions types that will trigger a SAVE (optional)
)
const { storageMiddleware, loadStorageMiddleware } = createStorageMiddleware({
storageKey: 'storage-key' // this is the key used to save the state in localStorage (required)
paths: [] // array of paths from state to be persisted (optional)
actions: [] // array of actions types that will trigger a SAVE (optional)
migrations: migrations // migration object that will migrate your localstorage
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be marked as (optional) too

})

const middleware = applyMiddleware(
// your other middlewares
Expand All @@ -325,6 +327,30 @@ const store = createStore(rootReducer, enhancer)
loadStorageMiddleware(store)
```

**Migrations**:

`migrations` looks like

`migrations.ts`:

```ts
export const migrations = {
1: migrateToVersion1(data),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the 1 migration ever exist? doesn't it start from 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, good catch!

2: migrateToVersion2(data),
3: migrateToVersion3(data)
}
```

Where every `key` represent a migration and every `method` should return the new localstorage data:

```ts
function migrateToVersion1(data) {
return omit(data, 'translations')
}
```

You don't need to care about updating the version of the migration because it will be set automatically.

**Reducer**:

You will need to add `storageReducer` as `storage` to your `rootReducer` and then wrap the whole reducer with `storageReducerWrapper`
Expand All @@ -347,7 +373,7 @@ export const rootReducer = storageReducerWrapper(

### Advanced Usage

This module is necessary to use other modules like `Transaction` or `Translation`, but you can also use it to make other parts of your dApp's state persistent
This module is necessary to use other modules like `Transaction`, `Translation`, `Wallet` and `Storage`, but you can also use it to make other parts of your dApp's state persistent
Copy link
Contributor

@cazala cazala Sep 17, 2018

Choose a reason for hiding this comment

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

Same as the comment above #11 (comment)


<details><summary>Learn More</summary>
<p>
Expand Down
62 changes: 62 additions & 0 deletions src/lib/localStorage.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { expect } from 'chai'
import { Migrations } from './types'
import {
hasLocalStorage,
migrateStorage,
getLocalStorage
} from './localStorage'
declare var global: any
let fakeStore = {}
global.window = {}

describe('localStorage', function() {
const migrations: Migrations<any> = {
'1': (data: any) => data,
'2': (data: any) => data
}

beforeEach(function() {
fakeStore = {}
global.window['localStorage'] = {
getItem: (key: string) => fakeStore[key],
setItem: (key: string, value: string) => (fakeStore[key] = value),
removeItem: (key: string) => delete fakeStore[key]
}
})

describe('hasLocalStorage', function() {
it('should return false if localStorage is not available', function() {
delete global.window['localStorage']
expect(hasLocalStorage()).to.equal(false)
})
it('should return true if localStorage is available', function() {
expect(hasLocalStorage()).to.equal(true)
})
})

describe('migrateStorage', function() {
it('should migrate', function() {
const key = 'key'
const localStorage = getLocalStorage()
localStorage.setItem(key, JSON.stringify('{}'))
let data = JSON.parse(localStorage.getItem(key) as string)
expect(data.storage).to.equal(undefined)
migrateStorage(key, migrations)
data = JSON.parse(localStorage.getItem(key) as string)
expect(data.storage.version).to.equal(2)
})

it('should not migrate if there is no migrations left', function() {
const key = 'key'
const localStorage = getLocalStorage()
localStorage.setItem(key, JSON.stringify('{}'))
let data = JSON.parse(localStorage.getItem(key) as string)
expect(data.storage).to.equal(undefined)
migrateStorage(key, migrations)
data = JSON.parse(localStorage.getItem(key) as string)
expect(data.storage.version).to.equal(2)
migrateStorage(key, migrations)
expect(data.storage.version).to.equal(2)
})
})
})
36 changes: 33 additions & 3 deletions src/lib/localStorage.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Migrations, LocalStorage } from './types'

export function hasLocalStorage(): boolean {
try {
// https://gist.github.com/paulirish/5558557
Expand All @@ -11,6 +13,34 @@ export function hasLocalStorage(): boolean {
}
}

export const localStorage = hasLocalStorage()
? window.localStorage
: { getItem: () => null, setItem: () => null, removeItem: () => null }
export function getLocalStorage(): LocalStorage {
return hasLocalStorage()
? window.localStorage
: {
getItem: () => null,
setItem: () => null,
removeItem: () => null
}
}

export function migrateStorage<T>(key: string, migrations: Migrations<T>) {
let version = 1
const localStorage = getLocalStorage()
const dataString = localStorage.getItem(key)
if (dataString) {
const data = JSON.parse(dataString as string)

if (data.storage) {
version = parseInt(data.storage.version || 0, 10) + 1
}

while (migrations[version]) {
const newData = migrations[version](data)
localStorage.setItem(
key,
JSON.stringify({ ...(newData as Object), storage: { version } })
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this line is getting rid of the loading: true property that comes with the initial state of the storage reducer (see this line https://github.com/decentraland/decentraland-dapps/pull/11/files#diff-8544c74e5fd9e4b18f35209ce5efd2beR12) I dunno if that breaks anything, probably @nicosantangelo knows better than myself about this, but it seems like this should rather be:

JSON.stringify({ ...(newData as Object), storage: { ...newData.storage, version } })

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not a big deal because the loading will typically be set after creating the middleware, as a part of the redux-storage lib.
That said I think it doesn't hurt adding the spread, just in case

Also, can we get rid of the as Object? Maybe be a bit more specific?

Copy link
Contributor Author

@nachomazzara nachomazzara Sep 16, 2018

Choose a reason for hiding this comment

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

@nicosantangelo any is ok? If we want something more specific we need to pass the type when calling the method. For the built-in localstorage data will be defined internally inside decentraland-dapps but for new types, will need to be defined outside

@cazala I guess we are not picking the loading property from the store (I could check it again). We only use version https://github.com/decentraland/decentraland-dapps/pull/11/files#diff-dc8e3a1c851df38886297332fa450dc2R40

)
version++
}
}
}
10 changes: 10 additions & 0 deletions src/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,13 @@ export type ModelByAddress<T extends AddressModel> = DataByKey<T>

export type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>
export type Overwrite<T1, T2> = Pick<T1, Exclude<keyof T1, keyof T2>> & T2

export interface Migrations<T> {
[key: string]: (data: T) => T
}

export interface LocalStorage {
getItem: (key?: string) => string | null
setItem: (key?: string, value?: string) => void | null
removeItem: (key?: string) => void | null
}
14 changes: 8 additions & 6 deletions src/modules/storage/middleware.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import * as storage from 'redux-storage'
import createStorageEngine from 'redux-storage-engine-localstorage'
import filter from 'redux-storage-decorator-filter'
import { hasLocalStorage } from '../../lib/localStorage'
import { hasLocalStorage, migrateStorage } from '../../lib/localStorage'
import { disabledMiddleware } from '../../lib/disabledMiddleware'
import { STORAGE_LOAD } from './actions'
import { StorageMiddleware } from './types'
import {
CHANGE_LOCALE,
FETCH_TRANSLATIONS_REQUEST,
Expand All @@ -19,23 +20,24 @@ import {
const disabledLoad = (store: any) =>
setTimeout(() => store.dispatch({ type: STORAGE_LOAD, payload: {} }))

export function createStorageMiddleware(
storageKey: string,
paths: string[] | string[][] = [],
actions: string[] = []
) {
export function createStorageMiddleware<T>(options: StorageMiddleware<T>) {
const { storageKey, migrations = {}, paths = [], actions = [] } = options

if (!hasLocalStorage()) {
return {
storageMiddleware: disabledMiddleware as any,
loadStorageMiddleware: disabledLoad as any
}
}

migrateStorage(storageKey, migrations)

const storageEngine = filter(createStorageEngine(storageKey), [
'transaction',
'translation',
['wallet', 'data', 'locale'],
['wallet', 'data', 'derivationPath'],
['storage', 'version'],
...paths
])
const storageMiddleware: any = storage.createMiddleware(
Expand Down
3 changes: 3 additions & 0 deletions src/modules/storage/reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import * as storage from 'redux-storage'
import { STORAGE_LOAD } from './actions'

export type StorageState = {
version: number
loading: boolean
}

export const INITIAL_STATE: StorageState = {
version: 1,
loading: true
}

Expand All @@ -21,6 +23,7 @@ export function storageReducer(state = INITIAL_STATE, action: AnyAction) {
switch (action.type) {
case STORAGE_LOAD:
return {
...state,
loading: false
}
default:
Expand Down
8 changes: 8 additions & 0 deletions src/modules/storage/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Migrations } from '../../lib/types'

export interface StorageMiddleware<T> {
storageKey: string
paths: string[] | string[][]
actions: string[]
migrations: Migrations<T>
}