ioof-holdings / redux-dynostore

These libraries provide tools for building dynamic Redux stores.
BSD 3-Clause "New" or "Revised" License
122 stars 15 forks source link

Simple question: What is dynostore equivalent to mapExtraState from redux-dynamic-reducer #9

Closed Ethorsen closed 6 years ago

Ethorsen commented 6 years ago

Simple question: What is dynostore equivalent to mapExtraState from redux-dynamic-reducer

I'd like to move right away from redux-dynamic-reducer to dynostore, but can't figure out how to use enhancers to map extra state to my subspace

Used to do something basic like this to just forward all app state to my module (just as a test)

withReducer (reducer, 'dashboard', {
  mapExtraState: (state, rootState) => ({
    ...state
  })
}) (Component)

My guess is that I'll have to use custom enhancers from now on, but from the current documentation in place I just can't figure out how it works. Could someone provide further explanation and ideally an example on how to define and use enhancers.

Thx

mpeyper commented 6 years ago

Hi @Ethorsen,

That bit of functionality was not brought across to redux-dynostore as we didn't use it. We found the redux-subspace wormhole functionality covered more of our use cases with less effort.

import { createStore } from 'redux'
import dynostore, { dynamicReducers }  from '@redux-dynostore/core'
import { applyMiddleware } from 'redux-subspace'
import wormhole from 'redux-subspace-wormhole'

const store = createStore(reducer, compose(
  applyMiddleware(
    wormhole((state) => state.passOnToEverthing, 'passOnToEverthing'),
    wormhole((state) => state.alsoPassOnToEverything, 'alsoPassOnToEverything')
  ),
  dynostore(dynamicReducers())
))

That's not to say it couldn't be implemented here as well though, if there was a strong enough desire for it.

I can see 2 reasonable options for this:

  1. part of the subspaced enhancer
import dynamic from '@redux-dynostore/react-redux'
import { attachReducer } from '@redux-dynostore/redux-subspace'
import subspaced from '@redux-dynostore/react-redux-subspace'

dynamic('dashboard', attachReducer(reducer), subspaced({
  mapExtraState: (state, rootState) -> ({...state})
})(Component)
  1. as a seperate enhancer
import dynamic from '@redux-dynostore/react-redux'
import { attachReducer } from '@redux-dynostore/redux-subspace'
import subspaced, { mapExtraState } from '@redux-dynostore/react-redux-subspace'

dynamic(
  'dashboard',
  attachReducer(reducer),
  mapExtraState((state, rootState) -> ({...state})),
  subspaced()
)(Component)

1 would be easier to implement (I'd just copy most of it from redux-dynamic-reducer), but I think 2 fits with the style of the library more (care would need to be taken about the order of the enhancers).

jpeyper commented 6 years ago

Is option 2 even doable without subspace?

What's the original goal here? I don't even remember the original functionality!

Copying the state with mapExtraState so cannot be modified by accident?

mpeyper commented 6 years ago

It would merge the resulting object with the state for the provided key so you could add extra values from the parent or root state to the child.

Option 2 without subspace would let you create derived state, copy a key to different name, or add constant state to the current level of nesting (I image it is technically a subspace the maps to the current state).

jpeyper commented 6 years ago

So what is the OP trying to do with

mapExtraState: (state, rootState) => ({...state})

?

jpeyper commented 6 years ago

Oh sorry, I just reread his message

Used to do something basic like this to just forward all app state to my module (just as a test)

jpeyper commented 6 years ago

I feel like if you need to forward all app state, the just don't subspace it? Dynamic reducer used to force subspaces, but dynostore doesn't.

And if you need specific values use wormholes.

Ethorsen commented 6 years ago

Thx for your feedback guys.

Actually, my app will allow a user to load one or multiple instances of the same module that should live side-by-side without any conflict. Let's say the app is a 'Launcher'. In this case I think subspacing them will be the only way to go. A bunch of state handling will be done at launcher level, managing for example login logic, language selection, high level configs, etc. So I want to expose to each module a small set of these state.

I will explore what wormholes has to offer. But from what I'm seeing, it seems that you can't control what each module will be receiving as extra state individually.

Solution 2 from @mpeyper would be ideal, as each module developer will be able to import extra state that he needs from the launcher, without modifying the store middlewares.

jpeyper commented 6 years ago

Yes that is an issue with wormholes, every child subspace gets them all. We do have a few ideas to fix/change them to be more selective (its on my todo list, it just hasn't been a priority... https://github.com/ioof-holdings/redux-subspace/issues/57)

Having said that, I'm still not sure I understand what kind of state the launcher is providing that couldn't be universally shared by all modules (they don't have to use everything). Normal use cases for wormholes are things like "configuration" or "user information".

Anything that is private for each of your modules would just be in that modules reducer and not be provided by the parent's state.

Both options 1 and 2 from @mpeyper are both functionally the same, and defined by within the child component (module).

Just some food for thought for yourself and @mpeyper:

  1. mapExtraState is a subspace feature (sort of, subspace makes it possible through the mapState argument), so to me it makes no sense to define it in a seperate enhancer

  2. mapExtraState creates a coupling between the direct parent and child and increases the complexity of supporting grandchild components. By this I mean if you have dynamic components within dynamic component, you will need to drill the state through all the levels, even if the intermediate levels do not actually require it.

jpeyper commented 6 years ago

Also, we want to make transitioning from redux-dynamic-reducer, so we definitely want to implement either option 1 or 2 (or similar), just to smooth the transition, even if we deprecate the feature later in favour of a better solution.

Ethorsen commented 6 years ago

I've been able to fix most of it with wormholes. You are right, almost all states from launcher is pretty much generic.

I'm still facing an issue. When the launcher is creating a new module, it needs to push new session information in its internal reducer.

Let's say I have a module like this

export default dynamic('dashboard', subspaced(), attachReducer(reducer) )(
  withRouter(
    connect(
      mapStateToProps,
      mapDispatchToProps
    )(DashBoardContainer)
  )
);

When launching the dashboard, I'll instantiate and inject the component in a new tab with

DashBoard.createInstance('dashboard'+ tabIndex);

The new instance needs to be aware of some information. Like tabIndex (for routing) and a new session fetched from the API (each instance will have its own session).

At this point, I would need to either

  1. map this specific part of the launcher state to the new instance. (all tab infos are being kept in a launcher state).
    Something like

    DashBoard.createInstance('dashboard'+ tabIndex).mapExtraState((state, rootState) -> ({ tabInfo: state.tabing.tabs[tabIndex]}));
  2. dispatch some more info to the state of the instantiated dashboard reducer. Is is possible to dispatch an action to the new instantiated reducer right after createInstance? Or at the same time maybe.

Thx a bunch for your help

Ethorsen commented 6 years ago

I believe option 2 is better as I agree with what you said:

Anything that is private for each of your modules would just be in that modules reducer and not be provided by the parent's state.

jpeyper commented 6 years ago

I'm starting to get a clearer picture of your problem now. Its not a perfect solution but...

An undocument feature of dynostore is that is passes the identifier to the dynamic component.

So, if you changed your tabs to be map rather than an array that took the entire identifier name, your wormhole can just be

wormhole((state) => state.tabing.tabs, 'tabs')

And inside DashBoardContainer's mapStateToProps you can use ownProps.identifier to get the specific tab information you want.

const mapStateToProps = (state, ownProps) => {
  return {
    tabInfo: state.tabs[ownProps.identifier]
  }
}

And another option available to you is just regular ol' props, dynostore will pass through props that are provided to the dynamic hoc.

<Dashboard tabInfo={state.tabing.tabs[tabIndex]} />

Dashboard can use that prop however it likes (dispatch it into its own state, put it on context, or just pass it around in props internally).

Ethorsen commented 6 years ago

Nice, I'll explore those avenues.

mpeyper commented 6 years ago

Thinking about it a bit more, I don't think option 2 is viable without reimplementing a lot of what subspace does.

I was thinking the enhancer would look something like:

import isPlainObject from 'lodash.isplainobject'
import { subspaced } from 'react-redux-subspace'

const mapExtraStateEnhancer = mapExtraState => {
  const mapState = (state, rootState) => {
    if (!isPlainObject(state)) {
      return state
    }
    const extraState = mapExtraState(state, rootState)
    return isPlainObject(extraState) ? { ...extraState, ...state } : state
  }
  const subspaceEnhancer = subspaced(mapState)
  return () => () => Component => subspaceEnhancer(Component)
}

export default mapExtraStateEnhancer

This would allow the additional state to be merged into the current state (e.g. you wanted to grab something from the rootState).

The issue is that if this enhancer is used before the subspaced enhancer, the subspace enhancer would throw it away anyway as it scoped the state to the identifier's slice. However, using it after the subspaced enhancer means that that parent's state is already lost so you can't include it in the child's state.

If we want to implement this I think it has to be as an option to the subspaced enhancer (option 1).

mpeyper commented 6 years ago

I think option 1 would look something like:

import isPlainObject from 'lodash.isplainobject'
import { subspaced } from 'react-redux-subspace'

const subspacedEnhancer = ({ mapExtraState = () => null } = {}) => identifier => {
  const mapState = (state, rootState) => {
    const componentState = state[identifier]
    if (!isPlainObject(componentState)) {
      // perhaps a warning/error
      return componentState
    }

    const extraState = mapExtraState(state, rootState)
    if (!isPlainObject(extraState)) {
      // perhaps a warning/error if not null
      return componentState
    }

    return { ...extraState, ...componentState }
  }
  const subspaceEnhancer = subspaced(mapState, identifier)
  return () => Component => subspaceEnhancer(Component)
}

export default subspacedEnhancer

But I don't have time to test or document it. It also doesn't provide the withExtraState fluent function that redux-dynamic-reducer had. Not sure how we could achieve that without a major rewrite of the internals.

Happy to review PRs for this. The file to change is here.

Ethorsen commented 6 years ago

Unfortunately that piece of code wont help solve my current issue. (I currently fixed using jpeyper suggestion, allowing my modules all tabs information and providing index in its props). I'm not saying this functionality should not be included in the project, just that its not for the current issue.

Each of my dynamic module can be instantiated one or multiple times. Each instance will receive its own tab information. So I can't specify a part of the store in the dynamic declaration, as that part of the store will only exists when the tab is being created.

The extra state should then be provided when running createInstance. Something like I already posted.

DashBoard.createInstance('dashboard'+ tabIndex).mapExtraState((state, rootState) -> ({ tabInfo: state.tabing.tabs[tabIndex]}));

I've been looking at your code and it's above my current level of react skills. Sry I can't help implement this new feature.

Ethorsen commented 6 years ago

Nevermind, I'll give it a try and might post a PR soon..

Ethorsen commented 6 years ago

This does seem to do the trick. Works nicely on my end.

I'll still be using wormholes for states that are shared across all dynamic modules