How to implement shouldComponentUpdate with this.context? #2517

Open
gaearon opened this Issue Nov 13, 2014 · 120 comments

Projects

None yet
@gaearon
Member
gaearon commented Nov 13, 2014

I know this.context is not officially there but quite a few libraries rely on it, and it seems like it's getting into shape with #2509.

I'm trying to understand how exactly shouldComponentUpdate is supposed to be implemented with context in mind. I noticed it accepts a third argument (nextContext) and I can extend PureRenderMixin to also check it:

  shouldComponentUpdate: function(nextProps, nextState, nextContext) {
    return !shallowEqual(this.props, nextProps) ||
           !shallowEqual(this.state, nextState) ||
           !shallowEqual(this.context, nextContext); // this will throw without context, read on
  }

Components that don't opt into this.context by not omitting contextTypes will not get this third argument, which is understandable.

However this presents a problem when we have a <Middle /> component in between between <Top /> context owner and <Bottom /> context consumer. If <Middle /> implements a restrictive shouldComponentUpdate, there is no way for <Bottom /> to react to <Top />'s context updates at all:

(fiddle)

var Bottom = React.createClass({
  contextTypes: {
    number: React.PropTypes.number.isRequired
  },

  render: function () {
    return <h1>{this.context.number}</h1>
  }
});

var Middle = React.createClass({
  shouldComponentUpdate: function (nextProps, nextState, nextContext) {
    return false;
  },

  render: function () {
    return <Bottom />;
  }
});

var Top = React.createClass({
  childContextTypes: {
    number: React.PropTypes.number.isRequired
  },

  getInitialState: function () {
    return { number: 0 };
  },

  getChildContext: function () {
    return { number: this.state.number };
  },

  componentDidMount: function () {
    setInterval(function () {
      this.setState({
        number: this.state.number + 1
      });
    }.bind(this), 1000);
  },

  render: function() {
    return <Middle />;    
  }
});

React.render(<Top />, document.body);

The same problem would occur if I tried to give Middle a generic context-aware shouldComponentUpdate as I wrote above, because Middle has no this.context unless it opts in.

This is possible to work around by adding contextTypes to Middle, but it doesn't look like a good solution. You'd need to explicitly add necessary contextTypes on every level with smart shouldComponentUpdate so it's too easy to slip up.

Will this be solved by #2112? Is there another solution in the meantime? What is the recommended way?

@jimfb
Contributor
jimfb commented Nov 14, 2014

This is an extremely good question. Thanks for raising it!

This is a case where you're manually pruning/optimizing the tree recalculation. It seems to me that you'd need to request access to any context variables that are of relevance to your calculation. We could also consider doing other fancy things like pass in a hash of the full context (or even the full context) if necessary.

@sebmarkbage - thoughts?

@gaearon
Member
gaearon commented Nov 14, 2014

Yeah, the problem is middle components don't know what context some distant child might need. There is no way they can know which contextTypes to declare to even be able to correctly implement shouldComponentUpdate.

@gaearon
Member
gaearon commented Nov 14, 2014

I wonder if context propagation could happen in a separate tree traversal, without being blocked by falsy shouldComponentUpdate in the middle?

Basically, when parent's context changes, it should mark all its descendants that receive this context as dirty, no matter how distant. Ancestor's context change should have the same effect as a state change for descendants who opt into this context—they should receive new context regardless of what their parents said.

@andreypopp
Contributor

@gaearon I think in that case you need to re-render everything and so shouldComponentUpdate won't have an effect of pruning subtrees. Otherwise context will be in inconsistent state with element tree.

@gaearon
Member
gaearon commented Nov 14, 2014

@andreypopp

I think that shouldComponentUpdate on middle components should have no effect on whether their descendants receive the new context, just like it has no effect on whether they receive new state:

http://jsbin.com/geseduneso/2/edit?js,output

(If this was the case, both numbers would be incrementing)

Where is the inconsistency?

@gaearon
Member
gaearon commented Nov 14, 2014

To put it differently, I think context should work exactly the same as if context owner had something like a “store” into which result of its getChildContext() gets written in componentWillUpdate, and all descendant context consumers that declared keys from that context in contextTypes, should receive that context it as if they were subscribed to that “store” and updated their own state.

I don't mean the actual implementation—but I want to show that this model is not any more inconsistent than any Flux app with shouldComponentUpdate in the middle. Context should act like a “sideway storage”, but scoped to a subtree.

@gaearon
Member
gaearon commented Nov 14, 2014

Edit: this won't work because parent may change

I talked to Glenjamin on IRC and he convinced me changing context might be a bad idea per se. You lose ability to reason about why something was updated if some root update implicitly causes different child updates.

But then, the only reasonable solution I see is to completely forbid context changes. That is, make getChildContext() similar to getInitialState(), which only gets called once before component is mounted.

This would make context a hell lot simpler to think about. We can remove third parameter from shouldComponentUpdate since context never updates.

In case when you need updates from context owner (e.g. like react-router wants activeRoutes to be passed top-down for usage in ActiveState mixin cc @mjackson), nothing prevents you from passing { addChangeListener, removeChangeListener, getActiveRoutes } in context. Descendants can now subscribe to changes and put them in state.

Is this a reasonable solution?

@glenjamin
Contributor

I think the key question to answer is:

In what scenarios is passing data through context preferable to passing data through props or triggering an event to make a component call its own setState.

I've been using context for passing object references around quite happily, as I only ever write to those objects, not read. eg. this.context.triggerAction("something")

@jimfb
Contributor
jimfb commented Nov 14, 2014

The use case for context has been for parameters that are applicable across a potentially large subtree, but which you don't want to pass through more generic container nodes. An example might be a color theme, where a large number of nodes might listen to see if the background color is supposed to be white or black; you wouldn't want to pass those around everywhere as parameters.

Making getChildContext() behave more like getInitialState() wouldn't actually solve the problem, because you could always replace a parent node which provides a given context value with another parent node that provides a different context value. For the affected subtree, this is indistinguishable from mutating the context variable's value.

I think we can find a solution that avoids having users wire up change listeners.

I think @andreypopp may be right. Or at the very least, we need to provide a way for shouldComponentUpdate to know if anything in the context has changed, so it can decide to always return true if there was a context variable change.

I'll chat with @sebmarkbage later today and see what he thinks.

@gaearon
Member
gaearon commented Nov 14, 2014

Making getChildContext() behave more like getInitialState() wouldn't actually solve the problem, because you could always replace a parent node which provides a given context value with another parent node that provides a different context value. For the affected subtree, this is indistinguishable from mutating the context variable's value.

Ouch. You're totally right, I haven't considered this.

@gaearon
Member
gaearon commented Nov 14, 2014

@jsfb On a second thought, I don't quite understand your comment. If you replace a parent node, entire child subtree will re-mount anyway, wouldn't it?

@jimfb
Contributor
jimfb commented Nov 14, 2014

Currently, yes, it will re-mount, but that is partly an implementation detail. You can imagine (and we have been discussing the implementations and ramifications of) reparenting subtrees without loosing node state.

@sebmarkbage and I chatted, and came to the following conclusions:

  • shouldComponentUpdate is a complex escape hatch, the implementation of which requires you to have a solid understanding of what is happening under the component. For this reason, it's not unreasonable to assume you know which context variables are being used, since you already need to know which properties are being used and how they're being used.
  • This change probably isn't making the situation much worse than it already was, and it's an improvement over using the old "owner" relationship. Therefore, this issue is still worth discussion, but is probably non-blocking.
  • Contexts are a complex feature/experiment at this point, and we're probably going to need to do more thinking before we officially support/document them. As we learn more, we'll likely continue to iterate on this topic.

@sebmarkbage, let me know if I missed anything!

Good discussions though! Thanks for all the feedback!

@gaearon
Member
gaearon commented Nov 14, 2014

Thank you for taking time to discuss this!

it's not unreasonable to assume you know which context variables are being used, since you already need to know which properties are being used and how they're being used.

If a top-level Feed component implements shouldComponentUpdate by using PureRenderMixin to avoid extra updates, it doesn't mean that Feed knows that somewhere inside it there is a Cell that happens to contain a Link that depends on router's context.

This gets even worse when frameworks (such as most popular React routing framework) use context and you may not even be aware of it. Somewhere, there are apps that don't change active link state because somebody optimized top-level components and had literally no idea they had to declare corresponding contextTypes to even get nextContext in their shouldComponentUpdate.

There is no way components are aware of all their possible descendants. Basically, if I move a context-dependant component anywhere inside a PureRenderMixin-enabled component, it will break but very subtly. Therefore, if you use context, the only way to avoid this subtle breakage of unrelated components is to never implement shouldComponentUpdate which goes against what React is saying about it.

Some frameworks on top of React, such as @swannodette's Om, make PureRenderMixin-ish shouldComponentUpdate default, it's weird to cut them off like that. That means context-breaking shouldComponentUpdate in every component for them.

I agree this is not blocking for your work, but I can't agree that current situation is satisfactory if context is to be used at all. It's not just hard to implement shouldComponentUpdate with context now—it's downright impossible unless we make assumption that each component is always aware of what its children could be.

Popular libraries already rely on context heavily. I understand it's not a supported feature, but in my opinion it either needs to at least be possible to make it work with shouldComponentUpdate, or it should be disabled, and libraries should be forced to not use it, because it is subtly broken.

@sebmarkbage
Member

This has been known from the start with introduced contexts. We need to be able to have undocumented and unsupported features as experimental features to be able to iterate on them to find special cases. For example, if we didn't have contexts we wouldn't have known that they need to changed to be container-based instead of owner-based. Subtle breakage is part of the contract of using undocumented features.

I think what we need to do is by-pass shouldComponentUpdate if there's a new context anywhere in the parent tree. Potentially with a shouldUpdateChildContext or something that determines if the entire subtree needs to reconcile.

@gaearon
Member
gaearon commented Nov 14, 2014

@sebmarkbage

I suppose this could work because context is hardly meant to change often.
In this case, shouldComponentUpdate doesn't need a third parameter, does it?

@sebmarkbage
Member

Correct, it was always kind of useless.

@mjackson
Member

I think what we need to do is by-pass shouldComponentUpdate if there's a new context anywhere in the parent tree.

This makes a lot of sense. Context changes in the parent need to override falsy shouldComponentUpdates in the subtree.

But it begs the question: how do we know when context changes? For state and props we have setState and render/setProps. It seems like we would need to keep a copy of the current context around somewhere and diff it with the result from getChildContext whenever render is called.

@ghost
ghost commented Nov 23, 2014

I'm using contexts as a way of managing store references. I like this because I can explicitly define what stores are required, so

  1. I can mock out sample data in just the stores I need to test a component, avoiding having to instantiate my larger framework, and
  2. pass in simple read-only stores with the known data for server-side rendering, which simplifies the server-side case as the bulk of the application code is not used.

So, looking at an example,

With my implementation, store data is accessed only through getters. My elements do not cache store data. I want a single version of the truth. As a simple non-async example, my render() would typically have something like

var peopleStore = this.context.peopleStore;
var person = peopleStore.get(this.props.personId);
return <div>person.fullName</div>;

Components attach listeners to stores. I haven't fully decided on the granularity. Stores all have an onChange event. However, I haven't decided on two other things,

  1. listeners for individual property changes
  2. if stores should contain stores

In this example, if "people" was "fb users", it's a large complex async store. Should I reuse the store structure for a PersonStore? One for general collection (getFriends, getPerson, etc) but many unique instances of the Person store type for an individual person.

So in my example, my Component requires the People store as a context param. Then it uses the personId prop to identify and subscribe to a specific Person store.

Now, to introduce some dynamic changes. Let's say the currently logged in user logs out and someone else logs in. Ignoring the fact that you'd probably redirect/refresh the page in a real application, how would this element update? Let's also assume that this element is still on the page and isn't just destroyed.

I would expect the application logic to first remove/destroy the existing People store. For that, my component needs to stop listening for updates. For this, I would recommend a ReactClass API. Eg,

onContextChange(prev, new)

The element can then compare the two peopleStore instances. Let's ignore public data and assume the new PeopleStore is null. The element would unsubscribe from the previous Store and trigger a render(). The render would then show some 'user unknown' type message.

When the user logs in as another user, a new Store is created, the element reattaches, and render() does its thing with new data.

Behind the scenes, "this.render()" couldn't be synchronous. For any of my design/example to make any sense at all, the render() calls need to be collected by the framework and batched together.

Unlike props, listening to stores is outside of the core React role of managing the rendering. That's why I don't think a context change should be involved in shouldComponentUpdate(). And despite my example including changing context values (a new store object), I don't think stores are going to be immutable in their high-level nature. I think that a typical flux application design will operate with a subscriber model, callbacks for async, etc. The base store object will usually live for the life of the application.

@gaearon
Member
gaearon commented Feb 26, 2015

There's quite a bit of interest in this. (See references above.)

@slorber
Contributor
slorber commented Feb 27, 2015

Yes @gaearon .

I want to pass localization data in context, and let the user be able to change its language at runtime, letting all the components re-render themselves with the updated translations,currencies, date formattings... As far as I understand, this seems hard to do for now.

See also
yahoo/react-intl#58
#3038

@gpbl
gpbl commented Feb 27, 2015

@slorber I also adopted a custom PureRenderMixin, it clearly could get broken with stricter shouldComponentUpdates in the middle, as @gaearon says (or if the third parameter will disappear). You can always rely on props, however. Let see how this issue will evolve :)

@slorber
Contributor
slorber commented Feb 27, 2015

@gpbl check that:
#3038 (comment)

It seems to me that in case we want to re-render the whole app from top in case of context change, we can unmount and remount the node in the same event loop tick, and it does not produce flickering effects (constantly). This is a perfect workaround for dealing with user language changes.

@cody
Contributor
cody commented Mar 5, 2015

@slorber Calling unmountComponentAtNode makes bad user experience, since it causes the loss of all local state.

I think it is time to declare context an official feature. Not using context is not an option for most people, because the router and ReactIntl uses it.

@jimfb
Contributor
jimfb commented Mar 5, 2015

@cody et al, To be clear: Context is still subject to change. We're still experimenting with it, and still recommend avoiding it until we've decided on a final API. Usage is at your own risk. Is it useful: probably. Is it ready: no.

@slorber
Contributor
slorber commented Mar 6, 2015

@cody this is not a problem to loose local state for me, as I don't have any local state and my whole app manages immutable state purely outside of React :) Check the video I've done for the framework: https://github.com/stample/atom-react

@gaearon
Member
gaearon commented Mar 21, 2015

I think what we need to do is by-pass shouldComponentUpdate if there's a new context anywhere in the parent tree. Potentially with a shouldUpdateChildContext or something that determines if the entire subtree needs to reconcile.

Yes, shouldUpdateChildContext makes it nice and symmetric.

@gaearon
Member
gaearon commented Apr 9, 2015

Any chance for this to go into 0.14?

@sebmarkbage
Member

If you design a nice API and implement it. :)

It should include some way to determine that only the children that is listening to context keys that has actually changed are actually updated. Instead of everything.

It is unlikely that our full time core team alone will have time for it. :(

On Apr 9, 2015, at 1:32 PM, Dan Abramov notifications@github.com wrote:

Any chance for this to go into 0.14?


Reply to this email directly or view it on GitHub.

@gaearon
Member
gaearon commented Apr 9, 2015

Sure, I'll take a look!

@alexlande alexlande referenced this issue in FormidableLabs/radium Apr 27, 2015
Closed

Media Queries not working for me #100

@gaearon
Member
gaearon commented Apr 30, 2015

Sorry folks, I still can't find time to start working on this. After the pending React DnD 1.0 release I'll be busy preparing for the conf, so it's very unlikely that I'm able to work on this anytime soon. :-(

@chrisui
chrisui commented May 22, 2015

@gaearon Any plans to look at this soon? I'm interested in jumping in on this as it seems this is one of the final pieces of making context fully usable. (might be something else I'm missing?)

The one thing I was wondering is whether or not the following statement is true:

A component's child context should always be a reduction of it's state props, & context

This ensures we can reliably flow the data within the known lifecycle. This is similar to the statement that "a component's render is always a reduction of state & props".

This is how I am currently envisioning it - would just be good to see if I'm totally off course!:

shouldUpdateChildContext determines whether the sub tree should be updated with new context. This part of the lifecycle is triggered by a change to the component's state props or context.

shouldComponentUpdate remains with 3 parameters so a component in the above mentioned sub tree can decide whether it should actually update anything (note: returning false here does NOT affect context flowing down the sub tree further)

Another thing I am wondering here is if there should be another new lifecycle method componentWillReceiveContext? - Called at a similar time to componentWillReceiveProps is but within the context data flow. Possible here to have both these methods called at the "same time".

@gaearon
Member
gaearon commented May 22, 2015

@Chrisui

Please jump on it! I'm too busy with other stuff at the moment.

This ensures we can reliably flow the data within the known lifecycle.

Also keep in mind there's observe hook that might or might not get implemented in 0.14. Which means there might or might not be derived data/observed/whatever it is called. But it's probably irrelevant to your task for now.

@chrisui
chrisui commented May 25, 2015

Got a very initial draft of this down on my fork and you can see it running at examples/context/index.html

I've gone with setting up a simple "context parent"/"context child" relationship. A "context parent" is any component which might change it's child context (ie. implements childContextTypes) and a "context child" is any component which depends on context or might change it's child context (ie. implements contextTypes or childContextTypes). This relationship is setup when components are mounted currently.

When the component update lifecycle comes into play we have two cases:

  1. If component say's do not update (Ie. shouldComponentUpdate() === false) then we check if we should update child context with shouldUpdateChildContext. If this is true (default) then if this component has any immediate "context children" we transition the child component into the update lifecycle. This continues until we get to the end of the context tree or we come across shouldUpdateChildContext() === false
  2. If a component say's do update then nothing is different. There is no side channeling of context data in this case and we let the normal flow continue until a component may hit case #1 again.

No doubt I've missed some important stuff so if anyone could take a quick look that'd be great. Welcoming comments on the architecture of context as well as actual writing the code to fit in with the React codebase! :)

@chrisui
chrisui commented May 26, 2015

For a quick psuedo-example consider the following:

<Root num={1}>  // Gives context {num: this.props.num}
  <Mid>         // Ignores any context and shouldComponentUpdate() === false
    <Child>     // Requires context {num: propTypes.number}

For the initial render (mounting) everything is as we expect. (Impl. detail: a parent/child relationship is created between Root & Child)

We update Root to have the num prop value of 2

<Root num={2} />

Mid also implements a shouldComponentUpdate() method which returns false because it's render() method doesn't care about context.num and nothing else has changed - so why should it update?

This is where in old cases the Child component would never see the new context as we have bailed out of the update down the tree.

With these changes we now check for an shouldUpdateChildContext() method on Mid which could (we've let the default actions take place in our examples) do a simple equality check to see if any context values have changed. Note: the signature of shouldUpdateChildContext() is currently identical to shouldComponentUpdate().

If shouldUpdateChildContext() is true (it is by default) then we update each of our nearest 'context children' (just Child in the example case) if any relationships were setup earlier. This will put that component into it's update lifecycle (contrary to discussions I saw earlier this will still call shouldComponentUpdate() as usual with the new context as the third parameter as this provides the component with granular control still over when it wants to update).

Hopefully that explains the process well enough!

@jimfb
Contributor
jimfb commented May 26, 2015

@Chrisui Cool, great to see some progress on this! We should also add a bunch of unit tests to sanity check basic functionality. At a minimum:

  • Verify child updates even if parent shouldComponentUpdate() returns false.
  • Verify child updates even if parent shouldUpdateChildContext() returns false.
  • Verify child does not update if component shouldUpdateChildContext() returns false.

Additionally, if these are supported features, we might want to verify:

  • Verify shouldUpdateChildContext() does not get called if a grandparent changes their provided context, but a direct parent overrides that context and does not change.
  • Verify shouldUpdateChildContext() does not get called if the component does not read from the context variable that changed.
  • Verify that if a prop changes and a context variable changes, we don't update/render twice.

cc: @sebmarkbage for feedback on the API design/idea.

@eplawless

@Chrisui This sounds fantastic so far, thank you for doing this work.

@chrisui
chrisui commented May 26, 2015

@jimfb Will definitely start getting some unit tests in - just wanted to have something to play with and confirm how we all think it should work before that!

Regarding the second set of cases these are areas which I've avoided in favour of a simpler design. For example it has been suggested that we watch to see which particular context keys are changed and only update these. I feel this might add too much complexity to the parent/child relationship currently and would have less symmetry with how other data flows through React. For example (I may be very naive and overlooking this) we do not compare changing props internally and leave it up for the user to explicitly define whether two separate sources of data (ie. current and next props) should cause an update or simply be ignored.

It would be good to see some more discussion around this specific detail if people feel strongly that it should be implemented.

As for a simple implementation of this if we did want to go forward with this I guess you'd keep merging the childContextTypes of components as you updated up the tree to compare with contextTypes to find valid update targets. Or maybe we can somehow do a bit of magic on mount and create the parent/child relationship only between the child and the nearest parent with a matching context child key. I have a feeling the latter would make it difficult to manage the update order later though if not impossible.

I will have a quick hack and try to mock this out now!

@chrisui
chrisui commented May 26, 2015

Actually regarding my latter suggestion for the specific context key comparison part of functionality it might not be as tricky maintaining the update order if we just follow the same sorted by mount order batch logic used already!

@jimfb
Contributor
jimfb commented May 26, 2015

@Chrisui @sebmarkbage So my concern (unless I'm forgetting/missing something obvious) is that every time a context-provider re-renders, it will cause all the (potentially hundreds) of child components that depend on the provided context variable to re-render, even if the context variable has not changed. My intuition is that we should only trigger a context re-propagation if we've had some sort of indication that a new value is available (like new value not being triple equals to old value, or some flag/notification).

Actually, this implementation is starting to look like a subscription system. Since context is effectively a scoped global, maybe the correct solution is to require components to sideways subscribe to context variables, thereby allowing us to utilize the same mechanism for context that we are going to use for all other subscriptions to global data. See #3398, #3858, and #3920 for relevant information.

Poking @sebmarkbage. I think my latest thinking is in favor of using our sideways data loading solution to solve this problem, to avoid API surface area. I'm curious to hear your thoughts.

@chrisui
chrisui commented May 26, 2015

@jimfb You're right about having hundreds of components re-rendering but I'm not sure it's any different to now if you trigger some state change and have to implement shouldComponentUpdate to prevent a mass update from occuring.

I agree though that context is just looking like a specific stream of side data. If those proposals can have the cascading effect of context I see no reason to really develop on context as it is now. Let's see what other's think!

Out of interest I added functionality to my draft to only create parent/child relationship between components if they have a matching context/child context key (prevents redundant tree traversal) and preventing an update if context hasn't actually changed would simply be done with the PureRenderMixin as always.

@jimfb
Contributor
jimfb commented May 26, 2015

@Chrisui I think the difference is that a single shouldComponentUpdate in a child component can cut off huge branches of the tree, whereas with context, it's much harder to find/fix all the places that will be re-rendered. Even if you bail out, a child that depends on the context variable will still re-render.

Yeah, I'm curious to see what others think about using the sideways data loading solution.

@glenjamin
Contributor

I think context is mostly orthogonal to sideways data loading

Ideally I'd want some way to pass a handle for my sideways data store to components that isn't a global - this is mostly how I use context.

Using context for DI like this reduces the likelihood that it will change during the course of the application though.

On 26 May 2015, at 22:10, Jim notifications@github.com wrote:

@Chrisui I think the difference is that a single shouldComponentUpdate in a child component can cut off huge branches of the tree, whereas with context, it's much harder to find/fix all the places that will be re-rendered. Even if you bail out, a child that depends on the context variable will still re-render.

Yeah, I'm curious to see what others think about using the sideways data loading solution.


Reply to this email directly or view it on GitHub.

@eplawless

It seems like all of the current advice around using context (beyond "don't") would still apply, where anything that changes frequently is going to incur a fairly high re-render cost. Increasing that cost in the very targeted way proposed wouldn't necessarily change how people think about using it. Plus, if you want to cut off branches of a tree it's still fairly straightforward:

class Mid extends Component {
  shouldComponentUpdate() { return false; }
  shouldUpdateChildContext() { return false; }
  ...
}

I'm happy with the relative simplicity of context as it stands + shouldUpdateChildContext. Hopefully changing the underlying mechanism to be in line with other global subscriptions wouldn't reduce its ease of use too dramatically.

@chrisui
chrisui commented May 28, 2015

@jimfb
As @eplawless has pointed out shouldUpdateChildContext can be used to block huge branches of your tree being updated.

I think the context api (with proposed changes) is simple enough that we can implement now and change the internals to use any side data loading react may support later.

Another suggestion: We could also make shouldUpdateChildContext() default to false to prevent this new behaviour by default and make it opt-in.

For the sake of iteration and getting something out into the wild to test (in the same fashion the context feature has always been - hence undocumented) should we pursue this api proposal for 0.14? (It would be good to hear from the gatekeepers!)

@sebmarkbage
Member

I opened up a PR for you: #3973

It is more likely you'll get quick feedback on PRs since they're more easily reviewable compared to master and get priority over issues.

You know I think that I was originally wrong about shouldUpdateChildContext. Do we really need it? Sorry about the thrash.

It seems like shouldComponentUpdate is a strategy that makes it the responsibility of a child to determine whether something has changed. For example, it may not use all the data that you provided to determine its output. As a parent, you don't know that. Perhaps that is enough.

I also think that we might want limit the parent->child connection to a fixed set of properties, like props. I.e. you wouldn't be able to have multiple different contexts supplied from one source, where any given child uses only parts of the context. There could be a 1:1 mapping. Additionally, I was always hesitant to have context bubble straight through a child that consumes it, while the legit use case is generally a "wormhole" between two components, rather than just broadcasting to everyone (which is an anti-pattern).

I'll think of it some more and comment on the PR with details...

@jimfb
Contributor
jimfb commented May 28, 2015

@sebmarkbage I could go either way on shouldUpdateChildContext. My preference is always to minimize API surface. We could always leave it out of the initial API (context just jumps through and initiates re-renders on any component that reads the context variable). If people need the additional escape hatch, we add it later.

I think the bigger problem, as @mjackson pointed out, is that we don't know when the context variable has changed. Do we really want to re-render every fbt/i18n text node every time the i18n ContextProvider happens to re-render? We haven't yet provided a way of saying "don't worry, nothing has changed, don't bother re-rendering every text element on the page".

I'm a little confused about your last paragraph; can you elaborate? I assume you're not suggesting that the following is an anti-pattern of context: An i18n component 'broadcasts' to all i18n-aware child components the user's preferred language/timezone/formatting/etc.

@eplawless

Commented on the PR, should reproduce here I guess:

@sebmarkbage @Chrisui @jimfb

Re: shouldUpdateChildContext, I think it's a useful addition. By default shouldComponentUpdate returns true, which makes it the responsibility of the child to determine whether something has changed. Implementing shouldComponentUpdate means that you're taking away that responsibility in the case where the parent knows better. The same is true of shouldUpdateChildContext, it only exists to take away responsibility from its children as an optimization.

Re: not broadcasting to the whole subtree, I think there are two valid patterns. Wormholes, as you suggested in #2517, make perfect sense. We use that pattern for grouping items within various subsystems (like focus and voice). Additionally, though, we'd want to use context to pass down i18n information to a whole section of (perhaps the entirety of) our application, and force a re-render of anything that uses it. That kind of broadcast pattern is probably relatively rare compared to the wormhole pattern, but I believe it's still valid.

Right now we're occasionally using mixins for focus and voice groups from the same source. I think there are valid use cases for not forcing a 1:1 mapping, though I understand the desire for the restriction.

@jimfb
Contributor
jimfb commented May 28, 2015

I actually thought the "wormhole" pattern was the antipattern, and broadcast was the expected pattern. Context is only interesting if there are lots of consumers of the variable (ie. if passing the prop explicitly would result in it being added to virtually every component and is therefore a lot of boilerplate), otherwise, probably better to pass it explicitly as a prop. But @sebmarkbage appears to have said the opposite (and he is usually right about this stuff), so now I'm confused and would like to get a clarification from him.

@sebmarkbage @eplawless What is an example of the wormhole pattern that you think is valid? What is the parent providing, how is it being used by the child, why can't it just be a prop on the child, etc.

@sebmarkbage
Member
<Table>
  <Cell />
  <Cell />
  <FancyCell />
</Table>
class FancyCell {
  render() {
    return <SomeWhatFancyCell>Some content</SomeWhatFancyCell>;
  }
}

class SomeWhatFancyCell {
  render() {
    return <Cell>{this.props.children}</Cell>;
  }
}

Take borderWidth and pass it to all cells of a table in the form borderLeft/Top/Right/Bottom.

One neat way is to pass borderWidth to <Table />, split it up and pass through context.

It provides a way for something that might render to a Cell to communicate with its conceptual parent like a Table through a hidden channel. This is already what DOM elements do with each other and likely required for doing layout in React.

@sebmarkbage
Member

Don't overfocus on the concept of a "context". The current concept isn't ideal. Maybe the ideal is something like two different concepts or some other channel?

@sebmarkbage
Member

Plausible alternative API:

class Table {
  render() {
    var w = this.props.borderWidth;
    var borderStyle = { left: w, right: w, top: w, bottom: w };
    return <context key={someSymbol} value={borderStyle}>{this.props.children}</context>
  }
}
class Cell {
  static contextKey = someSymbol;
  render() {
    var borderStyle = this.context;
    ...
  }
}

Just spitballing here.

@jimfb
Contributor
jimfb commented May 28, 2015

Continuing the spitballing...

Could use an alternative syntax...

<Table borderStyleChannelKey="myKey">
  <Cell borderStyle={myKey} />
  <Cell borderStyle={myKey} />
  <FancyCell borderStyle={myKey} />
</Table>

Now you've made the communication channel explicit, avoided all possibility of name conflicts, and etc. (reactjs/react-future#28)

@sebmarkbage
Member

A symbol doesn't have name conflicts neither, but you've made the communication channel at a cost. You've passed it through multiple levels of indirections. Anytime you need to change or add another channel, such as background color, you need to go update everything even though the documented contract (Cell within a table) is still the same.

Btw, the essence of React already IS a hidden communication channel: state. It doesn't explicitly pass through each child.

@jimfb
Contributor
jimfb commented May 28, 2015

Call it 'styleinfo' or 'celldata' instead of 'borderStyle', and make it an object. Then adding fields/info doesn't require changing the API contract between Table and Cell.

The only difference between my variant and your variant is that (in my variant) the only way for the child to actually "read" the value is to have it passed explicitly as a prop from the parent. Functionally, your latest proposal is literally identical to my proposal from reactjs/react-future#28.

To be clear, I really like your latest proposal... so I'm not going to complain... but it does solve a subtly different problem. That's what you told me back in March, and I ultimately came to the conclusion you were right (for instance, the broadcast problem). If we're ok with not solving the broadcast problem (which was how I was convinced that context did provide some value), then by all means, let's do something like this!

@jimfb
Contributor
jimfb commented May 28, 2015

An arbitrary symbol does have name conflicts, unless the key name is always decided by the owner, in which case you might as well make it visible to the owner because the owner needs to communicate the key name to the child anyway (otherwise, how would the child know where to look?). By forcing the owner to be involved, you naturally encourage components to use symbols instead of hard coded keys.

@sebmarkbage
Member

I mean a global Symbol (capital S) that have to be orchestrated through some channel such as a common module. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol

The nice part about my new proposed API is that the key is not an "object property" key which gets confused with control flow and program analysis. In my proposal is a first class dynamic value just like the key that powers state.

@jimfb
Contributor
jimfb commented May 28, 2015

Oh, ic, @sebmarkbage is pulling out fancy new javascript again :). I should have known; he is at a TC39 meeting today.

Ok, you're right. That would be very effective in avoiding name conflicts, and would solve the broadcast problem. You've sold me. I like it!

I wish you were in the office today! I feel like this would have been a fantastically fascinating conversation to have in person!

@gaearon
Member
gaearon commented May 28, 2015

Personally I'm glad he wasn't, as this keeps us all in the loop too ;-).

I really like the Symbol thing. The over-reliance of the context on the string keys across a potentially alien tree always had some bad vibe.

@gaearon
Member
gaearon commented May 28, 2015

For example, if two libraries depend on a parent-child wormhole in a third library, and happen to use independent copies of it, those copies wouldn't clash or see each other. Sure it's a weird scenario to imagine, but it seems more consistent.

@sebmarkbage
Member

To be fair, Symbols are already possible to work on the current objects. Even if you use Symbols, you probably end up requiring a module through a potentially global namespace such as an npm top level module name (although hopefully it is relative).

It is just that the current API makes it very natural to use a simple string name. It also conflates runtime semantics with specific property identifiers, makes things like closure compiler advanced mode more difficult to reason about. Also screws with VM optimizations.

@jimfb
Contributor
jimfb commented May 29, 2015

@sebmarkbage You could use UUIDs (aka Symbols), yes, but your new proposal does allow the key to be decided at runtime, meaning the parent COULD influence it via props (not sure what you think of that though). Having said that, I'm not sure how useful that is without making the values available within the jsx scope, I'll need to think on that more, but it does add some cool flexibility (maybe).

@glenjamin
Contributor

I'm not so sure about the "wormhole" analogy, the broadcast scenario and the i18n example are the ones where i've run into the problem the proposed PR aims to solve.

I'm also not entirely clear how #2517 (comment) helps - this seems semantically equivalent to getChildContext and contextTypes to me.

What I do like about the shouldUpdateChildContext PR is that it doesn't require enumerating all of the children at any depth to propagate context changes.

AIUI The original problem was always that although shouldComponentUpdate is passed context, the nature of context means that components in between the "broadcaster" and "receiver" could return false because it didn't know context was passing it.

It seems like the two ways to get around this issue are to track which components are reading from a context above them in the tree, or to have an additional mechanism for propagating context changes down the tree.

@jquense
Contributor
jquense commented Jun 7, 2015

I'd like to offer my 2 cents on the subject after using parent based context in our form components for a bit. The lack of a good update mechanism between a parent and children depending on context as led to creating a parallel, flux like mechanism where the form (functioning as a 'store' instance) passes a listen() method down via context, to which Field Components register with on mount. The form will trigger a change which propagates down. It's all very flux-y and works ok but it destroys the composability model of React Components.

The general strategy of wrapping a component in a HoC doesn't work here as it places the new, wrapping, component outside the update channel instead of in the middle of it (as with props). The great benefit to the prop system is that the flow top down and can be intercepted by anything stuck in between. In the case here of using a flux like update system, the wrapping component would also need to listen to the Form, but then you run into an issue where both the wrapping component and the original (wrapee) component are listening to the Form, instead of the HoC "taking over" and passing the data down to the original component.

Context clearly needs to be "protected" somewhat from components that have no business with the particular context but it does need to be simple to jump into the data stream if a component wants to. In the specific case of our form components, most children of theForm should be isolated from the context it passes down, and only the Field component should receive it. However, if a consumer wants to wrap Field in a HoC (to set defaults or adjust behavior) there should be some straightforward way to do that.

@jquense
Contributor
jquense commented Jun 7, 2015

which is all to say that any context API really ought to allow intermediate (authorized) components to act as reducers for said context, in the same way components can reduce/map props as they pass them down to children. the difference being that whereas props are a "public" steam, context is a private, opt in, stream

@Morhaus
Contributor
Morhaus commented Jun 7, 2015

I've played a bit with the idea of a subscription based context over at react-side-context.

Under the hood, context broadcasters expose a { subscribe(key, cb), getValue(key) } interface to their children. A subscription to a context key bubbles up the context tree until it reaches either a broadcaster of that particular key or the top of the tree. Broadcasters emit changes to context keys via broadcast({ [key]: newValue, ...otherKeys }).

@broadcasts([...keys]) and @observes([...keys]) class decorators tie this API to React components, using context to propagate the context tree through the component tree and setState to enqueue an update in response to a context change.

With a bit of boilerplate, you can setup an API equivalent to the current shouldUpdateChildContext proposal.

To avoid name clashes, context trees are unique, so you'll need a reference to a context in order to broadcast or observe keys within it.

@jedwards1211
Contributor

I've never used context for passing down any dynamic values, only unchanging references. I see some toy examples of that here, but has anyone used it for passing down dynamic values in production apps? I often find real-world use cases more enlightening.

For instance for I18n, is anyone passing down the key-value pairs directly? Or are you just passing down an unchanging reference to an object components can get values from and observe?

@jedwards1211
Contributor

I was interested at one time in implementing some sort of bubble-up event dispatching in context, I forget what exactly for, but the idea was to allow parents to intercept and add more context to events coming from children so that children wouldn't have to have any knowledge about where they fit in the global context.

@taion
taion commented Oct 12, 2015

To add my 2 cents here, I would strongly prefer if any fix here were implemented in such a way as to not break StaticContainer.

I think it would make the most sense StaticContainer continued to work as it did, and blocked all updates coming down from parents, both props and context.

@taion
taion commented Oct 12, 2015

IMO the best way to handle this would be to allow shouldComponentUpdate to potentially detect changes to context used by children, rather than ignoring shouldComponentUpdate entirely whenever context is involved.

@skiano skiano referenced this issue in dowjones/react-cellblock Oct 19, 2015
Closed

shouldComponentUpdate #2

@jimfb
Contributor
jimfb commented Jan 7, 2016

@taion I'll go talk with the Relay folks and figure out what's going on there. Until we have a definitive answer on that, I wouldn't recommend applying that technique in your own code.

cc @sebmarkbage

@taion
taion commented Jan 7, 2016

@jimfb

Probably easier for you to do that (:

My understanding of the idea there is that, when triggering new data to be loaded, by default we want to keep rendering the old data until the new data are ready.

By returning false from shouldComponentUpdate, the Relay root container/renderer has an easy way to just keeping rendering whatever was there before.

It's not blocking interactions per se – it's blocking upstream data fetching updates while in a transient state.

@jimfb
Contributor
jimfb commented Jan 7, 2016

@taion Yeah, I understand what it's doing, but I'm skeptical that it's a pattern we want to support. I'll talk with Sebastian and the relay people, but my guess is that the answer is going to be "yeah, that was a hack, don't do that".

Normally, the way you accomplish that behavior, is to put the old values in to state (until the new values show up / arrive) and render the old values from state.

@taion
taion commented Jan 7, 2016

@jimfb

That pattern is only possible with immutable data or something similar. If you have (non-clonable) stateful objects, you may not be able to follow this pattern. I suspect Relay may not have this issue, but the current implementation of React Router does have this problem.

For general async data libraries, this seems like a reasonably general and convenient pattern.

@jimfb
Contributor
jimfb commented Jan 7, 2016

@taion The approach you're considering seems even more error prone to me ESPECIALLY in the case where you have mutable data. If your data is mutable, then you're going to have race conditions where children sometimes randomly update (due to an event handler, a context change, a force update, etc), and your code becomes very unpredictable. Let me sync with some people and figure this out. Ping me in a couple days if I haven't posted the outcome of that discussion.

@dantman
Contributor
dantman commented Jan 7, 2016

I don't think the StaticContainer use case is something that should be dealt with through that pattern. It sounds like an unstable way to try and get an effect from the side effects of another feature. It would be much better to have a explicit support for what is actually being requested.

In this case it seems the request would be that some low-level React libraries want a way to intercept unmount, divorce the DOM resulting from a component from the React tree, cutting it's connection off and deferring the unmount lifecycle until it's finished.

I think this is a low-level feature we should support with an explicit interface. Some way of unmounting or intercepting unmount of a component, which breaks the link, allows the caller to mess with the underlying context for awhile, and returns a function that will resume the unmount livecycle flow when called.

We may need some special form of fragment or placeholder this API can use to explicitly tell React that a DOM node disconnected from React should be kept in a specific location.

That can be don't without a super special internal node. But then again the idea of a special low-level type of react node that renders as a noscript (the typical react hack for null returns, etc...) and declares that its nextSibling is a DOM node that should be kept in that location but otherwise ignored by React would be a really interesting node type that could be useful in fixing a variety of other bugs.

@jimfb
Contributor
jimfb commented Jan 7, 2016

@taion Ok, I just talked with one person on the Relay team, and a couple of other React people. We are all in agreement that this is not a good pattern. Please don't do it. Using state to store your data while waiting for an update is the official solution, until we come up with a better api/recommendation for that use case.

@taion
taion commented Jan 7, 2016

I can deal with that – I think we've cleaned up a few things on the React Router side such that we can move away from using this pattern anyway.

Thanks!

@ghost
ghost commented Jan 22, 2016

I also want to add one more question. I have something like this.

var BlogPosts = React.createClass({
  getChildContext: function() {
    return {
      currentBlogPost: this.props.currentBlogPost,
      currentUser: this.props.currentUser
    };
  },

  childContextTypes: {
    currentBlogPost: React.PropTypes.object,
    currentUser: React.PropTypes.object
  },

  render: function() {
    return <BlogPosts blogPosts={this.props.blogPosts}/>
  }
});

function select(state) {
  const { blogPosts, currentUser, currentBlogId } = state;
  console.log( state.blogs[currentBlogId]); 
  // first time the above is undefined and then blogs get populated and I have the object;
  return { blogPosts, currentUser, currentBlogPost: state.blogs[currentBlogId] };
};

export default connect(select)(BlogPosts);

now in the BlogPosts component there is BlogPostText , BlogPostImage, PodCast ...depending on whether blogPosts[index].type is text, image, or audio.

in one of the components I have checked for the owner like this,

var BlogPostText = React.createClass({
  canDeleteMemory: function(post, blog, user) {
    return user && (blog.userId == user.id || post.userId == user.id)
  },
  render: function() {
    let isOwner = this.canDeleteMemory(this.context.currentBlogPost, post, this.context.currentUser);
    return isOwner ? <a>Delete</a> : null;
  }
});

then I always get an error at blog.userId because blog is undefined ... so I change the condition like this let isOwner = this.context.currentBlogPost && this.canDeleteMemory(this.context.currentBlogPost, post, this.context.currentUser);
but then the delete icon never shows... but instead of using the contextType if I wrap the BlogPostText component with redux select and use this.props.currentBlogPost then it works fine..

so change in context doesn't trigger a re-render or something like that ... or am I using it wrong.

@jimfb
Contributor
jimfb commented Jan 22, 2016

@aghosh47 I think a context change does trigger a rerender, but it's possible we missed an edge case. If you could create a simple jsfiddle that demonstrates the issue, that would help us investigate.

Also, please post it to a new issue, as we want to keep github issues on topic, to the extent possible.

@ghost
ghost commented Jan 22, 2016

@jimfb ok, I will recheck my code, see if I missed something and create a new thread if I can't resolve.. I just started with react.. and its actually hard to find quality help. so, thanks for the feedback.. appreciate it.

@jedwards1211
Contributor

@jimfb where is the discussion about reparenting without remounting descendants? I would never have expected such a thing, so I'm concerned that unexpected behavioral changes could result in my app with future versions of React if that gets introduced. So I want to stay informed about that discussion

@jedwards1211
Contributor

Another use case for contexts I have stumbled upon is when a component is beneath multiple transition groups. For example, I would like to be able to focus an input when all of its ancestor transition groups actually appear/enter, rather than initially mount. Registering callbacks for ancestor appeared/entered via methods passed down through context would be the most convenient way to do it.

@jimfb
Contributor
jimfb commented Feb 10, 2016

@jedwards1211 To be honest, I don't remember where it was discussed. It's entirely possible that it was an in-person discussion, but there are many reparenting discussions online and maybe you'll find it there.

@jimfb
Contributor
jimfb commented Feb 10, 2016

Ping @aghosh47

@HaoDing1993

哈哈哈

@ghost
ghost commented Mar 21, 2016

@jimfb well, the issue was resolved ... there was some error in code and the people who wrote the code didn't use catch to resolve any errors from promises, so reducer state wasn't getting updated properly and hence the change wasn't getting reflected.

So, if the props in the parent changes, the contextType in the children do get reflected ... Thanks.

@caub
caub commented Mar 24, 2016

what's really wrong in using window for one object storing those globals?

@fatfisz
fatfisz commented Mar 30, 2016

@cauburtin This doesn't really work on the server-side where all created components share the same global state. Context is global wrt all of the components rendered in a single tree, but local to the tree itself. It doesn't leak to other trees, unlike the global state.

On a side note, this question doesn't have anything to do with the issue, does it? 😉

@caub
caub commented Mar 30, 2016

@fatfisz I don't know well your use cases, you could also have a require('foo') that points to a window property only on client

btw in my app I ended up passing down an object in props of all components, since context seems unclear, and since almost all components use it in my case

thanks

@caub
caub commented Apr 4, 2016

what's wrong doing:

// context.js
module.exports  = { // sorry for using commonjs, since most of you use import/export I guess
  // some shared variables, initialized and used by components
};

then require('./context.js'); in all files that need to access context variables and methods

encapsulation is not respected maybe, but well..

(@gaeron)

@gaearon
Member
gaearon commented Apr 4, 2016

@cauburtin Context is useful because:

  1. It can change and trigger re-rendering unlike module exports
  2. It can be overridden by the parent, which is its most useful feature
  3. It doesn’t have to be a singleton, which is useful for server rendering where you want to isolate data
@caub
caub commented Apr 4, 2016

Agreed for 2) and 3)
For 1) I haven't needed to do that yet, but you could pass a react instance in the context too (I have often heard that it's bad practice, you probably think that too, but it's even mentioned in the docs: Recall that you can also pass entire React components in props if you'd like to (where props could be this singleton context instead in this case). For me, React is the view, events listeners and (a top/root) remote control. And this shared context would be a shared access to the remote control. Sub components can update when their props change, and possibly access to and use that global context. (Not a fan of singletons in this case particularly, I could reuse the 'passing down react instance in props' way, if that's useful, this 'passing down' can be more or less automatized/hidden by a common parent class or composition)

well writing this, I realize that singleton context idea is bad :)

what's confusing is that React docs don't invite much to use context, they are like optional props actually, where you get them when you ask for them?

@ericf ericf referenced this issue in yahoo/react-intl Apr 11, 2016
Closed

Changing locale dynamically #365

@shivangbtech

I am implementing Drawer in android using react native and trying to create different file for drawer menu code and drawer content code. To do this I have created react component in different. I am able to do all work in these file but I need drawer reference to perform some operations of drawer in component files. Here is my code, How can I pass reference of drawer to other component file to use drawer methods like openDrawer().

'use strict';

    var React = require('react-native');
    var { View,
          StyleSheet,
          TouchableHighlight,
          } = React;

    var DrawerLayout = require('react-native-drawer-layout');
    var DrawerScreen = require('./DrawerScreen');
    var DrawerMenu = require('./DrawerMenu');

    var DrawerLayoutExample = React.createClass({

      render: function() {
        var navigationView = (
          <View >
               <DrawerMenu/>
          </View>
        );

        return (
          <DrawerLayout
            onDrawerSlide={(e) => this.setState({drawerSlideOutput: JSON.stringify(e.nativeEvent)})}
            onDrawerStateChanged={(e) => this.setState({drawerStateChangedOutput: JSON.stringify(e)})}
            drawerWidth={200}
            ref={(drawer) => { return this.drawer = drawer  }}
            keyboardDismissMode="on-drag"
            renderNavigationView={() => navigationView}>
            <View style={styles.container}>
 // Here is content component for drawer, need to refer drawer reference
            <DrawerScreen ></DrawerScreen>
            </View>
          </DrawerLayout>
        );
      }
    });

    var styles = StyleSheet.create({
      container: {
        alignItems: 'center',
        justifyContent: 'center',
        flex: 1,
        flexDirection: 'column',
      },
     });

    module.exports = DrawerLayoutExample;

DrawerScreen.js

'use strict';
var React = require('react-native');
var {
  AppRegistry,
  StyleSheet,
  Text,
  View,
  Dimensions,
  Image,
  TouchableHighlight,
  TextInput,
} = React;

var deviceWidth = Dimensions.get('window').width;

var DrawerScreen = React.createClass({


  render: function() {
    return (

        <View style={styles.container}>

          <Text style={styles.welcome}>Content!</Text>

          <TouchableHighlight onPress={() => this.state.openDrawer()}>
            <Text>Open drawer</Text>
          </TouchableHighlight>
          <TextInput style={styles.inputField} />
        </View>
    );
  },

});

var styles = StyleSheet.create({
   container: {
      alignItems: 'center',
      justifyContent: 'center',
      flex: 1,
          flexDirection: 'column',
    },
    inputField: {
      backgroundColor: '#F2F2F2',
      height: 40,
    },
});
@wmertens
Contributor

Right now I am using redux to subscribe to and store mediaqueries, so components can decide to render differently on e.g. a phone.

This means that otherwise pure components need a subscription to the store, and that kind of smells wrong to me, not to mention the subcriber model of Redux resulting in a whole lot of subscriptions for something that rarely changes.

I think that in this case context is a much better place to store it, but this issue prevents me from using that.

@caub
caub commented Apr 28, 2016

@wmertens this is something that just need to be done on componentDidMount right?

@wmertens
Contributor

@cauburtin not at all, at any point the user can choose a different language, and at any point the user can resize the browser…

@caub
caub commented Apr 29, 2016

for resize, you could listen in componentDidMount, for languages I would re-render everything

@wmertens
Contributor

I prefer not repeating all resize logic when I have something cool like a
redux actioncreator that listens for the whole apo and works server side
too…
I have state.responsive.isPhone/isPrerender/screenwidth etc. On the server,
I dispatch based on user agent. Neat.

On Fri, Apr 29, 2016, 4:52 PM Cyril Auburtin notifications@github.com
wrote:

for resize, you could listen in componentDidMount, for languages I would
re-render everything


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2517 (comment)

Wout.
(typed on mobile, excuse terseness)

@wmertens
Contributor

But yeah, a simple key={lang} at the top of the tree should suffice for
languages. Not so nice for sites with animations though.

On Fri, Apr 29, 2016, 6:07 PM Wout Mertens wout.mertens@gmail.com wrote:

I prefer not repeating all resize logic when I have something cool like a
redux actioncreator that listens for the whole apo and works server side
too…
I have state.responsive.isPhone/isPrerender/screenwidth etc. On the
server, I dispatch based on user agent. Neat.

On Fri, Apr 29, 2016, 4:52 PM Cyril Auburtin notifications@github.com
wrote:

for resize, you could listen in componentDidMount, for languages I would
re-render everything


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2517 (comment)

Wout.
(typed on mobile, excuse terseness)

Wout.
(typed on mobile, excuse terseness)

@caub
caub commented Apr 29, 2016

I was like you, but resize event listeners are actually cheap, I don't think it's bad to have many of same type listeners

@wmertens
Contributor

Nono, I don't doubt that they are cheap, it is just that it is much easier
to write this.context.isPhone than managing the resize handlers.

On Fri, Apr 29, 2016 at 6:38 PM Cyril Auburtin notifications@github.com
wrote:

I was like you, but resize event listeners are actually cheap, I don't
think it's bad to have many of same type listeners


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2517 (comment)

Wout.
(typed on mobile, excuse terseness)

@ryanbrainard ryanbrainard referenced this issue in heroku/react-refetch May 27, 2016
@nfcampos nfcampos added pure option
- only re-render component if props or requests shallowly changed
- this.setAtomicState now creates new objects for all 4 keys
- added tests for `pure: true` and `pure: false`
- added `pure` option to api docs
7a88aea
@markboyall

The problem is that react-redux returns an === ReactElement to signal "no update" from the render method. But actually this shortcut is only hit if the contexts are === as well, which can't be controlled from the component. I'm not wholly sure why this is done this way, though. Perhaps a question for the react-redux guys.

@taion
taion commented Jun 6, 2016

For working with Redux, see ReactTraining/react-router#470 and continued discussion in PRs.

At some point we'll actually resolve ReactTraining/react-router#3484 to make this easy for other libraries to just use, but it's harder than we originally thought.

@jquense
Contributor
jquense commented Jun 6, 2016

Note about the solution currently used by react-router. It works but is a a frustrating hack. It requires we mutate context in-place or require that users maintain referential identity between their context objects.

Neither is great, but the real problem is that wormholing context around the component hierarchy breaks one of the nicest bits of the React component model for data flow, which is that anything in the middle (with authority) can adjust the context as it goes through. By passing context through "out of band" components would need to use a different mechanism for mapping context to children, or just not do it. Admittedly its not a common use case, but that describes all of context usage anyway

@taion
taion commented Jun 6, 2016

Between the two approaches it's at least somewhat generic though... best we can do for now I think 😛

@jquense
Contributor
jquense commented Jun 6, 2016

yes inded :P, just noting that its a still suboptimal and not a good replacement for fixing the way context works in general

@DarylCantrell
DarylCantrell commented Jun 8, 2016 edited

@jquense Couldn't the component in the middle just declare contextTypes to listen for the context object from above, and childContextTypes / getChildContext() to provide a modified copy of that context to its descendents?

@1000hz
1000hz commented Jun 8, 2016 edited

@DarylCantrell Components need to opt-in to each piece of context they request. There's no way to declare contextTypes to pass the whole context wholesale to a component.

@jquense
Contributor
jquense commented Jun 8, 2016

n any case my point is that adjusting bits of context is possible and a great feature of context, which is broken by workarounds to this issue

@DarylCantrell
DarylCantrell commented Jun 8, 2016 edited

@1000hz Sure, but he was talking about "adjusting parts of the context as it goes through". In that scenario you don't need to get the "whole" context, just whichever part you're planning to override.

@1000hz
1000hz commented Jun 8, 2016

@DarylCantrell Whoops, I misread your intent.

@bvella
bvella commented Jul 7, 2016 edited

I have submitted a pull request for this issue: #7213

(from pull request)
I am using context to propagate locale and routing information. My problem is that some pure components stop subtree rendering when the context changes (because pure components only check state and props). This is a problem for many people as per issue #2517.

I reasoned that not masking the context for intermediate components would be better than masking, and the defined contextTypes would only be used for validating, not for filtering. This is similar to how propTypes work, where the properties specified are validated but the ones not specified are still available to the component. i have updated the tests to reflect this update.

Since components that are not interested in context would typically not use the context argument, not masking the context would not cause any existing component to break, unless said components explicitly are checking that the context param is undefined. This would, however, create an opportunity for a refined pure component, say a ContextAwarePureComponent which would compare state, props and context in shouldComponentUpdate, allowing context updated to cause re-renders of pure components.

I am currently running in development with this patch, using radium and some react-bootstrap components, and did not experience any issue

@DarylCantrell

Hello @bvella, I think you meant this pull request: #7213, not 7212.

@bvella
bvella commented Jul 7, 2016

@DarylCantrell you're right, thanks

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