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

RFC: New wormhole API #57

Closed
mpeyper opened this issue Dec 29, 2017 · 9 comments
Closed

RFC: New wormhole API #57

mpeyper opened this issue Dec 29, 2017 · 9 comments

Comments

@mpeyper
Copy link
Contributor

mpeyper commented Dec 29, 2017

What happened?

Recently I lost quite a few hours debugging an issue in a thunk that looked more or less like:

const fetchData = (id) => (dispatch, getState, api) => {
  if (Object.keys(getState()).length === 0) {
    api.get(id).then((data) => dispatch({ type: 'FETCHED_DATA', data })
  }
}

Essentially, the idea here was to only fetch the data only if the state didn't already have any keys.

The reducer looked something like:

const reducer = (state = {}, action) => {
  if (action.type === 'FETCHED_DATA') {
    return { ...state, ...action.data }
  }

  return state
}

This worked fine when developing and testing the component, but would never attempt to fetch the data once it was integrated into a parent component.

Eventually I tracked it back to the use of wormholes in the root application. The wormholes added additional keys to the state, so when checking the length of the keys, it was never empty.

What should have happened?

In the above scenario, I would have preferred if the state was unchanged, particularly as this component did not care about any of the wormholed values. That way, the component would not need to be modified to work within the root app.

Any Ideas?

The issue I see here is the way wormholes get globally applied to all subspaces. This can lead to difficult to find bugs in child components that had no control over what the wormholes, if any, are being applied to their state. The scenario above is an example of this.

I propose changing the wormhole API to be an opt in addition to the components state. In the new API, components would nominate which wormholes, if any, they want to receive, and if no wormholes are nominated the components state is unchanged.

I'm thinking that this could work similarly to React's Context feature where higher in the tree, a component can add values into context, and descendants of the components can opt into received one or more of those values. There is also potential here to improve the developer experience here by providing dev time warnings if the requested wormhole values are not provided by the parent.

Translating this concept to subspaces, the wormhole middleware would be the point higher in the tree to add the values. Currently, multiple wormholes are added a seperate calls to the wormhole middleware. In this proposal the api will change to an object of key to wormhole selector, i.e.:

const store = createStore(reducer, applyMiddleware(
  wormhole((state) => state.value1, 'key1'),
  wormhole((state) => state.value2, 'key2'),
  wormhole((state) => state.value3, 'key3')
))

would become:

const store = createStore(reducer, applyMiddleware(
  wormholes({
    key1: wormhole((state) => state.value1),
    key2: wormhole((state) => state.value2),
    key3: wormhole((state) => state.value3)
  })
))

This is arguably a nicer API than the existing one if there are lots of wormholes, but it does remove the ability to use the selector shorthand that has been adopted in many of the subspace feature (i.e. wormhole((state) => state.value, 'value') can be shortened to just wormhole('value')). I'd like to investigate ways to maintain backwards compatibility with the existing API on this end, but my feeling is that it won't be possible if we want to provide warnings when requested keys are missing.

On the sub-component side placeholder values would be added to it's state to select the values the component wants to be included from the wormholes.

This could be done as a higher-order reducer:

const reducer = combineReducers({ reducer1, reducer2)
const enhancedReducer = withWormholes('key1', 'key3')(reducer)

or as a reducer to combine with others:

const reducer = combineReducers({
  reducer1,
  reducer2,
  wormholes: wormholesReducer('key1', 'key3')
)

There are advantages and disadvantages to both approaches, so I would be keen to hear how you would prefer to use it.

When it comes to using the subspaces, the wormhole middleware would inspect the state of the subspace and replace any of the placeholder values with the actual values from the wormhole selectors (and warn about any that can't be found). There is also potential here to add shape validation (e.g. prop-types) to the wormholed values to ensure the parent is providing them in a usable format for the child, again improving the developer experience.

Pros

  • Declarative
  • Component state is closer to runtime expectations
  • Better developer experience

Cons

  • Inspecting the state in the dev tools would display the placeholder values in their raw format
  • It is (probably) not backwards compatible with the current implementation

One of the biggest differences I see in philosophy between the current implementation and explicit nature of this proposal is that currently, the components don't have to care where the extra values in their state come from. They can be from wormholes, added when mapping the component state in a SubspaceProvider or any other kind of magic you can pull off the get the values there when getState is called, but with the new approach, they can only come from a wormhole. There is a whole series of questions this philosophical change brings in, but I don't have time to go into those now. Perhaps I'll add a following post to ask those soon.

Anyway, how does all this sound? Let me know what you think, ask any questions you want, suggest changes to the proposal, or propose your own changes.

@mpeyper
Copy link
Contributor Author

mpeyper commented Dec 30, 2017

There is a whole series of questions this philosophical change brings in, but I don't have time to go into those now. Perhaps I'll add a following post to ask those soon.

As promised...

There are currently two main ways to add global state to a child subspace:

  1. Mapping additional state from the rootState parameter when creating the subspace

    <SubspaceProvider mapState={(state, rootState) => ({
      globalValue: rootState.globalValue,
      ...state.childState
    })}>
      <ChildComponent />
    </SubspaceProvider>
  2. Using wormholes

    const store = createStore(reducer, applyMiddleware(
      wormhole((state) => state.globalValue, 'globalValue')
    ))

The main difference between the two is that wormholes are applied to all subspaces, but mapState method only applies to the subspace being created. The other important thing to note is that wormholes were only introduced in version 2, with the mapState method being the only option prior to that.

The biggest issue with the mapState method is that if you have a truely global piece of state, e.g. an auth token, then the mapping should be done at all levels, for every subspace, which can be quite repetitive and easy to forget in intermediate subspaces that don't require the global value themselves.

This whole proposal is about fixing one of the biggest issues with wormholes (forcing the global values values into the component's state whether they want them or not), but there is another as well.

The other issue with wormholes is that currently they can only be created when the store is created in the root application. This means that any wormholes must be known up front and can't change (although there is no penalty for having a wormhole to an undefined value until it is available).

If redux-subspace was being created from scratch today, I do not believe the mapState method would be a thing. By this I mean that the rootState parameter would not be supplied to the mapState function. Wormholes have proved to be a more useful way of injecting global state and it caused one hell of a bug with the redux-devtools-extenstion (which I still don't fully understand). That said, I don't want to throw it away just because it isn't my preference on how to use it.

So here are my questions

  1. How would you all feel about removing the rootState parameter from mapState?
  2. If it wasn't removed, should it also be opt in?
  3. If it is opt in, should it be seperate from the wormhole reducer (withWormholes or wormholesReducer from the previous post) or should they become more generic (e.g. withExternal or externalReducer)

For me, either removing rootState entirely or treating both approaches as opt in would alleviate one of my biggest concerns with this proposal which is that having the wormhole specific placeholders in the component's state feels a bit weird if there are other legitimate ways to inject additional state. I kind of like the idea of treating them generically, so that the one creating the subspace can choose how they want to inject the additional state, rather than the child enforcing one particular way.

The only other bit of functionality I would want before being comfortable in removing rootState is the ability to add additional wormholes at arbitrary points in the hierarchy, where they would only apply to subspaces created from that point and below. I think this is a worthwhile feature whether rootState is removed or not.

The easiest way I can think of to achieve this is to expose an enhancer parameter when creating a subspace to compose with the other enhancers used to create the subspace, then use the existing applySubspaceMiddleware enhancer to add additional wormholes. This new parameter would need to be exposed through all the packages that create subspaces as well.

<SubspaceProvider
  mapState={(state) => state.childState)}
  enhancer={applySubspaceMiddleware(wormhole((state) => state.someValue, 'someValue'))}
>
  <ChildComponent />
</SubspaceProvider>

Some care would need to be taken with the new parameter to ensure it is passed down for subsequent subspaces to use as well, and to ensure it doesn't override anything in the subspaceOptions and accidentally leak back up the tree, but all that is just implementation details.

This approach means the only time new wormholes can be created is when a subspace is created. This may lead to situations where an unnecessary subspace layer is introduced purely to create a wormhole, e.g.

<SubspaceProvider
  mapState={(state) => state) /* note: no state change or namspacing applied */}
  enhancer={applySubspaceMiddleware(wormhole((state) => state.someValue, 'someValue'))}
>
  <div>
    { /* ... */ }
      <SubspaceProvider mapState={(state) => state.childState}>
        <ChildComponent />
      </SubspaceProvider
    { /* ... */ }
  </div>
</SubspaceProvider>

Alternatively, we could provide an new mechanism for expanding the enhancer used when creating subspaces. Really, all we really need to do is change the store that is used to create the subspace which is relatively easy in the React use case, but not so straight forward in other use cases (e.g. sagas).

const newStore = enhanceSubspace(applySubspaceMiddleware(
  wormhole((state) => state.someValue, 'someValue')
))(store)
<SubspaceEnhancer enhancer={applySubspaceMiddleware(wormhole((state) => state.someValue, 'someValue'))}>
  <div>
    { /* ... */ }
      <SubspaceProvider mapState={(state) => state.childState}>
        <ChildComponent />
      </SubspaceProvider
    { /* ... */ }
  </div>
</EnhanceSubspace>

More questions:

  1. Which of these two approaches do you prefer?
  2. Can you think of a better API for adding new wormholes?

This is all I can think of for now, but I'll post more if it comes to mind.

@mpeyper
Copy link
Contributor Author

mpeyper commented Jan 5, 2018

I've been thinking about this more recently and I wanted to get some feedback on an idea I've had for the API when creating the wormhole middleware.

As previously mentioned, the proposal would likely mean changing from multiple calls the the wormhole middleware to a single call to a wormholes middleware that would construct all the wormholes with a single call, e.g.

const store = createStore(reducer, applyMiddleware(
  wormhole((state) => state.value1, 'key1'),
  wormhole((state) => state.value2, 'key2'),
  wormhole((state) => state.value3, 'key3')
))

would become:

const store = createStore(reducer, applyMiddleware(
  wormholes({
    key1: wormhole((state) => state.value1),
    key2: wormhole((state) => state.value2),
    key3: wormhole((state) => state.value3)
  })
))

Recently I've been enjoying using emotion for styling and one of the things their API allows is multiple ways of setting the styles, e.g.

const Container = styled.div(
  {
    margin: 10
  }
)

or

const Container = styled.div(props => ({
    margin: props.margin
  })
)

or

const defaultMargin = css({
  margin: 20
})

const Container = styled.div(defaultMargin)

The interesting part here is that you can provide multiple items of varying style to it and it will merge them together for the final product, e.g.

const defaultMargin = css({
  margin: 20
})

const Container = styled.div(
  defaultMargin,
  {
    margin: 20
  },
  props => ({
    margin: props.margin
  })
)

I'm wondering if enabling a similar functionality for wormholes would be wanted/useful?

The variations I can see are:

  1. a object of key to:
    1. function - a selector for the value to provide as key
    2. string - shorthand for building a selector, e.g. 'value' -> (state) => state['value']
  2. string - shorthand for building a single key variation of the object variation, e.g. 'value' -> { value: (state) => state['value'] }
  3. function - accepts state and returns and object that will be spread into the wormhole values. This could be useful for creating conditional wormholes or for reducing duplication in a selectors.

Putting a very contrived example together, this would allow a (admittedly messy) wormhole configuration like:

wormholes(
  'value1',
  {
    value2: (state) => state.nested.value,
    value3: 'simpleValue'
  },
  (state) => ({
    value4: state.hasThing ? state.thing : state.otherThing
  }),
  (state) => {
    const { complicated: { way: to = { find: 'values', in: 'state' } } } = state
    return {
      value5: to.find,
      value6: to.in
    }
  }
)

where, if the state looked like

{
  value1: 'this',
  nested: {
    value: 'is'
  },
  simpleValue: 'pretty',
  hasThing: true,
  thing: 'something else',
  otherThing: 'cool',
  complicated: {
    way: {
      find: 'right',
      in: '?'
    }
  }
}

the resulting wormholes would be

{
  value1: 'this',
  value2: 'is',
  value3: 'pretty',
  value4: 'cool',
  value5: 'right',
  value6: '?'
}

Obviously the wormholes don't have to be strings, but then I wouldn't have been able to make that happen 😛

Thoughts?

@jpeyper
Copy link
Collaborator

jpeyper commented Jan 5, 2018

(typed out on my phone so forgive any weirdness)

Honestly, I think that it's overkill and confusing. Just because you can do it does not mean you should.

Emotion has and needs to be flexible in the ability to import styles from all over the place in different ways (straight objects, from external props or shared styles) and bring them together

Here we can keep the cognitive overhead down and just have one way to do it and document it appropriately.

Personally I like your original proposal (map of keys to functions). The others methods are just shorthand for the same thing.

@jpeyper
Copy link
Collaborator

jpeyper commented Jan 5, 2018

The only other bit of functionality I would want before being comfortable in removing rootState is the ability to add additional wormholes at arbitrary points in the hierarchy, where they would only apply to subspaces created from that point and below. I think this is a worthwhile feature whether rootState is removed or not.

I think it would be super cool and nice if wormholes added were available not only down the tree, but anywhere, assuming you can mark your opted in wormholes as optional (no guarantee something not up your branch of the tree actually exists).

It would be a really nice way for having isolated components that function in their own right but when both present they augment each other.

For example (perhaps a poor one):

You have a search component that takes you to a route when you select an entity. It provides a wormhole to the selected entity.

You also have a slide out information component that can display information about an entity. It uses the searched entity wormhole. It provides a wormhole that just says it exists.

The search component can use the slide outs wormhole to disable the route changing.

I could see us using this internally at ioof for badging.

@mpeyper
Copy link
Contributor Author

mpeyper commented Jan 5, 2018

Honestly, I think that it's overkill and confusing. Just because you can do it does not mean you should.

Whist I agree that things shouldn't be done "just coz", I do feel as though each of the options, except perhaps, 1.ii (object of key to string), offers something useful.

  • 1.i: the how wormholes are actually defined
  • 2: the shorthand for building selectors that is used in many, subspace feature (including the exisiting wormhole middleware)
  • 3: a thunk like feature that allows computation of wormholes based on state, rather than being needing to be known up front (I had a use for this in our storybook-utils decorator already but got worked around it by writing a custom middleware then duplicated 90% of the wormhole logic).

Right now, you can do 1.i, 1.ii and 2 by having multiple wormhole middleware

const store = createStore(reducer, applyMiddleware(
  wormhole('key1'),
  wormhole('value2', 'key2'),
  wormhole((state) => state.nested.key3, 'key3')
))

The main difference comes from my prediction that the proposal will need the middleware to know about all wormholes, so all the calls will need to be made together somehow, or we start losing features.

Without being able to accept multiple wormhole creation arguments the above example would look like

const store = createStore(reducer, applyMiddleware(
  wormholes({
    key1: (state) => state.key1,
    key2: (state) => state.value2,
    key3: (state) => state.nested.key3
  })
))

I am proposing the original version could be replicated like so

const store = createStore(reducer, applyMiddleware(
  wormholes(
    'key1',
    {
      key2: 'value2',
      key3: (state) => state.nested.key3
    }
  })
))

Then then only new feature is the thunk like variation which would allow it to be written as

const store = createStore(reducer, applyMiddleware(
  wormholes((state => ({
    key1: state.key1,
    key2: state.value2,
    key3: state.nested.key3
  }))
))

This new feature would allow things like:

  1. conditional wormholes

    const store = createStore(reducer, applyMiddleware(
      wormholes({
        key: (state) => state.thing ? state.thing.value : undefined
      }
    ))

    vs.

    const store = createStore(reducer, applyMiddleware(
      wormholes((state) => {
        if (state.thing) {
          return { key: state.thing.value }
        }
        return {}
      })
    ))

    This simple example might actually be better in the basic way, but more complicated examples might be nicer in the "thunk"

  2. reduced duplication of complex wormhole setup

    wormholes({
      key1: (state) => {
        const { complicated: { way: to = { find: 'values' } } } = state
        return to.find
      },
      key2: (state) => {
        const { complicated: { way: to = { in: 'state' } } } = state
        return to.in
      }
    }

    vs.

    wormholes((state) => {
      const { complicated: { way: to = { find: 'values', in: 'state' } } } = state
      return {
        key1: to.find,
        key2: to.in
      }
    }
  3. creating wormholes for unknown keys when creating the store (e.g. wormholes for all the keys of an object). This can't be done without this feature.

    const store = createStore(reducer, applyMiddleware(
      wormholes((state) => ({ ...state.objectWithChangingKeys }))
    ))

@mpeyper
Copy link
Contributor Author

mpeyper commented Jan 5, 2018

I think it would be super cool and nice if wormholes added were available not only down the tree, but anywhere, assuming you can mark your opted in wormholes as optional (no guarantee something not up your branch of the tree actually exists).

This fits well with the "wormhole" analogy where a wormhole is just a tunnel between to points in space (state). I'm not sure how possible it is, and how we would avoid key conflicts, but I'm happy to review any propositions in this direction.

@mpeyper
Copy link
Contributor Author

mpeyper commented Jan 6, 2018

Another question on the proposal. would the wormhole middleware need to traverse the substate for wormhole placeholders, or just the immediate keys of the mapped state?

For example if the reducer for a subspace was defined as:

const reducer = combineReducers({ reducer1, reducer2 })
const enhancedReducer = withWormholes('key1', 'key3')(reducer)

the state might look something like

{
  key1: { _fromWormhole: true },
  key3: { _fromWormhole: true },
  reducer1: { ... },
  reducer2: { ... }
}

and the middleware would look for any _fromWormhole values and replace them. What if the reducer was defined as

const reducer = combineReducers({ reducer1, reducer2: withWormholes('key3')(reducer2) })
const enhancedReducer = withWormholes('key1')(reducer)

Then state might look something like

{
  key1: { _fromWormhole: true },
  reducer1: { ... },
  reducer2: { 
    key3: { _fromWormhole: true },
    ...
  }
}

Would you expect key3 to be replaced for this subspace?

On the surface, my gut say yes, but if you take nested subspaces into account, it's going to be impossible to determine which _fromWormhole values we defined in the subspaces reducer, and which were for a reducer combined into it's state for a nested subspace. Also, recursively replacing the placeholders on a large state tree might become an expensive exercise to do every time getState is called.

The current wormhole functionality will add the keys to the root of the subspace state, so not doing it recursively is consistent, but now that they subspace is responsible for defining where the wormhole reducer exists, this in it's state tree, it may be a bit confusing.

So would it be safe to assume any wormhole values are only expected at the root of a subspace?

@jpeyper
Copy link
Collaborator

jpeyper commented Jan 18, 2018

So @mpeyper and I have been chatting about this outside of github.

We like:

  • Wormholes being opt-in only
  • Wormholes resolving anywhere in the state tree, not just at subspace root.
  • Subspaces being able to provide new wormholes to other subspaces (with a defined strategy for handling name conflicts)
  • Wormholes being visible and resolved in the state tree (useful for debugging with redux dev tools)

We have a few ideas on how to achieve this, and I hope to get some time to work on this soon (although don't let that stop you form giving it a crack yourself).

@mpeyper
Copy link
Contributor Author

mpeyper commented Sep 8, 2019

Closing due to inactivity. Happy to repoen if there is more discussion to be had.

@mpeyper mpeyper closed this as completed Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants