mobxjs / mobx-react-lite

Lightweight React bindings for MobX based on React 16.8 and Hooks
https://mobx.js.org/react-integration.html
MIT License
2.13k stars 90 forks source link

New Context - Performance #62

Closed svkgit closed 5 years ago

svkgit commented 5 years ago

I’m trying to figure out how to handle global state in a new application that is using mobx, mobx-react-lite, and react hooks. The basic plan (what I would like to do) is to have the global state available via context and then use hooks in components to select/filter data and initiate mutations. The question/issue revolves around using react’s new context and possible performance implications. I’ve been following discussions on the react-redux project and elsewhere and there seems to be a concern with how context currently works. The issues are best described by Mark Erikson here.

Here is the relevant portion:

Context API Limitations

At first glance, React's new createContext API appears to be perfectly suited for the needs of a library like React-Redux. It's built into React, it was described as "production-ready" when it was released, it's designed for making values available to deeply-nested components in the tree, and React handles ordering the updates in a top-down sequence. The <Context.Provider> usage even looks very similar to React-Redux's <Provider>.

Unfortunately, further usage has shown that context is not as well suited for our use case as we first thought. To specifically quote Sebastian Markbage:

My personal summary is that new context is ready to be used for low frequency unlikely updates (like locale/theme). It's also good to use it in the same way as old context was used. I.e. for static values and then propagate updates through subscriptions. It's not ready to be used as a replacement for all Flux-like state propagation.

React-Redux and Hooks

In addition to concerns about performance and state update behaviors, the initial release of the React Hooks API will not include a way to bail out of updates triggered by a context value update. This effectively blocks React-Redux from being able to ship some form of a useRedux() hook based on our current v6 implementation.

To again quote Sebastian:

I realize that you've been put in a particularly bad spot because of discussions with our team about concurrent mode. I apologize for that. We've leaned on the use of context for propagation which lead you down the route of react-redux v6. I think it's generally the right direction but it might be a bit premature. Most existing solutions won't have trouble migrating to hooks since they'll just keep relying on subscriptions and won't be concurrent mode compatible anyway. Since you're on the bleeding edge, you're stuck in a bad combination of constraints for this first release. Hooks might be missing the bailout of context that you had in classes and you've already switched off subscriptions. :(

I'll pull out one specific sentence there for emphasis:

I think [v6's use of context is] generally the right direction but it might be a bit premature.

Does mobx-react-lite workaround this somehow or is this somehow not relevant to mobx?

xaviergonz commented 5 years ago

Basically you'd usually use an object (an observable store), which does not change its reference. The update process is done through mobx reactions that trigger a component update when the accessed variables of such object (or sub objects change).

To make it short, context API will most likely never trigger an update itself (since the object reference won't change), it will be mobx triggering those updates.

svkgit commented 5 years ago

I see. So is it fair to say that the difference maker is mobx use of a mutable store (thus the reference doesn't change) vs the redux store that is immutable and therefore changes with each state change?

xaviergonz commented 5 years ago

You got it :)

danielkcz commented 5 years ago

Well said @xaviergonz 👍 Might be worth to add a note to README portion talking about this.

I really don't envy people stuck with Redux that have to deal with stuff like that. I think that MobX is so much better and it's even easier to learn once you get over the "magic" part. It's kinda pity it is not getting bigger attention.

@svkgit Feel free to reopen if you got more questions on the topic.

danielkcz commented 5 years ago

Today, I've published an article partially inspired by this issue :)

https://levelup.gitconnected.com/graceful-graphql-with-react-hooks-and-mobx-38b66e8d0ee7

joenoon commented 5 years ago

@FredyC another excellent article, thanks! Your formik article inspired me to build that out further and I now have a custom useForm hook similar to your example that entirely replaces the need for formik and is very straightforward - forms have never been so easy. I think one of the less obvious benefits to hooks is really the typescript implications. Until hooks getting typings to play nice via hocs or render props required either lots of voodoo or just settling for losing some typings to avoid the extra work. With hooks, I've noticed typings, inferred and all, just work out of the box. I think in the upcoming HOC/renderprop conversations that may be a key point as I'm still seeing many people say they can do X with HOC/renderprop but not with Hooks. In reality they can likely also do X with Hooks AND get seamless typescript integration, and they actually can't do X with HOC/renderprops and get seamless typescript integration. There are probably a lot of people who didn't realize they were missing/discarding lots of type information to begin with.

danielkcz commented 5 years ago

@joenoon I am so glad to hear that my article has inspired you. That was pretty much my intention :)

You are definitely right about TypeScript. Hooks are just plain functions, after all, which are a first-class citizen in the JavaScript. They have to work flawlessly.

Not many people realize the power of Hooks yet. Yesterday I've convinced someone who was HOC follower to experiment with Hooks and in a matter of hours he changed mind :) https://github.com/apollographql/react-apollo/issues/2539#issuecomment-464500033

Yea bright future is ahead of us :) It's great to be React programmer these days :)

danielkcz commented 5 years ago

For the sake of closure here, there is a recipe about context and inject on the new site.

https://mobx-react.netlify.com/recipes-inject

brentluna commented 5 years ago

@FredyC I noticed in the graphql/hooks/mobx article you wrote, that you never used Context.Provider, in the code. I thought useContext had to link to a provider in its hierarchy. Is this some mobx specific loophole?

danielkcz commented 5 years ago

@brentluna You should probably reread React docs about Context. The Provider is optional. You can simply have a default value in createContext and that will be accessed through useContext.

You might object it's the same as global variable, but it isn't. Generally, using global variables is bad for testing because you would have to ensure proper cleanup between tests. With Context even if you don't use Provider in the app, you can still use Provider in tests which allows you to simply replace context value with a fresh one instead of cleaning the existing one.

One nice little example out of MobX is for mocking Date which is always the pain. Instead of that I have the following piece of code. The app will just use context and for tests, I can override it with fixed value without touching window.Date.

const context = React.createContext(() => new Date())

/**
 * Simple hook providing a function that returns current Date.
 *
 * True power lies in utilizing Context and allowing to override
 * behavior in tests.
 */
export function useNow() {
  return React.useContext(context)
}

/**
 * Overrides useNow() to return specific Date
 */
export const FixedNowProvider: React.FC<{ date: Date }> = ({
  date,
  children,
}) => {
  return React.createElement(context.Provider, { value: () => date }, children)
}
brentluna commented 5 years ago

@FredyC appreciate the response. Huh, wasn't aware of that, guess I should reread the context docs. So you're saying there is no real benefit to using the provider, if you're passing a mobx store as the default value? And then I can still use provider in tests to intercept the use of context with a mocked value on the provider?

danielkcz commented 5 years ago

That's exactly what I am saying :) If you are able to create a store as the default value of context, it certainly has its perks. The Provider is surely useful in-app code too when deeper part of React tree needs a different value (eg. styling, theme...). For MobX stores not so much I reckon as they have a stable reference.

brentluna commented 5 years ago

@FredyC Was wondering if you could clear up best practice for instantiating a store that I don't want to be globally instantiated, for benefits of garbage collecting. I only want the store instantiated if the component is used. I'm trying to grasp which is the correct way that doesn't cause unnecessary recreation of the instance, while it's mounted, but in theory would get GC'd if it unmounts

function Comp() {
  const store = new Store();
  return <Blah.Provider value={store}><child/></Blah.Provider>
}

function Comp() {
  let store;
  useEffect(() => {
    store = new Store()
  }, [])
  return <Blah.Provider value={store}><child/></Blah.Provider>
}
function Comp() {
  const [store] = useState(new Store());
  return <Blah.Provider value={store}><child/></Blah.Provider>
}

function Comp() {
  const store = useRef(new Store());
  return <Blah.Provider value={store}><child/></Blah.Provider>
}
danielkcz commented 5 years ago

Option no. 3 is probably the best AFAIK, but ... useState(() => new Store());. We use the same approach in useLocalStore.

brentluna commented 5 years ago

Option no. 3 is probably the best AFAIK, but ... useState(() => new Store());. We use the same approach in useLocalStore.

I was thinking useRef would have been ideal, as they are almost identical, just I'm forgoing the value referenced causing re-renders. Like either would technically work, but useRef would have possibly been more explicit, that the reference itself shouldn't be causing re-renders. Is there a problem w/ useRef that you are aware of? Thinking maybe

danielkcz commented 5 years ago

@brentluna The useRef does not allow for lazy init, but there is something in Hooks FAQ for that. If you do it as you did, you will be creating (and throwing away) the Store on each render.

brentluna commented 5 years ago

You think useRef value doesn't persist across renders? My understanding is that was the main point of it.

This is from the docs  The only difference between useRef() and creating a {current: ...} object yourself is that useRef will give you the same ref object on every render

danielkcz commented 5 years ago

It does persist, but it also creates and throws away the value you are passing into it in next renders because it's not a function. So on every render the new Store() is called and the instance is not used anywhere.

https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily

brentluna commented 5 years ago

Interesting. So you're saying, the ref or state value won't actually ever be reassigned, however the expressions are still evaluated? Like with either useState(new store()) or the useRef method, the values aren't actually updated but an instance of the store will be made, but never actually assigned so just garbage collected? So the first value always persists on the variable? Thanks for clarifying and discussing with me. Just trying to make sure I am understanding correctly