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

How to merge slicer subset into initial state #2

Closed
klaasman opened this issue Aug 4, 2015 · 21 comments
Closed

How to merge slicer subset into initial state #2

klaasman opened this issue Aug 4, 2015 · 21 comments

Comments

@klaasman
Copy link

klaasman commented Aug 4, 2015

I've defined a slicer to specifiy a subset which is saved to localStorage.
Inside one of my reducers I've got an initial state defined, comparable to this example.

const initialState = {
  list: [],
  favorites: [],
  isLoading: false
}

// the default value `initialState` won't be used anymore when there's 
// already a subset of the initialstate define in localstorage
export default function jobs (state = initialState, action) {
  const reduceFn = actionsMap[action.type]
  if (!reduceFn) return state
  return Object.assign({}, state, reduceFn(state, action))
}

My reduce won't consume the initial state anymore, since there's another initial state passed to this reducer, together with the action named "@@redux/INIT.

How should I merge both the initialState and the state coming from local storage?

@jdlehman
Copy link
Contributor

jdlehman commented Aug 4, 2015

Hmm, the challenge here is that in your jobs reducer, initialState is only used if state is undefined, but you have state coming in from localStorage like you mentioned.

You could do something like the following to merge the initialState with any existing localStorage state on initialization.

export default function jobs (state = initialState, action) {
  switch (action.type) {
     case '@@redux/INIT':
      const newState = {...state, ...initialState};
     return newState;
     // etc ...
  }
}

@jdlehman
Copy link
Contributor

jdlehman commented Aug 4, 2015

Note that I am not sure if hooking into redux's INIT action type is a good practice or not, but I assume it exists for a reason.

@klaasman
Copy link
Author

klaasman commented Aug 4, 2015

Yes, for now I've done something similar, although I'm not sure if this is the correct way: in this issue they mention "Handling @@INIT manually will break hot reloading". Could this potentially be an issue?

@jdlehman
Copy link
Contributor

jdlehman commented Aug 4, 2015

I thought just as much. Thanks for linking the issue, I had been wondering about the status on that.

To ensure your code works in the future (and does not break hot reloading), you definitely should not manually hook into this action. Some of the solutions provided in that redux issue could be used. You could also take advantage of the deserialize config option that we provide in this repo.

// this object would hold all the initial states for each reducer
// you would probably want to be able to import the initialState from each reducer 
// to ensure the logic lives in one place only (in each reducer probably)
const initialStates = {
   reducer1: {},
   reducer2: {},
   ...
};

// set up config for `persistState`
const config = {
  deserialize: (stateStr) => {
     // parse string from local storage
    let state = JSON.parse(stateStr);

    // merge localStorage state with initialStates
    Object.keys(state).forEach((key) => {
      state[key] = {...state[key], ...initialState[key]};
    });
    return state;
  }
};

const myPersistState = persistState(paths, config);

@klaasman
Copy link
Author

klaasman commented Aug 4, 2015

Thanks a lot, interesting approach and I'll dive into it 👍

@klaasman klaasman closed this as completed Aug 4, 2015
@elgerlambert
Copy link
Owner

Hi klaasman,

I just reopened this issue. Current suggestions might provide a workaround, but I’d like to discuss it further.

Could you share your custom slicer? Am I correct in thinking that you’re only persisting jobs.favorites or jobs.list and not the other one?

The first thing that comes to my mind, is to split your reducer into two, for guides/examples of different ways to do this have a look here: http://gaearon.github.io/redux/docs/basics/Reducers.html. That way you can provide each reducer with it's own state = [] default.

@elgerlambert elgerlambert reopened this Aug 5, 2015
@klaasman
Copy link
Author

klaasman commented Aug 5, 2015

Thats good to hear :-)

Your assumption is correct, my slicer currently looks like this:

function storageSlicer () {
  return (state) => ({
    jobs: {
      favorites: state.jobs.favorites || []
    }
  })
}

Initially the store should be populated with the favorites coming from localStorage, while maintaining the shape of the store.

I've thought about splitting up reducers, but, since it's possible to supply this module with 'root' paths, doesn't that somehow make the slicer unnecessary, at least for deeply nested properties?

To be honest, the favorites had it's own reducer when I started implementing Redux, however it resulted in slightly more boilerplate in other places of the application (e.g. a smart component which needed both actions for fetching jobs as for favoriting jobs, this led to call bindActionCreators for both jobsActions as for favoritesActions)

Would you suggest, in case of a subset of the store coming from localStorage, to split up the reducers again?

@elgerlambert
Copy link
Owner

There are a number of options at your disposal.

Option 1 is to pass in your initialState when calling your "enhanced" createStore:

createPersistentStore(reducer, {jobs: {list: [], favorites: [], isLoading: false}})

The initialState passed to createStore and your persistedState are merged during initialisation, so this should give you the result you're looking for. Everything is used as it is intended so it's relatively clean, but it still feels like a workaround, you'd probably rather keep this contained within your reducer.

Options 2 is to check whether state.list is defined in your reducer.

export default function jobs (state = initialState, action) {
  if (!state.list) state.list = []
  ...
}

Everything is contained in your reducer, but it's hacky. This added line is going to look super redundant and someone else (or you yourself at a later time) is going to question why it's there, perhaps even remove it and create a bug.

Option 3 split your reducers further.
I still feel this is (potentially) the best option. It's slightly harder to give you concrete advice since the internals of your reducer are abstracted away by actionMap in your first post, the important thing to realise though, is that you can nest your reducer functions. So when I say split it doesn't mean you have to flatten your store state to e.g. {favorites: [], jobs: []}, you could do something like the following:

export default function jobs (state = initialState, action) {
  if (action.FETCH_JOBS_DATA_SUCCESS) {
    return action.payload // e.g. {favorites: [fetched data], list: [fetched data]}
  } else {
    return {
      favorites: favorites(state.favorites, action)
      list: list(state.list, action)
    }
  }
}

This might not be a great code snippet, the point I'm trying to illustrate though, is that (further) splitting of your reducer can be done in a way that your components don't have to know about. Each function favorites and list can then have it's own state = [] argument as mentioned previously.

p.s. I don't fully understand your remark about needing separate actions to fetch both jobs and favorites if they each have their own totally separate reducer, I wonder if there might be other things that could be improved, but then we're really getting into the implementation details of your application and redux in general..

@elgerlambert
Copy link
Owner

I've thought about splitting up reducers, but, since it's possible to supply this module with 'root' paths, doesn't that somehow make the slicer unnecessary, at least for deeply nested properties?

You're right, if you end up flattening your store state your custom slicer function wouldn't be needed anymore - which would be a good thing! Though, like I said in my previous comment, splitting your reducers further doesn't have to mean flattening your store state.

On a side note, I'm looking into supporting nested paths, which would remove the need for a custom slicer in this scenario and instead allow you to pass in something like jobs.favorites as path argument.

@klaasman
Copy link
Author

klaasman commented Aug 6, 2015

FYI, my reducer looks currently like this:
(the previous favorites have been renamed to favoriteIds, the favorites property will be populated with actual job-data (we only store job-ids of jobs in localStorage, we fetch job-details from an API))

import * as constants from '../constants'

const initialState = {
  list: [],
  details: null,
  isLoading: false,
  favoriteIds: [],
  favorites: []
}

const actionsMap = {
  [constants.FETCH_JOBS]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_JOBS_SUCCESS]: (state, action) => ({ isLoading: false, list: action.list }),
  [constants.FETCH_JOBS_FAIL]: (state, action) => ({ isLoading: false }),

  [constants.FETCH_JOB_DETAILS]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_JOB_DETAILS_SUCCESS]: (state, action) => ({ isLoading: false, details: action.details }),
  [constants.FETCH_JOB_DETAILS_FAIL]: (state, action) => ({ isLoading: false }),

  [constants.FETCH_FAVORITES]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_FAVORITES_SUCCESS]: (state, action) => ({ isLoading: false, favorites: action.favorites }),
  [constants.FETCH_FAVORITES_FAIL]: (state, action) => ({ isLoading: false }),

  [constants.FAVORITES_ADD]: (state, action) => ({ favoriteIds: state.favoriteIds.concat(action.id) }),
  [constants.FAVORITES_REMOVE]: (state, action) => ({ favoriteIds: state.favoriteIds.filter(id => id !== action.id) })
}

export default function jobs (state, action) {

  // this is currently the 'hack' to preserve the state coming from `localStorage`
  state = {...initialState, ...state}

  const reduceFn = actionsMap[action.type]
  if (!reduceFn) return state
  return {...state, ...reduceFn(state, action)}
}

If I understand it correctly, I should be able to split up reducers as follows:

import * as constants from '../constants'

const initialState = {
  list: [],
  details: null,
  isLoading: false,
  favorites: [],
  favoriteIds: []
}

function favoriteIdsReducer(state = [], action) {
  return action.type === constants.FAVORITES_ADD ? state.concat(action.id) : state.filter(id => id !== action.id)
}

const actionsMap = {
  [constants.FETCH_JOBS]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_JOBS_SUCCESS]: (state, action) => ({ isLoading: false, list: action.list }),
  [constants.FETCH_JOBS_FAIL]: (state, action) => ({ isLoading: false }),

  [constants.FETCH_JOB_DETAILS]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_JOB_DETAILS_SUCCESS]: (state, action) => ({ isLoading: false, details: action.details }),
  [constants.FETCH_JOB_DETAILS_FAIL]: (state, action) => ({ isLoading: false }),

  [constants.FETCH_FAVORITES]: (state, action) => ({ isLoading: true }),
  [constants.FETCH_FAVORITES_SUCCESS]: (state, action) => ({ isLoading: false, favorites: action.favorites }),
  [constants.FETCH_FAVORITES_FAIL]: (state, action) => ({ isLoading: false }),

  // here the `sub`-reducers appear: `favoriteIdsReducer`
  [constants.FAVORITES_ADD]: (state, action) => ({ favoriteIds: favoriteIdsReducer(state.favoriteIds, action) }),
  [constants.FAVORITES_REMOVE]: (state, action) => ({ favoriteIds: favoriteIdsReducer(state.favoriteIds, action) })
}

export default function jobs (state = initialState, action) {
  // some logs I'll mention later
  console.group(action.type)
  console.log(state)
  console.groupEnd()

  const reduceFn = actionsMap[action.type]
  if (!reduceFn) return state

  return {...state, ...reduceFn(state, action)}
}

The app loads as expected when there's no data in localStorage.
However, with the next load of the app there's data written to localStorage:

  • data without saved jobs: {"jobs":{"favoriteIds":[]}}
  • data with saved jobs: {"jobs":{"favoriteIds":["02032-144516598153","02032-372767868482"]}})

This load with data (both with as without saved jobs) results in the following logs:
image
(as logged from within function jobs () { ... }, as I've mentioned in the code-block above)

The initial state got completely lost on the moment the state from localstorage came through, however it's bound to a @@redux/INIT action, which I'm not allowed to edit ..

Maybe I'm missing something completely, you should note I'm quite new to this react/flux/redux thing, as in a week or three, still wrapping my head around some patterns :-)

@klaasman
Copy link
Author

klaasman commented Aug 6, 2015

p.s. I don't fully understand your remark about needing separate actions to fetch both jobs and favorites if they each have their own totally separate reducer, I wonder if there might be other things that could be improved, but then we're really getting into the implementation details of your application and redux in general..

They also had there own actions, e.g.

// actions/jobs.js
export function fetchJobs (options) {
  return dispatch => api.fetchJobs().then(list => dispatch({
    type: constants.FETCH_JOBS_SUCCESS,
    list
    // ...
  }))
}

// actions/favorites.js
export function addToFavorites (id) {
  return {
    type: constants.FAVORITES_ADD,
    id
    // ...
  }
}

Somewhere else in a React component we need those actions:

import * as jobActions from '../actions/jobs'
import * as favoritesActions from '../actions/favorites'

// ...

const { fetchJobs } = bindActionCreators(jobActions, dispatch)
const { addToFavorites } = bindActionCreators(favoriteActions, dispatch)

// do something with `fetchJobs()` and `addToFavorites()`

I wanted to prevent to call those bindActionCreators multple times, although it's not that big of an issue.

For the described case in this issue it doesn't really matter and it is indeed an implementation detail. Let's focus on the nested data instead :-)

@elgerlambert
Copy link
Owner

Hey Klaasman,

I just published version 1.0.0-beta1, I recommend checking it out. It takes care of your (original) issue without any workarounds or adjustments!

@klaasman
Copy link
Author

Thanks! I'll take a look at it

@klaasman
Copy link
Author

First, I really like the new abstraction of the adapter 👍
I've implemented v1.x, though I didn't find a way to easily merge the subset of localstorage in my store (which was my original issue).

I might be missing something, but got it working this way:

const REDUX_LOCAL_STORAGE_INIT = 'redux-localstorage/INIT'

const storageSlicer = (slicer) => (storage) => ({
  ...storage,
  put: (key, state, callback) => {
    storage.put(key, slicer(state), callback)
  }
})

const storage = compose(
  storageSlicer(state => ({
    jobs: {
      favoriteIds: state.jobs.favoriteIds
    }
  })),
  adapter(window.localStorage)
)

const mergeLocalStorage = (reducer) => (state, action) => {
  if (action.type === REDUX_LOCAL_STORAGE_INIT) {
    return action.payload ? defaultsDeep({}, state, action.payload) : state
  }
  return reducer(state, action)
}

createPersistentStore(
  applyMiddleware(thunk),
  persistState(storage),
  createStore
)


const wbdoApp = compose(mergeLocalStorage, combineReducers(reducers))
const store = createPersistentStore(wbdoApp)

Any suggestions?

@elgerlambert
Copy link
Owner

Hi Klaasman,

I just published 1.0.0-beta2, which includes support for filtering 'nested.keys'. That'll remove the need for your custom storageSlicer!

What does your defaultsDeep function do? As far as I can tell the default merge strategy should work fine in your case, which means you don't have to handle the redux-localstorage/INIT action yourself, it'll be handled for you. If you grab 1.0.0-beta2 from npm the following should work:

import {compose, applyMiddleware, createStore} from 'redux'
import thunk from 'redux-thunk'

import adapter from 'redux-localstorage/lib/adapters/localStorage'
import {filter} from 'redux-localstorage/lib/enhancers'
import persistState from 'redux-localstorage'

import * as reducers from './reducers'

const storage = compose(
  filter('jobs.favoriteIds'),
  adapter(window.localStorage)
)

const createPersistentStore = compose(
  applyMiddleware(thunk),
  persistState(storage),
  createStore
)

const reducer = combineReducers(reducers)
const store = createPersistentStore(reducer)

@klaasman
Copy link
Author

It's working as expected now! The deepmerge isn't necessary anymore, great job!

@elgerlambert
Copy link
Owner

Thanks klaasman, glad to hear everything is working now!

@klaasman
Copy link
Author

I yelled too early I'm afraid, the deep merge still is necessary:

const initialState = {
  something: {
    key: 123
  },

  somethingElse: {
    key1: [],
    key2: {},
    key3: false,
    // ...
  },

}

const persistedState = {
  somethingElse: {
    key1: [1, 2, 3]
  }
}

const mergedResult = {...initialState, ...persistedState}

/*
mergedResult equals:

{
  something: {
    key: 123
  },

  somethingElse: {
    key1: [1, 2, 3]
    // key2 and key3 are missng due to the not so deep merge
  }
}
*

Perhaps a deep merge could be made the standard merge strategy?

@elgerlambert
Copy link
Owner

Third time's a charm right?

Grab beta3 from npm; your state should be merged correctly now!

@klaasman
Copy link
Author

Exactly, thanks again ;-) facing no problems with beta3.

@elgerlambert
Copy link
Owner

You're welcome again =)
Apologies for closing the issue pre-maturely btw and thanks for sharing your issues!
Good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants