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

Accept calculateChangedBits as a prop on Context.Providers #60

Closed
wants to merge 2 commits into from

Conversation

markerikson
Copy link

Per a suggestion from Dan Abramov: this RFC proposes allowing a Context.Provider instance to accept a calculateChangedBits function as a prop, in addition to the current implementation of accepting them as an argument to React.createContext.

View rendered RFC

@sebmarkbage
Copy link
Collaborator

Thanks for writing this up. It's a well written RFC.

I'd like to add some context about this API though. The calculateChangedBits API is a bit sketchy, likely to change. The context API in general is optimizing for things that rarely change and when it changes, isn't all that high priority. If you care about fine grained updates enough to care about calculate changed bits, maybe you should just use subscriptions instead? Just because there's a new API doesn't mean it's better.

Being in the niche of having to use calculateChangedBits is a very small niche in the first place. Being in the niche of needing specific features out of calculateChangedBits is even less likely to being the best solution to the problem. If everyone needs such features to use it, then maybe we should just rip it out completely. It's already adding an uncomfortable amount of code and execution in hot paths. So that's the lens I'm viewing this RFC through.

A few points:

  1. How does this relate to the global context API? Context.set(newGlobalValue) facebook/react#13293 The idea there is that you shouldn't have to take on a dependency of a provider in most cases. How would your solution work there?

  2. What happens when you change the calculateChangedBits function from one function to another gets a bit weird. We've found that it's often not bad to have two contexts. One "implementation context" which is essentially dependency injection and one "state context" which is the current value. The implementation context can be the specific one you generate per store and never changes. If it does change, the semantics are clear. It clears the tree.

  3. How are you planning to use this in practice? We imagined that this would more likely be used by mutable/change tracking based stores that can generate a list of changed values from that. Then that list can be used to compute the bits. If you have to diff the tree to get the changed values, only to then rerender the React tree again, you're in dangerous territory of just spending more time diffing than it would take to rerender.

@markerikson
Copy link
Author

markerikson commented Aug 25, 2018

In the case of React-Redux, we already use per-component-instance subscriptions up through the current version. The Redux store is currently passed down through legacy context, and every connected component instance is a separate subscriber to the store.

We're currently attempting to transition to using createContext as a core change for a version 6 (per a couple potential reimplementations in PR 995 and PR 1000 ).

Since createContext is capable of correctly propagating updates to deeply nested components (which legacy context could not do), there's no reason to have separate per-component subscriptions any more. This refactor involves passing down the current Redux store state via context and having connected components consume that store state value instead of directly retrieving it themselves. In addition, we hope that this would solve any potential "tearing" issues with async React, as all components would be seeing the same Redux store state during a render tree calculation.

Our current rough performance benchmarks indicate that the createContext approach is a bit slower than the original v5 implementation in an artificial stress test scenario, but still competitive (see related discussion and numbers in React issue 13456 ). However, a very quick proof-of-concept using the current calculateChangedBits API (and relying on the state structure of our benchmark app) was actually significantly faster than v5. So, it's a promising approach that we would like to make use of if possible.

Answering your comments specifically:

Being in the niche of needing specific features out of calculateChangedBits is even less likely to being the best solution to the problem. If everyone needs such features to use it, then maybe we should just rip it out completely. It's already adding an uncomfortable amount of code and execution in hot paths.

In this case, I wouldn't say it's actually adding a "specific feature" around calculateChangedBits - it's just exposing the same ability in a more flexible way, and there's effectively no additional code being executed.

  1. How does this relate to the global context API? Context.set(newGlobalValue) facebook/react#13293 ?

Assuming I'm reading that correctly, 13293 would allow an end user to globally update the current value for a given Context instance using a sort of static function, without ever having to render a Context.Provider and "manually" pass down some sort of this.updateTheContextValue method to its descendants.

I don't think this RFC changes anything related to that PR. The change I'm proposing only matters if you are actually rendering a MyContext.Provider. If you're skipping rendering a MyContext.Provider and just calling MyContext.set(), then it would still be able to use whatever calculateChangedBits function was provided when you called createContext.

  1. Changing the provided calculateChangedBits function

Implementation-wise, I think the example reimplementation code I wrote should work if the user either switches from giving a function to not giving one, or vice-versa.

Your example of using separate "implementation context" and "value context" instances sounds like what I described in the "Alternatives" section of the RFC. To me having to use two separate Context instances like that feels really hacky, but I guess it could potentially work.

  1. How are you planning to use this in practice? We imagined that this would more likely be used by mutable/change tracking based stores that can generate a list of changed values from that. Then that list can be used to compute the bits.

That sounds like exactly how I would want to use it :)

In the RFC, I wrote this hand-waved function:

function calculateChangedBitsForPlainObject(currentValue, nextValue) {
    // magic logic to diff objects and generate a bitmask
}

Here's how I imagine an un-hand-waved implementation would work.

Let's say we've got a Redux store whose top-level state looks like {a, b, c, d}. The contents of those state slices is irrelevant - we can assume that the values are being updated immutably, as you're supposed to do with Redux.

A standard shallow equality check loops over the keys for two objects , and for each key, checks to see if the corresponding values in the two objects have changed. If any have changed, it returns false ("these objects are not equal"). I would want to implement a similar function that returns an array of all keys whose values have changed in the newer object (such as ["a", "c"]).

From there, I would run each key name through a hash function of some sort, which hashes the key string into a single bit. I briefly experimented with using a "minimal perfect hashing" library a while back as an early proof of concept.

So, every time the Redux store updated, the React-Redux <Provider> component, as the sole subscriber to the store, would determine which keys had their values changed and generate the changedBits bitmask accordingly. It wouldn't be a "deep" comparison of the state trees, but rather a quick and simple "top level" list of changes, at least as a default implementation.

Connected components would by default still subscribe to all updates, but we would provide a way for the end user to indicate that a connected component only cares about updates to certain state slices (hypothetical example: connect(mapState, null, null, {subscribeToStateSlices: ["a", "d"]})(MyComponent).

@NE-SmallTown
Copy link

NE-SmallTown commented Aug 27, 2018

So, every time the Redux store updated, the React-Redux component, as the sole subscriber to the store, would determine which keys had their values changed and generate the changedBits bitmask accordingly. It wouldn't be a "deep" comparison of the state trees, but rather a quick and simple "top level" list of changes, at least as a default implementation.

I agree with this. I have thought about this(the hash solution, like immutable) before the new context api, to be honest I think this is irrelevant with the context api, even in old context api we could do this. But this is not about react-redux perf, it's just about reducer logic. Just some rough thought, we could provide a new api to allow developer update one deeper object/array, such as allow return a string(or return a object array, or provide a individual api) when use a reducer like foo.bar.baz, it means we only depend on foo.bar.baz to update. We could hash this string to avoid destruct in reducer.

Connected components would by default still subscribe to all updates, but we would provide a way for the end user to indicate that a connected component only cares about updates to certain state slices (hypothetical example: connect(mapState, null, null, {subscribeToStateSlices: ["a", "d"]})(MyComponent).

But I think maybe this is not a good idea, if we do this, we will put the complexity to the developer, this implementation(subscribeToStateSlices) of dependencies collection is terrible(the read is complex, and you have to write it many times in many components), we need think about other ways(maybe put the dependences to reducer like createReducer(func, [myComponentA, myComponentB]), but this is not a good idea too maybe, I don't know), otherwise use something like immer/immutable maybe a good choice.

Finally, IMO, I don't think we could do something like this rfc to perf react-redux. Because redux is unaware of the relationship between component state dependencies and redux store. So, even we add a wrap like react-redux, we also can't get it, no matter how we do in react-redux. Once enter a normal world(like redux) we will lost any specific info no matter how where you from(like react-redux or something else).

@markerikson
Copy link
Author

@NE-SmallTown : I'm not quite sure what point you're trying to make, and it doesn't exactly seem relevant to this RFC.

The primary use case I'm describing in the RFC is indeed for use with React-Redux, largely because A) I do want to use it in React-Redux, and B) that's what I know the most about. I already provided examples of how React-Redux could benefit from this. But, I can see it being useful for other state handling libraries as well.

The subscribeToStateSlices thing was just a hypothetical example of how a connected component might ultimately decide how to set the unstable_observedBits value, but the actual mechanism doesn't matter for now.

The rest of the comments about reducers and stuff aren't on topic for this discussion - if you've got other suggestions, please file an issue on the Redux repo.

@langpavel
Copy link

Ufff 🙂, this is really hard discussion…
Context is global thing and if not, then it should live in state.
Distinguishing carefully of this will save your business I hope 😬
Even in one application you can use different context, but this should be rare;
like preview of component at different landscapes (English, Czech, Norway… Chinese) or bright/dark theme…
———
I really like new createContext API too, but if you want provide it safely for usackers (I can use it, I'm hacker so — really, not funny 😅), you must care about misusing… Unfortunately there are no simple metrics nor linter rules for telling me that something is wrong, seriously…
———
So, like shouldComponentUpdate can take CPU more busy when somebody do a little change, this may have even more impact.

@kafein
Copy link

kafein commented Sep 16, 2018

@sebmarkbage

We've found that it's often not bad to have two contexts. One "implementation context" which is essentially dependency injection and one "state context" which is the current value. The implementation context can be the specific one you generate per store and never changes.

Hi. For a while i'm thinking for similar approach. Is there any example implementation like this? Thanks

@wyqydsyq
Copy link

wyqydsyq commented Dec 6, 2018

Maybe it's just me but I feel like the same use-cases enabled with this change to Context.Provider could be supported without breaking anything if the Context API simply checked for and called a lifecycle function (let's say... shouldApplyContext) on each Consumer with the new and previous contexts, and only continues calling render/other lifecycle hooks with the new context if true is returned? So basically a component-level shouldComponentUpdate specifically for context changes instead of props/state? Or even adding nextContext as a third argument to the existing shouldComponentUpdate lifecycle hook and having that be called before re-rendering due to context updates would suffice.

Wrapping components in nested Context.Consumers becomes incredibly cumbersome with data structures that need to be shared across a wide range of components where each component only cares about a couple of properties.

The alternative of setting contextType and context on your component classes is much cleaner, but restricts you to using a single context per component, which encourages people to use a singleton context that will cause all components consuming it to re-render upon any change regardless of whether the values that changed in the context are actually used by the consuming component.

Consider the following example of the common pattern of using a global state singleton to populate context:

import React, { createContext } from 'react';

const defaultContext = {
  video: {
    stream: undefined,
  },
  userData: {
    id: 1,
    name: 'Bob',
  },
};
const Context = createContext(defaultContext);

export class Main extends React.PureComponent {
  state = {};

  constructor(props: any) {
    super(props);
    this.state = defaultContext;
  }

  async componentDidMount() {
    const stream = await navigator.mediaDevices.getUserMedia({});
    this.setState(
      {
        video: {
          stream,
        },
      },
      async () => {
        // dummy fetch method, imagine it returns a user with ID and name
        const userData = await fetchUserData();
        this.setState({
          userData,
        });
      },
    );
  }

  render() {
    return (
      <Context.Provider value={this.state as any}>
        <HelloWorld />
      </Context.Provider>
    );
  }
}

export class HelloWorld extends React.PureComponent {
  static contextType = Context;
  context!: React.ContextType<typeof Context>;

  shouldApplyContext(prev, next) {
    return prev.userData.name !== next.userData.name;
  }

  render() {
    return (
      <div>
        Hello, {this.context.userData.name} <Video />
      </div>
    );
  }
}

export class Video extends React.PureComponent {
  static contextType = Context;
  context!: React.ContextType<typeof Context>;
  ref: HTMLVideoElement;

  shouldApplyContext(prev, next) {
    return prev.video.stream.id !== next.video.stream.id;
  }

  componentDidMount() {
    this.ref.srcObject = this.context.video.stream;
  }

  render() {
    return <video ref={(el) => (this.ref = el)} />;
  }
}

Currently, both child components above would render multiple times due to the Main component updating context multiple times in response to some async actions. If the Context API checked the result of each component's shouldApplyContext (if present) prior to applying updates, each component would only re-render when the fragment of context it's actually interested in has changed.
Essentially I should be able to prevent the Video component from re-rendering in response to context changes it doesn't care about (like if the user changes their name). Currently it will re-render again after the userData property is updated, even though it does not use or care about that value.

Having a singleton store that gets fractally consumed is a pattern/use-case that is common and trivial in MobX (because MobX observers subscribe to properties they consume instead of changes to the entire store), but does not work well when porting to the Context API.

I haven't looked at how the internals of the Context API work around here, but I imagine it could be implemented by adjusting the code that applies changes to consumers to inspect the class of the given consumer for a shouldApplyContext method and if found, check the result of calling that to determine whether a component should be updated instead of relying purely on changes to observedBits.

My current real-life use-case is I have an app with a singleton context containing 3 properties: session, publisher and view for sharing login, RTC and UI state across components respectively. I have lots of components that depend on values from all 3, so I need to either split them into separate contexts and be littering most of my components with 3 nested <Context.Consumer>s or use a singleton context, either way all of the components will re-render in response to changes to any of the 3 contexts even if they aren't using properties that changed.

@markerikson
Copy link
Author

Closing in favor of #119 .

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

Successfully merging this pull request may close these issues.

7 participants