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

Switch the Context to use the Parent tree instead of the Owner tree #2112

Closed
sebmarkbage opened this issue Aug 29, 2014 · 36 comments · Fixed by #3615
Closed

Switch the Context to use the Parent tree instead of the Owner tree #2112

sebmarkbage opened this issue Aug 29, 2014 · 36 comments · Fixed by #3615
Assignees

Comments

@sebmarkbage
Copy link
Collaborator

I'm pretty convinced at this point that Contexts are more useful in the Parent tree than the Owner tree. It also opens up new use cases for parent->child communication that wasn't possible before.

@1st1
Copy link

1st1 commented Sep 30, 2014

I'd be a big +1 for this. This would really make it much easier to design complex components in react.

@syranide
Copy link
Contributor

syranide commented Oct 1, 2014

@sebmarkbage I'm not entirely sure exactly what problem contexts are meant to solve. But in the hypothetical context of reinvented styles (without selectors) I can imagine a benefit in being able to propagate hoveringOverTheBigButton (active/etc) to all descendants... and same for implicitly passing props from far up the hierarchy to far down (as is often requested). This should obviously be in the Parent tree then.

It seems that using a GUID style approach would be preferable for avoiding any and all conflicts, components would instead opt-in. Something like:

var Child = React.createClass({
  contextSources: {
    ancestorIsHovered: Parent...hovered
  },
  render: function() {
    return <div>{this.context.ancestorIsHovered ? 'Ancestor hovered' : 'Ancestor not hovered'}</div>;
  }
});

var Parent = React.createClass({
  childContextTypes: {
    hovered: React.PropTypes.boolean
  },
  getChildContext: function() {
    return {hovered: true};
  },
  render: function() {
    return <Child />;
  }
});

But again, I'm not exactly sure what contexts are meant to solve so I could be way way of the mark here.

@mjackson
Copy link
Contributor

If someone who is more familiar with the React internals would care to put together a short summary of what needs to be done here, I'd be glad to give it a shot and make a PR. react-router depends heavily on context, and it's starting to break for some people's use cases.

Things I don't fully understand:

  • What exactly is the difference between the parent and the owner trees? Is one DOM-based and the other a React thing?
  • How might this change affect users currently using context?

@appsforartists
Copy link

I'd love to know more about when/if this might happen and what it will take to get there. I share @1st1's belief that this will make component/library composition better. In my particular case, I'm working on a library that wraps ReactRouter, and I'd like to be able to pass data down from my library, through the routes, and into the app's components. I can't presently do this without either a) changes by ReactRouter to support this directly, or b) the ability to pass a context into an app's already-created routes.

@sophiebits
Copy link
Collaborator

@sebmarkbage I assume we'll want to support reparenting someday… is there anything we need to keep in mind while doing this? Guessing not but wanted to check.

@mjackson
Copy link
Contributor

Things I learned yesterday while talking to @sebmarkbage about this:

  • The parent tree is made up of parent components, not necessarily DOM elements. The owner tree is made up of components that were on the bottom of the render stack at the time a given component was rendered.
  • Changing context to use the parent tree gets us one step closer to being able to remove _owner entirely, which is a good thing.
  • The vast majority of people currently using context should be unaffected by this change since the owner tree is by definition a subset of the parent tree.

@sebmarkbage
Copy link
Collaborator Author

cc @JSFB

jimfb added a commit to jimfb/react that referenced this issue Nov 7, 2014
jimfb added a commit to jimfb/react that referenced this issue Nov 10, 2014
jimfb added a commit to jimfb/react that referenced this issue Nov 12, 2014
@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2014

+1 for this. So happy this is finally being worked on!

We use context indirectly through react-router, and use it directly for analytics (e.g. in which of several "areas" of the screen / on which screen was "Sign Up" button pressed).

Another use case is when we want some pages to be "themed" with a specific background color and we'd like all controls, no matter how deep in hierarchy, be able to use this color (can't do this in CSS because we want to calculate whether text needs to be dark or light based on that, etc).

@jimfb jimfb self-assigned this Nov 17, 2014
jimfb added a commit that referenced this issue Nov 18, 2014
jimfb added a commit that referenced this issue Nov 18, 2014
Start monitoring uses of withContext, related to issue #2112
@natew
Copy link

natew commented Mar 4, 2015

@jamesknelson is this enabled in RC2 or would it be worth updating and trying the patch there?

@zenlambda
Copy link

Is there any chance that we might get some documentation on how contexts work? I've seen a vague mention of this feature on the React blog. I've seen it in use in other projects such as React-router and perhaps also Reapp. There is code in those projects that throws warnings and needs migrating. I'd love to contribute but I feel unable to help make the situation better because I'm still in the dark about how contexts are supposed to work.

@esamattis
Copy link

@natew
Copy link

natew commented Mar 31, 2015

Just as a heads up to anyone using the contextPatch and running into type.toUpperCase() is undefined errors, this just bug bit me and drove me mad for the last couple hours.

If your child Class is attempting to use an undefined component this happens. In my case I was trying to import "Textarea" rather than "TextArea".

I was certain the error was due to how I was mounting routes (in the parent class) and funny enough both children routes I tried had this same Textarea bug, so I spent forever changing the parents until I got this. 💀

@1st1
Copy link

1st1 commented Apr 6, 2015

Any chance this will land to 0.14?

@sophiebits
Copy link
Collaborator

@1st1 That's the plan.

jimfb added a commit to jimfb/react that referenced this issue Apr 7, 2015
jimfb added a commit to jimfb/react that referenced this issue Apr 7, 2015
jimfb added a commit to jimfb/react that referenced this issue Apr 7, 2015
jimfb added a commit to jimfb/react that referenced this issue Apr 7, 2015
jimfb added a commit that referenced this issue Apr 9, 2015
Switch to parent-based context.  Fixes #2112.
zpao added a commit to zpao/react that referenced this issue Apr 20, 2015
This reverts commit 7d44917.

Only doing this temporarily here to make it easier to sync FB
jimfb added a commit to jimfb/react that referenced this issue Apr 24, 2015
ntucker pushed a commit to ntucker/react that referenced this issue May 28, 2015
Conflicts:
	src/core/__tests__/ReactCompositeComponent-test.js
@danscan
Copy link

danscan commented Jul 20, 2015

Does React Native use parent context now as well? Seems like it still uses owner context.

@sophiebits
Copy link
Collaborator

React Native is still on React 0.13.2. We'll update after the 0.14 final release.

@jedwards1211
Copy link
Contributor

Cool, glad to know this is on the way! I've also used context to pass down stores, out of a desire to avoid singletons. I've used them in a few other cases as well.

timbotnik pushed a commit to meteor/chromatic that referenced this issue Jul 27, 2017
This behavior has long been fixed (facebook/react#2112) and is causing errors when we have “null” children inside of a Form (a common pattern in more modern versions of React)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.