Skip to content
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

React 16 experiment: rewrite React-Redux to use new context API #898

Closed
wants to merge 5 commits into
base: master
from

Conversation

@markerikson
Copy link
Contributor

markerikson commented Mar 9, 2018

FOR DISCUSSION ONLY - DO NOT MERGE

Per discussion in #890 , React-Redux will need to change to support React's upcoming async rendering behavior.

I spent this afternoon experimenting with an update of our current v5 codebase to use the new React.createContext() API that will be out in React 16. To my very pleasant surprise, it actually seems to be working out better than I thought it would.

Summary of Changes

  • Previously, every single connected component instance subscribed to the store separately, as implemented in connectAdvanced. Now, <Provider> subscribes to the store, and calls this.setState({storeState}) in the subscription callback (sorta similar to how the v4 connect implementation did things).
  • React-Redux calls React.createContext() once internally to create a single ReactReduxContext provider/consumer pair. Both <Provider> and connectAdvanced use that pair. All references to contextTypes have been removed.
  • Since the connectAdvanced wrappers now directly use the context pair, the entire Subscription concept has been removed
  • connectAdvanced.componentWillReceiveProps was renamed to UNSAFE_componentWillReceiveProps. (Note that I'm not sure if this is still needed.)
  • The selector init/update logic was modified to take the current store state as an argument, rather than closing over the store directly.
  • The main call to the memoized selector was moved inside of the new renderChild render prop callback.
  • Connected components no longer accept store as a prop, because they don't/can't subscribe to it anyway. There could be use cases where this is still desirable in some way (particularly for testing?). We may need to investigate alternatives, such as exporting a createReactReduxContext() function, or possibly allowing <Provider contextProvider={myContext.Provider}> somehow.

Observations and Testing

Tests

I made a few quick edits to our unit test suite, primarily changing ProviderMock to point to the actual Provider class instead. Right now we've got 25 failing unit tests, but I think that most of them are likely to no longer be relevant (such as "should unsubscribe before unmounting").

Main Testing Setup

I cloned and built facebook/react@fcc4f52. I then used yarn link with the built copies of react and react-dom inside of build/node_modules, and used yarn link on my local clone of react-redux.

I then opened up the local repo for my Project Mini-Mek sample application, and used yarn link to load in react, react-dom, and react-redux.

Main Testing Observations

When I start and load Project Mini-Mek, the application appears to correctly load and run as expected. I can click through the tabs, load the sample data, select pilot entries, and edit a pilot entry, with no obvious errors in the console or the UI.

If I view which components are updating and re-rendering with a combination of turning on "Highlight Updates" in the React DevTools, and adding console logging to connectAdvanced.renderChild()), I see only the components that I expect to re-render updating. In particular, the list items in the current HEAD of Project Mini-Mek are not currently using memoized selectors. I tried optimizing them via the makeMapState() technique, and saw that they no longer are re-rendering on every store state update.

Other Testing Observations

Before modifying React-Redux v5, I threw together a quick tiny version of the "Provider subscribes" approach separately, and whipped up a tiny Redux counter example in a CRA app. I then compared behavior between dispatching 1 "INCREMENT" action and dispatching 5 `"INCREMENT" actions in a row, in both React click event handlers and non-React DOM element click handlers.

I observed that dispatching 5 actions in a row in a React event handler only caused the Provider and the connected component to both re-render once, as React was batching up its setState() calls. However, dispatching five times in a row from a DOM element click handler caused five separate Provider -> connect -> component update cycles, as the setState() calls were not batched.

Finally, I tried adding a test key to the Project Mini-Mek Redux store, had a component subscribe to that key, and then created a second Redux store instance with the same key but a different value. I then had my app render a nested <Provider store={secondStore}>, and had a test component dispatch an action. The nested test component correctly picked up the data from the second store, and the dispatched action updated the second store.

Conclusion

Given that I've only hacked on this for the last few hours, this actually appears to be working out way better than I had anticipated. I'm sure there's all kinds of edge cases I haven't run into yet, but this looks like a viable first attempt at a React-Redux v6 that would be compatible with React 16.3+.

@gaearon, @acdlite, @bvaughn , @timdorr, @jimbolla : so, how many ways is this broken right now? :)

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 9, 2018

Also, while I haven't tried this yet: I'm seriously considering the idea of leveraging the calculateChangedBits / observedBits aspect of the new context API to implement as a first-approximation performance optimization.

The idea would be to have Provider do a shallow equality check and determine which top-level keys had changed, and return a bitmask from calculateChangedBits using a "minimal perfect hash" to determine how the key names map to the bits in the bitmask. Meanwhile, connectAdvanced would need to do something fancy internally to determine which top-level keys its mapState function actually accesses. I'm thinking this could be done by wrapping the state value in a Proxy when mapState is called, and tracking which keys are accessed. I've seen at least two or three examples of the "use a proxy to track what's accessed" concept lately, so it seems feasible. Not sure what the runtime perf benefits or negatives would be on that.

I briefly played with the "minimal perfect hashing" idea in a sandbox a couple days ago, and it seemed that I could get string keys to consistently map to the same output bits, so that seems like it'd be feasible too.

@jimbolla

This comment has been minimized.

Copy link
Contributor

jimbolla commented Mar 9, 2018

So the whole need to subscribe every component was in case an intermediate component implements shouldComponentUpdate() and returns false.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 10, 2018

Other side note: it appears that Context.Provider and Context.Consumer now appear in the React DevTools component tree. We are really going to need an option to hide those, because the depth of a Redux-connected component tree just grew considerably and it's annoying :(

(Existing open issue: facebook/react-devtools#604 )

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Mar 10, 2018

I'd check out my PR on React Router: ReactTraining/react-router#5908

Some things of note that I'd recommend doing:

  • First, tab size should be 2 spaces. And install Prettier 😛
  • You can polyfill the new context API on older Reacts (16.0-16.2) with create-react-context.
  • Leave the older context API in place. Less breakage that way. Then just pass the return of that function into your Provider's value. Makes it a little easier to implement that way.
  • Rebase this on #856 so we can take advantage of those improvements. Less work is needed here as a result too.
@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 10, 2018

I don't think I can meaningfully rebase this on top of 856 - some of the changes I made directly conflict with the ones in 856. 856 is still trying to directly subscribe to the store in connect, 898 isn't. Also, 856 removes makeSelectorStateful, I kept it and modified it.

I don't think I can maintain the old context API in here either, because connect isn't getting the store at all.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 10, 2018

Are you going to export context adapters? Someone could need the direct access to the “current” store, or reproviding it(and not only me).

About hashing in areStatesEqual - first you need to “perfect hash” the changes, next you can use the “change masks” from connected components to early reject the changes.
But - how many collisions you gonna get in in only 32 possible masks?
32 top level keys are good enough, but hashing could reduce it to 6 values in the real case with easy.
I’ll better keep the track of top level keys(usually they never change), and count and use the order number as a bit mask. Close to zero collisions in result

As a early rejection itself - I’ve already skipped idea in my beautiful-react-redux, as long I have that proxy underneath. Sounds promising.
The only 2 concerns - Android(no proxy support, needs polyfill), and the amount of time a could spent in areStatesEqual.
Especially having in mind changes in this PR, when you are already inside react render loop and early rejection will not save some “React cycles”.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 10, 2018

Looks awesome. @markerikson I had a hunch you were gonna work on this when I saw your tweets.

I think this is a really good first draft that does a good job preserving the old stuff (i.e. for backwards compatibility), but I think if we were able to start fresh and if React were still to require so many little workarounds to beam global state efficiently to individual components, the new Context API has missed the mark.

react-redux really should be a use-case where the new Context API shines; and not just making this more performant, but also simplifying the implementation. It's basically black magic to anyone that doesn't spend hours analyzing it, and that shouldn't be the case for something doing relatively little.

I wonder what the start fresh approach would look like, if the primary goal was to harness new APIs as much as possible...


...after about an hour of tinkering, I've come up with the following basic format, which seems congruent with how the new context api is best put to use. Additional functionality can be brought back once the basic structure emerges. Hopefully somehow this time around thanks to the new context API + getDerivedStateFromProps API we don't need as many workarounds.

export default function connectAdvanced(selectorFactory, options) {
  return function wrapWithConnect(WrappedComponent) {
    class Inner extends Component {
      constructor(props) {
        super(props)
        this.selector = selectorFactory(props.dispatch, options)
      }

      static getDerivedStateFromProps(nextProps, prevState) {
        const { dispatch, state, ...props } = nextProps
        return this.selector(state, props, prevState) // returns `null` if no changes; `prevState` (i.e. previous props to our wrapped component) used for comparisons; no need to make "stateful" ourselves.
      }

      addExtraProps(props) {/* the usual */}

      render() {
        return createElement(WrappedComponent, this.addExtraProps(this.state)) // <-- feels right
      }
    }

    function Connect(props) {
      return (
        <ReactReduxContext.Consumer>
          {({ dispatch, state }) => <Inner dispatch={dispatch} state={state} {...props} />}
        </ReactReduxContext.Consumer>
      )
    }

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

The hallmark of this approach is 3 components, rather than 2; and a transfer of context "state" from the 1st component to regular state of the 2nd component, to props of the 3rd.

The new Context API allows us to use the latest best practices for re-rendering based on incoming props, but in this case using our entire store state as props.

FINAL NOTE: back to my original point that "react-redux really should be a use-case where the new Context API shines": the new Context API would benefit from having a non <Consumer /> render props API as well. So one example of what that API would need is the following signature for getDerivedStateFromProps:

static contextConsumers = [ReactReduxContext.Consumer]
static getDerivedStateFromProps(nextProps, nextContexts, prevState) { 

That would eliminate the need for 3 components, which was less than ideal. And the official way to get access to contexts would in fact be in getDerivedStateFromProps, via transfer of global/parent context to local state. Like, that "transfer" would become a thing in react world.

ps. I wonder how this compares perf-wise to @jimbolla 's major optimizations a year and a half ago.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 10, 2018

Here's the hypothetical/additional context API in action:

export default function connectAdvanced(selectorFactory, options) {
  return function wrapWithConnect(WrappedComponent) {
    class Connect extends Component {
      static contextConsumers = [ReactReduxContext.Consumer]
    
      static getDerivedStateFromProps(nextProps, [context], prevState) { // perhaps 3 arg signature is only used when `static contextConsumers` is present
        const { dispatch, state } = context
        const selector = selectorFactory(dispatch, options) // wouldn't have to do this if we could get `dispatch` in constructor -- otherwise we can cache it on component state
        return selector(state, nextProps, prevState)
      }

      addExtraProps(props) {/* the usual */}

      render() {
        return createElement(WrappedComponent, this.addExtraProps(this.state))
      }
    }

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

Ideally this task is as simple as that, minus things like HMR, etc.

I never really thought it made sense that there was only a render props API for context--aside from clarity of "messaging"--when a new static properties API could also coexist with the old one, provided this isn't a huge implementation hurdle in React internals. At least on the consumer side this seems like a must.

@pierr

This comment has been minimized.

Copy link

pierr commented Mar 10, 2018

This is great

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 10, 2018

@faceyspacey : yeah, I agree that an approach that requires 3 components is not preferable. As it is, I'm already annoyed at all the extra Context.Consumer entries that are going to show up in the component tree (per my comment earlier).

I'm definitely curious about the performance aspects. Since we're no longer subscribing in every component, there's certainly fewer actual subscribers to the store. I haven't implemented any perf optimizations yet other than shuffling around the existing this.selector usage, but we could actually put the initial "has the root state changed?' check directly in <Provider>'s subscription callback:

store.subscribe( () => {
    const storeState = store.getState();
    if(storeState !== this.state.storeState) {
        this.setState({storeState});
    }
});

That would avoid even triggering the consumers if the state hasn't changed.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 10, 2018

Btw: so why the context format should be changed? Why it could not hold the “old” store and let components “subscribe” in an old way?
Using context to transfer the state just moves logic from redux to react, but not changing the idea.
And the all you get - less control, and less speed, as long internally context is far more complex thing than simple redux’s subscribe.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 10, 2018

@theKashey we have to jump through considerable hoops to make sure subscriptions cascade efficiently. In older versions of react-redux performance was a major issue as a tree would have many unnecessary re-renders as connected parents re-rendered connected children multiple times. So perf isn’t just about doing it outside of React—because if our work doesn’t parallel the React’s rendering algorithm, we will in fact trigger more renders.

So similarly we would hope that the new context api finds the absolute most performant and intelligent algorithm to re-render all children using the same context the least number of times, among other optimizations we can’t do outside of React. By relying on React to perform this task, we can insure this stays performant as React evolves, while being able to keep this library minimal and easy to reason about.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 10, 2018

@theKashey : because the main concern here is not actually performance, but rather ensuring that the entire React component tree consistently sees the same "current Redux state" as components re-render at slightly different times. In order to do that, we need to have the current state being passed down from the <Provider>, rather than having the store get passed down and each component subscribing to the store separately. That way, we'll hopefully avoid the "tearing" situation that Andrew has talked about.

If you read the discussion in #890 , you'll see that Dan and Andrew have been talking about actually "reimplementing the Redux store API on top of React", in order to fully make use of React's internal scheduling behavior. I'm hoping that this approach will prove to be an acceptable alternative to that.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 11, 2018

@markerikson just went through all of #890 , and it really doesn't seem like the strategy to "reimplement the Redux store API on top of React" makes much sense anymore once you discovered passing down the state as context. Would you agree? Basically, between that and what I wrote above, it's painstakingly clear to me exactly how this is supposed to work. I don't see how the storeRef stuff @gaearon pointed out is actually needed. However, it does meet @gaearon 's stated goal here:

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.

As far as suspense and "tearing"--which I assume means when different parts of the page use stale state--it shouldn't really matter if part of the page is out of sync while something loads, as when putting all state in context, it's React Async's job to make sure they use the context state that existed previously. I.e. rather than update a suspended leaf because the context it depended on updated more recently.

In general, the suspense stuff seems unrelated, though I think @timdorr came up with a powerful connector concept for global state stores and React Async:

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.

However that's basically a different Redux library, not just in regards to built-in promise support, but one that has automatic "loading" state handling. I think there's something to the idea, but no library can sweepingly make the decision for you that you should start showing spinners, i.e. suspend all connected components, or suspend a top level component containing most of your app. Redux-coupled components can check if a specified state is available, and throw if not, triggering <Placeholder />. Middleware can take part in the action in various ways without needing for the Redux chain to be async. For example, a code splitting middleware library could bring along both a middleware, reducer and component, and on import() trigger state that makes the component throw, which is later successfully rendered when import() resolves. While I'm sure other libs will emerge to make it easier on middleware, Redux can still cope with the current suspended landscape very well.

In short, I think what we're trying to do here revolves around the new Context API + getDerivedStateFromProps and if used correctly, it's React Async's job to render it all properly.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 11, 2018

@theKashey the funny thing about your amazing memoize-state lib is it seems we could get built-in memoization and never have to use reselect again:

class Connect extends Component {
      constructor(props) {
        super(props)
        this.selector = memoize(selectorFactory(props.dispatch, options))
      }

     static getDerivedStateFromProps(nextProps, prevState) {
        const { dispatch, storeState, ...props } = nextProps
        return this.selector(storeState, props, prevState)
      }

That's how many people first looking at react-redux think they can just use it for best performance. Thanks to your lib, we may be able to give them their dream.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 11, 2018

@faceyspacey : well, there are more reasons for using selectors than just performance - they're also beneficial for state shape encapsulation, and as a place to put state derivation logic.

Couple other quick thoughts:

  • I know I've seen comments that there's still several meaningful environments out there that don't support ES6 Proxies. Thus far, React-Redux hasn't used anything that couldn't be compiled back to ES5, so bumping that up would be something we'd have to discuss.
  • Your getDerivedStateFromProps examples appear to have an issue in that they refer to this.selector, which won't work because it's a static function and therefore doesn't have access to this.selector.
@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 11, 2018

@markerikson , you’re right, my mistake. The one based on the "hypothetical/additional" static context api didn't use this, and as commented in the code could be cached. Here's what a cached selector would look like in the current render props context api:

export default function connectAdvanced(selectorFactory, options) {
  return function wrapWithConnect(WrappedComponent) {
    class Inner extends Component {
      static getDerivedStateFromProps(nextProps, prev) {
        const { dispatch, storeState, ...props } = nextProps
        const hasSelector = prev && prev.selector
        const selector = hasSelector ? prev.selector : selectorFactory(dispatch, options)
        const state = selector(storeState, props, prev) 

        if (!prevSelector) state.selector = selector
        return state
      }

      addExtraProps(props) {/* the usual */}

      render() {
        return createElement(WrappedComponent, this.addExtraProps(this.state)) // <-- feels right
      }
    }

    function Connect(props) {
      return (
        <ReactReduxContext.Consumer>
          {({ dispatch, storeState }) => (
              <Inner dispatch={dispatch} storeState={storeState} {...props} />
          )}
        </ReactReduxContext.Consumer>
      )
    }

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

So essentially, on the first run, we use the selector factory, but thereafter, we use the cached version.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 11, 2018

Here's a demo of the above stuff working with the Redux-First Router demo on codesandbox:

https://codesandbox.io/s/64mzx7vjmz?module=%2Fsrc%2Fconnect%2Findex.js

"Standard" context + getDerivedStateFromProps usage seems to be working as advertised.

You'll notice not just userland memoization has been offloaded but also some of the internal shallowEqual comparison work to @theKashey 's memoize-state library. I primarily did it just to simplify making a quick demo and seeing how this structure would pan out, and guess what? It worked swimmingly! I did quite a few checks on the re-renderings, tweaked things, repeat, and it's looking pretty good.

The next steps would be to take this example and bring back all the potential signatures connect and connectAdvanced can take, and remove the fun memoize thing. Then the exciting part of running @jimbolla 's benchmarks from a while back to see how using Context to "broadcast" a single subscription compares to the old route of many subscriptions. Fingers crossed :)


The primary connected components to checkout are:

  • /components/Sidebar.js
  • /components/Switcher.js
  • /components/List.js
  • /components/Video.js

ps. it will be interesting to see if "tearing" is no longer an issue, which it absolutely shouldn't be, since this is idiomatic context usage.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 11, 2018

@faceyspacey - usually then you want to Map something - you use mapStateToProps, and it should be memoize. If you want to dispatch - mapDispatch, and it could be memoized, but usually - not. If you want to mix them - you use mergeProps.
Divide and Conquer.

Redux + memoize-state related talks have moved to theKashey/memoize-state#3

@theKashey

This comment has been minimized.

Copy link

theKashey commented Mar 12, 2018

@markerikson is absolutely right about "structuring" the access using selectors - cascades could "share" memorization, and just beautiful, but sometimes it is not easy to use them - for example in "adapters"
Lets imagine you have some complex structure, and have to extract the "meta data" from it - it is loading, in error, displayable and so on. To make memoization work you have to "select" all "spare parts", which is not a problem, but changes the way you shall write the function. That's the problem - "need of mind shift".
Actually this is why I love redux - it forces you to structure things.

I know I've seen comments that there's still several meaningful environments out there that don't support ES6 Proxies.

I've just tested - ES5 polyfill (ie describe property +get) is just 5% slower.

What else - even proxies and magic stuff around in production is discussable thing, it is still possible to do another trick - use memoize state in dev mode, to double check that user have mapStateToProps properly memoized. Ie if result got changed, but should not - console.error('Please check the function') (or more complex thing - "purity check")

@ericanderson

This comment has been minimized.

Copy link
Contributor

ericanderson commented Mar 13, 2018

@markerikson I think you could very easily extend this to support multiple stores; Instead of creating one Provider/Context, you create a map of storeKey: {Provider,Context}.

@faceyspacey

This comment has been minimized.

Copy link

faceyspacey commented Mar 19, 2018

@markerikson you check out the demo? What you think--you think we'll be able to get rid of a lot of what made this library so intimidating in the past?

I'm also curious as to whether this eliminates the additional work regarding @gaearon 's storeRef idea, tearing, etc. Unless there's something I'm not privy to, it seems like it just works.

Here's the link again:
https://codesandbox.io/s/64mzx7vjmz?module=%2Fsrc%2Fconnect%2Findex.js

ps. didn't mean to hi-jack your PR, just seemed like the best and only place to discuss this and it was something I had been thinking about for a while.

@coryb08

This comment has been minimized.

Copy link

coryb08 commented Mar 20, 2018

@markerikson re the last point on summary of changes:

Connected components no longer accept store as a prop, because they don't/can't subscribe to it anyway. There could be use cases where this is still desirable in some way (particularly for testing?). We may need to investigate alternatives, such as exporting a createReactReduxContext() function, or possibly allowing somehow.

For testing, couldnt we just manually pass store to connected components as a prop? or is that way too cumbersome/out of the question..?

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Mar 21, 2018

@coryb08 : I'm saying that in this PR, that capability no longer exists. Based on some Twitter feedback, sounds like it's getting some active use, so I'll see if I can re-implement the capability in this PR.

@faceyspacey : uh... what do you mean by "what made this library so intimidating" ?

And yes, the idea behind this PR is that it would hopefully work correctly with async rendering without the tearing issues. I'm hoping to talk to the React team this week to get some feedback on the approach.

@elisherer

This comment has been minimized.

Copy link

elisherer commented Apr 13, 2018

@timdorr, that is exactly the case in react-waterfall.
The problem I see with this is to enable third parties create automatically connected components.
In this "new generation", every component would have to get a copy of the store in order to work (or be connected manually).

Hypothetic example:|

//before
import { reduxForm, Field } from 'redux-form';
...
<Field name="firstName"/>
...
export default reduxForm(config)(MyForm);

//after
import { reduxForm, Field } from 'redux-form';
...
<Field name="firstName" store={store} />
...
export default reduxForm(store, config)(MyForm);

Creating a pair of Provider/connect would fit the Context API but would break the redux "automagic".

@theKashey

This comment has been minimized.

Copy link

theKashey commented Apr 14, 2018

Bad idea, as long (reusable/third party) component should just establish a connection to the (user defined) store, but could not obtain a reference to it (read - import '../../createStore.js'), as long isolated.
There should be a straight way to connect to the "default" endpoint.

@TrySound TrySound referenced this pull request Apr 19, 2018

Closed

using new context API #927


// TODO How do we express the invariant of needing a Provider when it's used in render()?

This comment has been minimized.

@timdorr

timdorr Apr 20, 2018

Member

The Consumer of createContext will take on the default value when it's not under the paired Provider. So, I think just leaving this invariant in place would do.

This comment has been minimized.

@timdorr
@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Apr 20, 2018

I've spent the last couple months prepping for a workshop, which just wrapped up. I need a bit of down time to try to recuperate, but after that I want to come back and look at both this PR and my starter kit package, and see about pushing things along.

@Hypnosphi

This comment has been minimized.

Copy link
Contributor

Hypnosphi commented Apr 21, 2018

You can polyfill the new context API on older Reacts (16.0-16.2) with create-react-context.

Actually, the polyfill works with React 15 as well

bors bot added a commit to jser/jser.github.io that referenced this pull request Apr 23, 2018

Merge #509
509: 2018-04-23のJS: Chrome 66、Redux 4.0、Svelte 2.0 r=azu a=azu

* [New in Chrome 66  |  Web  |  Google Developers](https://developers.google.com/web/updates/2018/04/nic66 "New in Chrome 66  |  Web  |  Google Developers")
* [Chrome Platform Status](https://www.chromestatus.com/features#milestone%3D66 "Chrome Platform Status")
* [Chromium Blog: Chrome 66 Beta: CSS Typed Object Model, Async Clipboard API, AudioWorklet](https://blog.chromium.org/2018/03/chrome-66-beta-css-typed-object-model.html "Chromium Blog: Chrome 66 Beta: CSS Typed Object Model, Async Clipboard API, AudioWorklet")
* [Svelte v2 is out!](https://svelte.technology/blog/version-2 "Svelte v2 is out!")
* [Release v4.0.0 · reactjs/redux](https://github.com/reactjs/redux/releases/tag/v4.0.0 "Release v4.0.0 · reactjs/redux")
* [React 16 experiment: rewrite React-Redux to use new context API by markerikson · Pull Request #898 · reactjs/react-redux](reduxjs/react-redux#898 "React 16 experiment: rewrite React-Redux to use new context API by markerikson · Pull Request #898 · reactjs/react-redux")


Co-authored-by: azu <azuciao@gmail.com>
Co-authored-by: azu <azu@users.noreply.github.com>
@0rvar

This comment has been minimized.

Copy link

0rvar commented Jun 4, 2018

What is the status here?

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Jun 4, 2018

@0rvar : see my roundup in #950 . I haven't had time to come back to the situation since I posted that.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Jul 6, 2018

Noting this here as a scratchpad for later reference. A user on HN commented that React-Redux makes a 3D VR app on mobile devices a lot slower. Could be an interesting benchmark overall, as well as a good perf comparison overall when I manage to push this forward:

https://news.ycombinator.com/item?id=17474305

It shouldn't but a few dozen of updates per second through Redux on a mobile take a toll on its performance serious enough to not be able to keep 60fps in a WebGL context.

I decided to remove Redux after noticing the performance overhead per function call in the Chrome performance profiler, so I know that in practice "dispatch" and "mapState" are not comparable to just editing or grabbing "a couple values from the Redux state"

My ex-coworker had the same issues because he needed to display a lot of items handled collaboratively with smartphones. In both of our cases we have a high rate of data updates, even if we optimize like crazy we are never going to fall under 4-5 update/second per connected user.

A good way to test it would be to make a simple websocket server that simulate n users each sending n' fake xyz position and rotation data per second. This data updates a Redux store in a React app made with Aframe (HTML wrapper around ThreeJS). You make n cubes move and rotate along the data in the store. Compare the fps you get against the fps you get with a vanilla solution. Also check what happens on the performance tab of chrome

@Kalkut

This comment has been minimized.

Copy link

Kalkut commented Jul 7, 2018

@markerikson I ended up implementing the benchmark with screenshots of the performance profile.

The performance drop after dispatch calls is real but it is not an inefficiency of Redux.

It is due to the fact that behind a call to dispatch there is a call to this.setState. and this.setState is heavy. I just think that apps and features like the one in the benchmark should not be made with React in the first place. It is preferable to wrap such an app in a React component after having built it in an other way that allows more mutability.

Now using Redux or something else as a data store is totally orthogonal so dispatch does not cause any inefficiency per se. It does when coupled with React.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Jul 7, 2018

@Kalkut : Thanks, really appreciate it!

Yeah, we've always said that the act of dispatching and running the root reducer is unlikely to be the bottleneck in most apps - it's usually the process of updating the UI instead.

I know that @jimbolla had some benchmark repos he worked with when he was writing React-Redux v5. We probably ought to do something about collecting some benchmarks in one place for future reference.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

markerikson commented Aug 15, 2018

This PR has been superseded by the discussions and changes in #950, #995, and #1000 , so I'm closing it.

@naholyr

This comment has been minimized.

Copy link

naholyr commented on 576f3f2 Sep 24, 2018

I'm scratching my head on this one as I was trying to use new Context API to write redux-like Provider and connect(), but I think it can just not work with server-side rendering :(

The most important thing is that one root component == one store, so you can call createStore for each request and render your component with given store in the tree, without fearing for race conditions between two independent requests which would use the same data.

That was OK with old Context API, as it was attached to the actual tree. Now it's attached to an external object we're doomed: in your current implementation (and I could not find better idea), you have only one global context, so if you have two trees, using <Provider store={store}>…, with two different stores, you will face race conditions as the first tree will suddenly be udpated by the second one.

It will work now, because between createElement() and renderToString() everything is synchronous, but that smells bad :(

Do you have any idea how to solve this?

This comment has been minimized.

Copy link
Member

timdorr replied Sep 24, 2018

@naholyr Nothing changes on your end. The same APIs exist for getting your per-request store into the component tree (renderToString(<Provider store={store}/>...). This doesn't affect server rendering at all.

@timdorr timdorr deleted the v6-react-16-rewrite-experiment branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.