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

Discussion: React-Redux and React async rendering #890

Closed
markerikson opened this Issue Mar 5, 2018 · 88 comments

Comments

@markerikson
Contributor

markerikson commented Mar 5, 2018

I'll open this up as a general forum for discussing anything related to how React-Redux is going to interact with React's new "Suspense" async rendering capability and time-slicing behavior.

At the moment, we don't really have a good idea of what that means for React-Redux. We're going to need a better understanding of how React's async rendering behavior works, how this suggested "caching" concept fits into things, and what the final React APIs are going to look like.

@lmatteis

This comment has been minimized.

lmatteis commented Mar 5, 2018

The general React API will change for suspense, but the main concept I believe is that this.deferSetState({ foo: 'bar' }) will try to render as much as the tree as possible, but only flush the actual changes when all the thrown promises in the tree have completed (movieFetcher.read() throws a promise). The actual update could occur after several seconds if the promises are HTTP requests.

If you call deferSetState again, the old tree is discarded to avoid race-conditions.

There are other bells and whistles, like Placeholder and Loading which allow to have finer-grained access to this process, but this is the overall idea I believe.

In the demo the fetchers seem to use a cache. Throwing an HTTP promise in the render is akin to "start this HTTP operation and re-render me when there's a response", so the cache seems to exist for the render to understand when the response arrived -- when to throw and when to render.

It's unclear how this can be used with Redux, and we should probably keep an eye on this PR #856

From a high-level, the cache that render's use to understand whether they need to start an async operation could very well be a redux store.

I imagine redux should be notified about when such async operations occur:

function MovieDetailPage({ movieId }) {
  const movie = movieDetailFetcher.read(movieId) // throws a promise
  ...

Instead, the connect HOC could pass to the component the actual fetcher so it knows about the promise:

function MovieDetailPage({ movieId, movieDetailFetcher }) {
  const movie = movieDetailFetcher.read(movieId) // throws a promise, and let's redux know about it
  ...

Just my two cents.

@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

You can throw a Promise both from render and from getDerivedStateFromProps. The latter looks like it has potential. But it would require us handling side effects explicitly. That's normally done in the middleware, so it's just a matter of this library providing its own "middleware" of sorts and expecting a particular return from middleware (this would be new).

Here's what I'm thinking: We would wrap the store.dispatch in a function that checks for a Promise return. If it gets one, we store it, fire a setState to trigger an update, and during the render we would throw that promise to React. When it resolves, we would get a re-render from React and should pass through with the resolved state.

The fun part is the specific timing of things with the React render cycle and the store subscription cycle. I think we just need to try it and see what happens.

It would also require middleware spit out that Promise, so we'd need ecosystem coordination. But I think that's do-able and we can provide some helpers for the common cases (wrap function returns from thunks, some sort of saga coordinator, etc).

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

One potential issue there is that I know a lot of people are depending on being able to do this.props.someThunkReturningAPromise().then( () => {} ) inside a connected component.

Another big aspect that's not clear yet is this whole "cache" thing. How are components expecting to access a cache? What does the caching API look like? How could this potentially map to data in a Redux store?

@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

That would pass through the dispatch wrapper. And in async mode, they'll be able to throw that promise too:

render() {
  if (!this.props.users) {
    throw this.props.loadUsers().then(this.handleLoadedUsers)
  }

  return this.renderUsers(this.props.users)
}

Of course, what are you seeing people do within that .then? If it's a loading state, that's handled entirely different by Suspense. So, there's some refactoring needed on the user side too.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

Don't have specific examples to point to atm, but it's usually triggering more fetches, dispatching actions, or setting component state.

@aweary

This comment has been minimized.

aweary commented Mar 5, 2018

You can throw a Promise both from render and from getDerivedStateFromProps

You can also throw a Promise inside state updater functions!

@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

You can also throw a Promise inside state updater functions!

Oh, that's a good one too!

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

I want to warn against coming up with something complicated. Our goal is to make React Redux work roughly the way it was conceived three years ago. Not put another super complicated system like we have now on top.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

Dan, I know that's what you and Andrew have said you want. I'm not sure that's what Tim, Jim, and I want. We don't yet have a full enough understanding of the situation to say that's the right approach.

One point of concern: React-Redux is currently usable with React alternatives like preact-compat and inferno-compat. What happens if we rewrite all of React-Redux's internals in terms of React 16.x-only behavior?

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

My tentative understanding is we want to move state handling in React and use setState updater form + context for it. Some follow up questions that I don’t know answers to:

Who Owns the State?

If React owns the state (which is what we need for time slicing and suspense) then createStore doesn’t make sense. Which might be fine. I imagine top-level Provider could give you an imperative store-like object with a compatible API (just like a ref).

For example, instead of this:

let store = createStore(reducer);
ReactDOM.render(
  <Provider store={store}>
    <App />
  </Provider>,
  node
);

store.dispatch(stuff());

you might have:

let storeRef = React.createRef();
ReactDOM.render(
  <Provider storeRef={storeRef}>
    <App />
  </Provider>,
  node,
  () => {
    let store = storeRef.value;
    store.dispatch(stuff());
  }
);

From user’s point of view it doesn’t matter where store is coming from since it has the same API.

How Are Priorities Handled?

Redux store has getState() (and middleware calls it). It’s not clear to me how this should work with deferred updates that haven’t finished yet. I haven’t really thought about this yet.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

One point of concern: React-Redux is currently usable with React alternatives like preact-compat and inferno-compat. What happens if we rewrite all of React-Redux's internals in terms of React 16.x-only behavior?

I understand if that’s important to you, but supporting this was never the goal of these bindings when we were creating them. I don’t think it makes a lot of sense to support the common denominator forever because:

  • These packages have already chosen to fork the ecosystem (inferno, preact)
  • This doesn’t let React move forward.

React Redux has a peer dependency on React, not Preact or Inferno. That expresses its contract. Of course we should avoid breaking people who rely on this in minors, but I don’t see supporting them forever as a viable strategy for this package.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

One problem with "the store lives purely inside of a <Provider>" is that people are doing lots of stuff with the store outside of the React component tree. Store setup, passing the store to persistence utilities, running tests, etc.

This doesn’t let React move forward.

Yes, but as maintainers of a package that is used by more than just React users, we've got to take the ecosystem as a whole into consideration.

It may be that the final approach is to maintain 5.x as the designated "backwards compatible" line, and 6.x onwards is the "React future-only" line. That's something we can discuss. But, my goal would be to find a way to minimize total API churn and compatibility issues for all of our users, even if that means that we have to do some complex bookkeeping internal to the library.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

One problem with "the store lives purely inside of a " is that people are doing lots of stuff with the store outside of the React component tree. Store setup, passing the store to persistence utilities, running tests, etc.

I agree, and we need to find a solution for these use cases. I’m sure one exists. For cases where you only need the store after rendering, my ref API above should work just fine. For other cases, we can come up with something similar.

I think the mistake here would be to spend a lot of energy on not addressing this, and instead jumping to our regular “let’s completely work around React” way of thinking.

@lmatteis

This comment has been minimized.

lmatteis commented Mar 5, 2018

My concern is also: can we get the same predictability with suspense? Will dispatching N actions always bring you to the same state?

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

React updater functions are deterministic, yes.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

I wouldn't phrase it as "let's work around React". Right now, I want to understand exactly what the constraints are that we need to properly work with React, so that we can figure out the possible solutions based on those constraints.

Can we come up with a specific list of known differences in behavior regarding async rendering, caching, tearing, etc, that we need to make sure are addressed by the final solution?

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

The main constraint, as I understand it, is that React should own applying the state updates. Which, luckily, very easily fits with the reducer paradigm. The main issue here is that createStore doesn’t quite work this way. So that’s what I think we should be looking at solving (instead of intercepting Promises, rethrowing them, etc).

@acdlite

This comment has been minimized.

Contributor

acdlite commented Mar 5, 2018

I like the storeRef idea, for apps that use both React and some non-React Redux consumer.

To expand a bit on Dan's "lowest common denominator" metaphor, the point here is that in the current architecture, Redux is in charge of scheduling:

store change event -> React render cycle

That means that React's scheduling can only ever be as sophisticated as the store. Which is: not sophisticated at all. Redux stores are dumb, synchronous event emitters. They do a bit of batching for nested dispatches, but that's it. Effectively, this means all rendering that occurs as a result of a Redux dispatch must be synchronous to avoid inconsistencies.

That worked fine in a pre-async world, but if we limit ourselves to only having scheduling as smart as what an event emitter can do, we're missing out on all of the amazing stuff Dan showed off in his demos.

By letting React control the scheduling, we get prioritization, rebasing, and suspense for free.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

They do a bit batching for nested dispatches, but that's it.

Note that even today (in sync world) they're already very inefficient for network requests. Because if you have many setStates as a result of network response, Redux store won't batch React rendering. Unless you use an explicit enhancer that is aware of unstable_batchedUpdates.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

I like the storeRef idea, for apps that use both React and some non-React Redux consumer.

One interesting question that comes out of this: what if you have multiple React roots that currently use the same store? How would that work?

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

Yes, I get the desired advantages of that approach. What I don't feel I have yet is a full list (or even a partial list) of the constraints and use cases that we need to actually solve in order to properly work with React.

I know the React team has spent a ton of time focusing on the React side of things, and you're naturally focused on doing everything "the React way". I'm trying to figure out all the other Redux-side use cases that would potentially conflict with that.

One interesting question that comes out of this: what if you have multiple React roots that currently use the same store? How would that work?

Yeah. For example, right now I've got an app that's 85% Backbone and 15% React, and we're progressively rewriting / adding new features with React. We've got many small ReactDOM.render() component trees spread throughout the UI, and they're currently sharing the same Redux store instance.

Other Redux-related constraints to consider:

  • How do middleware fit into this? What about store enhancers?
  • What about calling store.getState() outside of React? What gets returned? How does that relate to what React is trying to show on the screen right now?
@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

I don't see how that storeRef can work. How does one apply middleware or store enhancers?

@acdlite

This comment has been minimized.

Contributor

acdlite commented Mar 5, 2018

what if you have multiple React roots that currently use the same store? How would that work?

Not sure. The most straightforward way to do this is that a dispatch calls setState on every root. Each React root has their own scheduling (they are committed separately), so in the current model, tearing is inevitable. But I think that's inherent to having multiple roots. If you want coordinated scheduling, you should use portals instead.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 5, 2018

How does one apply middleware or store enhancers?

Props to Provider. Again, not saying it’s the final API I propose, just want to get something started. :-)

@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

Can we put together a small example that illustrates the problem with code? All this theory talk without a practical example has me almost completely lost.

@acdlite

This comment has been minimized.

Contributor

acdlite commented Mar 5, 2018

That's why I published those canaries :D Try building Dan's movie demo using Redux and you'll see the problem immediately.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

My gut still says that some kind of a React-Redux-specific store enhancer would be a potential approach here. Override dispatch() and subscribe(), and have that hook into React's state management and lifecycles somehow. (Maybe some kind of private/internal API function that Provider could pass as the setState() completion callback, and until that's executed, the store still returns the "old" state.)

@timdorr

This comment has been minimized.

Member

timdorr commented Mar 5, 2018

Is Dan's demo published somewhere? More than happy to take the lead on that. For once, I want to build a broken application 😄

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 5, 2018

So here's the next specific concern I have:

Dispatching an action is intended to be 100% synchronous unless altered by a middleware, and calling store.getState() immediately after dispatching returns the updated state. This pattern is frequently used to do additional logic with the updated state, such as:

// An example of checking state after a dispatch
function checkStateAfterDispatch() {
    return (dispatch, getState) => {
        const firstState = getState();
        dispatch({type : "FIRST_ACTION"});
        
        const secondState = getState();
        
        if(secondState.someField != firstState.someField) {
            dispatch({type : "SECOND_ACTION"});
        }    
    }
}
  • How does multiple-dispatching tie into React's reconciliation process?
  • If React were to "own the state", how would the thunk logic access the updated state synchronously right after dispatching the first action?

(This also applies for middleware as well.)

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 9, 2018

I should have some free time tomorrow. I really should be trying to do workshop prep, but I think I'm going to spend the day playing around with a proof-of-concept spike for connect.

@gaearon , @acdlite , @bvaughn : what's the latest and bleeding-edge-est React copy that I can play around with? Clone the main repo, merge in Andrew's branch, and build that? Or is there a good alpha floating around I can grab? I guess I don't care about the Suspense stuff atm, I just want new context + any async / time-slicing flags I can turn on.

@bvaughn

This comment has been minimized.

bvaughn commented Mar 9, 2018

$ npm info react | grep 16.4
     canary: '16.4.0-alpha.0911da3' },
     '16.4.0-alpha.3174632',
     '16.4.0-alpha.7926752',
     '16.4.0-alpha.0911da3' ],
     '15.3.0-rc.3': '2016-07-21T22:59:16.416Z',
     '16.4.0-alpha.7926752': '2018-02-13T00:52:25.835Z',
     '16.4.0-alpha.3174632': '2018-02-24T05:07:52.478Z',
     '16.4.0-alpha.0911da3': '2018-02-27T02:14:07.333Z' },

Looks like 16.4.0-alpha.0911da3 was the latest release, for Dan's demo. I don't know if there are caveats or gotchas.

If you want to build the latest from master:

git clone git@github.com:facebook/react.git
cd react
yarn install

Maybe toggle some feature flags

yarn build
@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 9, 2018

I've just created a PR with a seemingly-working first cut at a rewrite of React-Redux v5 to use the new React.createContext() API. Please see #898 for details.

@reduxjs reduxjs deleted a comment from apapacy Mar 12, 2018

@faceyspacey

This comment has been minimized.

faceyspacey commented Mar 12, 2018

@markerikson regarding the multiple stores + multiple contexts issue, I see 3 options:

  1. provide your own Context to <Provider /> and all your calls to connect:
const context = React.createContext(null)
<Provider context={context} store={store} />
connect(mapState, etc, { context })

not ideal

  1. offload all the context providing/consumption work to us and allow users to provide a map of stores to our <Provider /> component, and we'll use a single Context.Provider to broadcast to all connected Consumers. Here's the usage:
<Provider stores={{ store1, store2 }}>
      <App />
</Provider>

And here's a quick implementation:

const Context = React.createContext(null)

export default class ReactReduxProvider extends Component {
  constructor(props) {
    super(props)

    let { stores, store } = props
  
    if (Array.isArray(props.stores)) {
      this.storeNames = Object.keys(stores)
    }
    else {
      this.storeNames = ['main']
      stores = { main: store }
    }

    this.state = this.storeNames.reduce((initialState, name) => {
      const { dispatch, subscribe, getState } = stores[name]
      initialState[name] = { dispatch, subscribe, getState, state: getState() }
      return initialState
    }, {})
  }

  componentDidMount() {
    this.storeNames.forEach(name => {
      const store = this.state[name]

      store.subscribe(() => {
        const state = store.getState()

        if (state !== store.state) {
          this.setState({ [name]: { ...store, state } })
        }
      })
    })
  }

  render() {
    return (
      <Context.Provider value={this.state}>
        {Children.only(this.props.children)}
      </Context.Provider>
    )
  }
}

connect(mapState, { store: 'store1' })
connect(mapState, { store: 'store2' })
connect(mapState) // useses 'main' store from `props.store`

function connect(mapState, etc, options = { store: 'main' }) {...}

This has a performance problem though. All the connected hocs will try to update when any of the store update. Each of their shouldComponentUpdate methods could return false in this case, but it still doesn't feel good enough.

  1. The most performant option (least amount of components updating): we could have 2 types of context providers:
  • 1 that provides a map of all available contexts, and it's "statically" available for connect HoCs to access. It's where they will dynamically learn of all available stores/contexts.
  • And then the context providers that actually provide the state for a given store.

The connect api would be the same as in #2, and an implementation would look something like this:

export const ReactReduxRegistryContext = React.createContext(null) // statically importable by `connect.js`

export default class ReactReduxProvider extends Component {
  constructor(props) {
    super(props)
    this.state = {}

    this.storeNames = Object.keys(props.stores)

    this.state.stores = this.storeNames.reduce((stores, name) => {
      const { dispatch, subscribe, getState } = stores[name]
      stores[name] = { dispatch, subscribe, getState, state: getState() }
      return stores
    }, {})

    this.state.contextRegistry = this.storeNames.reduce((registry, name) => {
      registry[name] = React.createContext(null)
      return registry
    }, {})
  }

  componentDidMount() {
    this.storeNames.forEach(name => {
      const store = this.state.stores[name]

      store.subscribe(() => {
        const state = store.getState()

        if (state !== store.state) {
          this.setState({ 
            stores: {
              ...this.state.stores,
              [name]: { ...store, state }
            }
          })
        }
      })
    })
  }

  render() {
    const contexts = this.storeNames.map(name => ({
      Context: this.state.contextRegistry[name],
      store: this.state.stores[name]
    }))

    const PropsChildren = React.createFactory(() => props.children)

    return (
      <ReactReduxRegistryContext.Provider value={this.state.contextRegistry}>
        {contexts.reduceRight((Child, { Context, store }, i) => {
          const ChildWrapped = () => (
            <Context.Provider value={store}>
              <Child />
            </Context.Provider>
          )

          return i === 0 ? <ChildWrapped /> : ChildWrapped
        }, PropsChildren)}
        </ReactReduxRegistryContext.Provider>
    )
  }
}

The long and short of it is we create this:

<ReactReduxRegistryContext.Provider value={this.state.contextRegistry}>
   <Context.Provider value={store1}>
              <Context.Provider value={store2}>
                  <Context.Provider value={store3}>
                      {React.Children.only(this.props.children)}
                  </Context.Provider>
              </Context.Provider>
    </Context.Provider>
</ReactReduxRegistryContext.Provider>

And then ultimately, connect looks something like a modified version of the one I've been working on (https://codesandbox.io/s/64mzx7vjmz?module=%2Fsrc%2Fconnect%2Findex.js):

import { ReactReduxRegistryContext } from './Provider'

function connect(mapState, actions, options = {}) {
  const storeName = options.store || 'main'

  return function wrapWithConnect(WrappedComponent) {
    class Inner extends Component {
      constructor(props) {
        super(props)
        this.actions = actions && bindActionCreators(actions, props.dispatch) // just basic binding for now
        this.state = { selector: memoize(mapState) }
      }

      static getDerivedStateFromProps(nextProps, prevState) {
        const { storeState, props } = nextProps
        const result = prevState.selector(storeState, props)
        return result === prevState.result ? null : { result } // `null` indicates no state changes occurred; also previously before I got the usage of `memoize-state` correct, I had an additional `shallowEqual` check here--so we get that for free from that library
      }

      shouldComponentUpdate(nextProps, nextState) {
        const resultEqual = nextState.result === this.state.result
        return !resultEqual || !shallowEqual(nextProps.props, this.props.props) // of course updates could be triggered not just from store state changes, but also from regular props, and for this POC we assume a pure component
      }

      render() {
        const { props, dispatch } = this.props
        const { result } = this.state
        const mergedProps = { ...props, ...result, ...this.actions, dispatch }
        return createElement(WrappedComponent, mergedProps)
      }
    }

    function Connect(props) {
      return (
        <ReactReduxRegistryContext.Consumer> // statically accessible consumer
          {(registry) => (
            do {
              const Context = registry[storeName]

              ;(
                <Context.Consumer> // dynamically discovered consumers
                  {({ dispatch, state }) => (
                    <Inner dispatch={dispatch} storeState={state} props={props} />
                  )}
                </Context.Consumer>
              )
            }
          )}
        </ReactReduxRegistryContext.Consumer>
      )
    }

    Connect.WrappedComponent = WrappedComponent
    return hoistStatics(Connect, WrappedComponent)
  }
}

@markerikson that's what I meant the other day by using a 2nd "junction provider."

The only thing we can't do is automatically figure out the context names. Developers will have to provide it as an option to all their connect hocs--or, wrap the hoc themselves, and import from one of several userland versions of connect. But that should be fine and makes logical sense.


The one thing this relies on is the new render props context api should not trigger updates on consumers if the value prop is the same, even if the component where the Provider is re-renders for other reasons (i.e. other stores updating). That's a must if we want to achieve the least amount of unnecessary context broadcasting.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Mar 12, 2018

@faceyspacey : appreciate the effort you went to there, but I think it's all a moot point :) I saw some comments indicating that if you nest instances of a Context.Provider, the closest one "wins". So, this should be legal:

<Provider store={store1}>
    <MainApp>
        <Provider store={store2}>
            <NestedApp />
        </Provider>
    </MainApp>
</Provider>
@faceyspacey

This comment has been minimized.

faceyspacey commented Mar 12, 2018

Unless they don’t necessarily want to match the closest parent provider.

That said, if what u just described works, I agree it’s probably best to forget about it. And if people have some complex multi store need, they should just supply the context to both the provider and connect as per #1 above. It’s not that big of a deal, and can be abstracted in userland by wrapping connect in a function that does it for you. Likely context props will become a thing with the new API.

@Lenne231

This comment has been minimized.

Lenne231 commented May 4, 2018

I'm not really into the internals of react-redux and react suspense, but is it possible to put the fetcher/cache into the redux store?

const getData = () => createCache(loadDataAsync);
const reducer = (state = { data: getData() }, action) => {
  switch(action.type) {
    case: 'INVALIDATE': return { ...state, data: getData() };
  }
  return state;
};

The fetcher/cache should be "immutable", i.e. if we want to invalidate the cache, we have to create a new one.

@markerikson

This comment has been minimized.

Contributor

markerikson commented May 4, 2018

@Lenne231 : "possible"? Sure, on the grounds that you can put literally anything into a Redux store. However, we discourage putting anything that's not plain serializable data into the store.

The whole "caching" aspect of using React Suspense isn't entirely clear yet. I know the React team has put out a small reference caching lib, and I know that the general idea is that it's something that should likely be passed down via context and should throw a promise if it is queried and doesn't have the data you want already available. I'm not yet sure on how that concept would relate to a Redux store.

edit: I see someone put this together as an example of how a Redux-based React Suspense cache might work: https://github.com/julianburr/react-suspense-redux-cache . Also ran across https://github.com/alexeyraspopov/redux-suspense , which is related.

@cellog

This comment has been minimized.

Contributor

cellog commented Jul 27, 2018

@gaearon would it be safe to say that React state is consistent in setState callbacks and post-render life cycles?

So would it work to modify react-redux so that dispatch flow is more like:

dispatch(action)
-> get reducer value
calls -> setState(reducer value)
callback/post-render -> update redux state
-> continue synchronously a la old way

In other words, we would delay the synchronous part of the redux so that redux outside of react becomes a subscriber to the state rather than a provider of state that can also push state updates back into react? Do we risk 2-way subscription race condition issues with this idea?

This won't solve the problem of store enhancers, but with enough warning, those (rather limited) examples should have time to update to a new API.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jul 28, 2018

@cellog : that sounds a lot like the "reimplement Redux on top of React" approach that Dan and Andrew have suggested. Problem is, there's tons of code out there that expects the store to have been synchronously updated as soon as dispatch() returns. Here's a basic example of a thunk that relies on that behavior:

function checkStateAfterDispatch() {
    return (dispatch, getState) => {
        const firstState = getState();
        dispatch({type : "FIRST_ACTION"});
        
        const secondState = getState();
        
        if(secondState.someField != firstState.someField) {
            dispatch({type : "SECOND_ACTION"});
        }    
    }
}

My vague train of thought, looking forwards, is that we might have to come up with something akin to Git branching and rebasing with dispatched actions (which is the same metaphor the React team has been using for async rendering). Since we've never guaranteed that the UI layer sees every action (especially for scenarios such as batched / debounced subscriber notifications), we might be able to have the Redux store itself update synchronously, but then have React-Redux do some fancy footwork and actually present a separate "current state" where actions get applied as React does updates. Or something like that. It's still just a vague set of ideas in my head. (I suspect that Brian's interaction tracking API proposal would be relevant here.)

@Kingdutch

This comment has been minimized.

Kingdutch commented Sep 5, 2018

I've been following this discussion as well as playing with React Suspense through the custom build in the React repository. I've also run into the fact that I was unsure on how to use redux with React Suspense because in my previous React apps I stored all data in the Redux store. However, I just had the following realisation.

Is it truly needed to modify Redux for usage with React Suspense? What if we separate state from data? This would allow us to continue doing state management with Redux but move the ownership of data to a simple-cache-provider and simply read data from our data cache on an as-needed basis (either loading it or not).

I would imagine that the state in the Redux store would be something like "Viewing list of blogposts in category Javascript on page 3" (i.e. { view: 'posts', category: 'javascript', page: 2 }. The data would then be the actual posts themselves.

The Posts component would then simply call posts.read({ category: 'javascript', offset: 2 * itemsPerPage } and would use React to either show a fallback or immediately show the data if it was previously cached.

This would still allow the user to change what they're doing by clicking a different button which would update the Redux state and re-render (and possibly load) the new state with different data.

The data could then either be stored in React's simple-cache-provider or a custom cache implementation. When using SSR then this data cache could be serialized and shipped across the wire in a similar fashion to how this currently happens for the redux store.

I'm not sure if this solves all the problems/use cases at the moment but I think it comes a long way there, I'd be interested to hear what you think.

I think if you'd be looking for a dividing line then you'd say that state is defined as anything that is owned by the local application (e.g. the view filter parameters) and data would be (read-only?) data that's owned by a remote source.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Sep 5, 2018

@Kingdutch : a few thoughts.

First, I'm less concerned about the "caching" aspects of Suspense for now, and much more concerned about React's abilities to pause and resume applying updates based on priorities (which conflicts with Redux's assumption that the state is always updated immediately).

Second, while it's entirely up to an app developer what data is in their Redux store, the Flux architecture has always been intended to be useful for developers who want to cache data they've fetched from a server. It's true that there's plenty of other options for handling data fetching and caching, such as Apollo and such, but I certainly wouldn't want to blanket declare that people shouldn't cache data in Redux when there's many Redux users doing that right now.

Third, I really don't see us making changes to the Redux core library itself as part of this. The main conceptual ideas atm seem to be either some kind of store enhancer that better integrates async React behavior + a standard Redux store, or a "reimplementation" of a Redux store that uses React component state as its back end.

Really, at this point we're in something of a holding pattern waiting for the React team to release some demos and further guidance around how to properly write apps and work with Suspense and time slicing, so that we can see exactly what issues that leads to with Redux usage and have something concrete to work on.

@cellog

This comment has been minimized.

Contributor

cellog commented Sep 6, 2018

@Kingdutch thanks for your ideas. Actually, redux functions as a cache, on a global basis, and so creating a version of something that checks to see if redux has loaded state, and throws a promise to load it (or, to be even fancier, throws a promise that dispatches an async action, and listens to the store for the "loaded" action and then resolves the promise) is a feasible option under suspense.

However, as Mark said, the bigger issue is how React will handle state updates under time slicing. There are 2 issues. The first is that rendering can be suspended and restarted, and so any dispatching that occurs in render, or in anything prior to componentDidMount or componentDidUpdate runs the risk of tearing, where redux state is based on outdated information. Thus, react-redux 5.x, which relies upon componentWillReceiveProps is intrinsically not safe for suspense.

The second issue is a bit more interesting, and has to do with how React will prioritize both rendering and local state updates. There are a couple of strange new functions in React, unstable_deferredUpdates and unstable_interactiveUpdates which are designed for time slicing in async mode. Deferred updates are less critical state changes, and interactive are unimportant state changes. React handles this by assigning each fiber a timeout value. It processes fibers in the order of shorter timeouts first, then longer. If higher priority fibers finish first, then the lower priority ones get handled. unstable_deferredUpdates sets the timeout to 100ms, and unstable_interactiveUpdates sets it to something like 800ms (I forget the exact number). So anything executed inside unstable_deferredUpdates or unstable_interactiveUpdates will modify the fibers created in order to change the timeouts. Thus, if you dispatch a setState and a redux dispatch in the same place, the redux dispatch will execute synchronously, and the setState will not.

As for how this will affect redux, honestly, I don't think it does, EXCEPT for when we handle async. So, if a user dispatches a load action and a select action at the same time as the load, there is a chance (with suspense) that the user would select something else and dispatch a different load/select action. If the code is designed so that the select happens after the load is concluded, this will select the option the user requested first, which would be incorrect. So the key will be to dispatch actions at the correct time to update redux state. However, redux apps that are not designed for suspense and use thunks, sagas, or epics for async will need to be redesigned to take advantage of suspense. There will be no drop-in for these situations. Suspense essentially replaces async middleware.

The short version: it's complex, but redux is probably async-ready, it's the async middleware that will be an issue, as it won't play nice with suspense. Also, because redux doesn't have batching built-in, it may be less performant under suspense without modifications to streamline it, although I suspect this won't be an issue ultimately, because React context (new one) batches updates to the state.

@Kingdutch

This comment has been minimized.

Kingdutch commented Sep 9, 2018

Thanks for the thorough answers @markerikson and @cellog! Really helps for understanding the problem area. I think I was mostly focused on the cache aspect of Redux as @cellog mentions in his first paragraph.

I indeed found that for a previous React/Redux app I was using redux-thunk for loading async data but I found that when playing around with Suspense I no longer needed it because throwing a promise essentially achieved the same things.

I wanted to highlight a line from @cellog: Thus, if you dispatch a setState and a redux dispatch in the same place, the redux dispatch will execute synchronously, and the setState will not.

This makes me believe that this is a problem that Redux can't solve (unless they were to hook into React's scheduler in some way to ensure order of start equals order of resolution) because you're firing off two uncoordinated dependent asynchronous requests. This would create a race condition in any system. The problem would then be that there's no single owner of the potentially conflicting state which should probably be prevented in userspace (even though I understand you don't want people to be able to shoot themselves in the foot like that).

Picking from @markerikson's comment a "reimplementation" of a Redux store that uses React component state as its back end would probably ensure that React maintains all state and could possibly solve the above as Mark stated.

As for waiting for the demo's: I assume you're both aware of the https://github.com/facebook/react/tree/master/fixtures/unstable-async sub-repo's that can be used to play with these things already? (It's what I've been using for the above mentioned experimentation).

@markerikson

This comment has been minimized.

Contributor

markerikson commented Sep 9, 2018

@Kingdutch : that actually kind of nails the situation as I understand it.

With the Redux core, calling dispatch() guarantees that the state has been updated and subscribers have been notified by the time the call returns. This means that a thunk can dispatch an action and immediately call getState() to retrieve the updated state afterwards.

Now, middleware and store enhancers do change that equation. Any middleware can intercept, pause, delay, stop, modify, or replace any dispatched action. Similarly, a store enhancer can override dispatch, getState, and subscribe, and provide its own implementation, such as debouncing the actual notification of subscribers.

So, it's not out of the question that we could have some kind of magic "React-Redux synchronizing store enhancer" that somehow interfaces between React and async React-Redux, but if so it will be very tricky.

@cellog

This comment has been minimized.

Contributor

cellog commented Sep 10, 2018

Here's the thing. A redux app that does not make any changes to use suspense or prioritization of setState will continue to work exactly as designed. The only situation where we could get into issues is if someone tries to mix and match suspense and async middleware.

This situation is most easily solved with a 2-pronged approach:

  1. document a recommended approach to using redux with the new async and time slicing features
  2. warn people not to shoot themselves in the foot by blending middleware and suspense
  3. make sure we have react-redux 6 released, which will avoid componentWillReceiveProps, because react-redux 5 will blow up in some circumstances with suspense even without async middleware in the mix.

The first thing can also include an implementation of simple-cache-provider that uses redux as the cache. Problem solved.

Frankly, I think it's best to point out that older async solutions will work, but you don't get any of the benefits of taking advantage of React's scheduler. It's not like redux will suddenly explode in React 17.

However, I am playing around with a redux-like thing based on using setState, and if it bears fruit will certainly report what I learn.

@timdorr

This comment has been minimized.

Member

timdorr commented Nov 6, 2018

We should be in a good spot for this with #1000 now on master and later on with #1063.

@timdorr timdorr closed this Nov 6, 2018

@vincentjames501

This comment has been minimized.

vincentjames501 commented Dec 14, 2018

@timdorr, is there any guidance or wisdom to actually use Suspense with React Redux? #1000 is great but doesn't answer the question to this thread. #1063 will also be amazing but I'm still unsure how hooks would specifically address suspense and asynchronous rendering.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Dec 14, 2018

@vincentjames501: no, we really haven't investigated the Suspense side. Part of that is because Suspense for data fetching is still a ways off. See the React team's roadmap here: https://reactjs.org/blog/2018/11/27/react-16-roadmap.html .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment