inbound and outbound wrong way round

[saberking]

I spent an hour debugging my code before I realised this was the issue.

[marvinhagemeister]

Thanks a lot for your PR! Are you sure if this is not an error in your client code? The documentation is consistent with our code:

function createTransform (inbound, outbound, config = {}) {
  let whitelist = config.whitelist || null
  let blacklist = config.blacklist || null

  function whitelistBlacklistCheck (key) {
    if (whitelist && whitelist.indexOf(key) === -1) return true
    if (blacklist && blacklist.indexOf(key) !== -1) return true
    return false
  }

  return {
    in: (state, key) => !whitelistBlacklistCheck(key) && inbound ? inbound(state, key) : state,
    out: (state, key) => !whitelistBlacklistCheck(key) && outbound ? outbound(state, key) : state
  }
}

export default createTransform

taken from https://github.com/rt2zz/redux-persist/blob/master/src/createTransform.js#L11

[saberking]

I just checked the code:

function rehydrate (key, serialized) {
  let state = null

  try {
    let data = deserialize(serialized)
    state = transforms.reduceRight((subState, transformer) => {
      return transformer.out(subState, key)
    }, data)
  } catch (err) {
    if (process.env.NODE_ENV !== 'production') console.warn('redux-persist/getStoredState: Error restoring data for key:', key, err)
  }

  return state
}

Should this be transformer.in? It seems to make sense to me that rehydrating the store is 'in' and persisting it is 'out'. I guess this is just my misunderstanding the sense of inbound and outbound, where you have used 'in' to mean into persistent storage as opposed to into the redux store. Maybe this could be renamed then, such as 'toReduxStore' and 'toPersistentStorage', in and out are a bit vague and I assumed the opposite of what they are actually being used to mean

[marvinhagemeister]

@saberking Thanks for your explanation. I see where the confusion comes from. I think just flipping the names would not suffice, because there are likely others who'd expected it the way it is. Perhaps we can find a more meaningful name that signals where the data is flowing to.

I like both toReduxStore or just toStore and toPersistentStorage/ toStorage or something similar. What are your thoughts about this @rt2zz ?

[rt2zz]

I agree the naming is ambiguous. From redux-persist's perspective I think of it as owning the state in storage, whereas redux owns the application state, hence the in/out mapping you currently see.

I agree with @marvinhagemeister rather than flipping it and making it confusing for the other half of users, lets make the terms explicit: outboundStateFromStorage / inboundStateFromRedux ?

[marvinhagemeister]

After thinking about it the only thing that I'm afraid of is the resulting API breakage. We just passed 1000 stars here on github and there is probably a lot if code out there which relies on the current behaviour. We could leave the current methods intact and print a deprecation warning whenever they are called.

[saberking]

Another option would be simply to make it explicit in the readme

[rt2zz]

If I am not mistaken this PR is just to update readme.

Side note. 1000 stars! thanks @marvinhagemeister, we made it ;)

[saberking]

Should I close this? From our discussion the change to the readme should certainly not be the one I have proposed in this PR

[rt2zz]

Just updated readme with explanatory comments 👍