Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

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

Closed
Ethorsen opened this issue Jun 18, 2018 · 18 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@Ethorsen
Copy link
Contributor

Ethorsen commented Jun 18, 2018

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 mpeyper added the question Further information is requested label Jun 19, 2018
@mpeyper
Copy link
Contributor

mpeyper commented Jun 19, 2018

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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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
Copy link
Contributor

mpeyper commented Jun 19, 2018

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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

So what is the OP trying to do with

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

?

@jpeyper
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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
Copy link
Contributor Author

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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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... ioof-holdings/redux-subspace#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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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
Copy link
Contributor Author

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]}));
  1. 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
Copy link
Contributor Author

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
Copy link
Collaborator

jpeyper commented Jun 19, 2018

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
Copy link
Contributor Author

Nice, I'll explore those avenues.

@mpeyper
Copy link
Contributor

mpeyper commented Jun 19, 2018

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
Copy link
Contributor

mpeyper commented Jun 19, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Ethorsen pushed a commit to Ethorsen/redux-dynostore that referenced this issue Jun 26, 2018
- Add a mapExtraState option to subspaced enhancer.
- Allow instance enhancers
@Ethorsen Ethorsen mentioned this issue Jun 26, 2018
@Ethorsen
Copy link
Contributor Author

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

mpeyper added a commit that referenced this issue Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants