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 to parent-based context. Fixes #2112. #3615

Merged
merged 1 commit into from
Apr 9, 2015

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Apr 7, 2015

Switch to parent-based context. Fixes #2112.

@sophiebits
Copy link
Collaborator

Can we add a new test that would have failed with owner-based context?

@jimfb
Copy link
Contributor Author

jimfb commented Apr 7, 2015

Yep, good idea, done.

@mridgway
Copy link
Contributor

mridgway commented Apr 7, 2015

Seems like ReactElement._context and ReactContext should be removed since they are no longer used with parent contexts.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 7, 2015

@mridgway That's a super easy change, but I'd like to separate functional changes from cleanup changes (which don't affect functionality) to make code reviews easier. That way, someone has a chance of actually understanding what's going on in any given commit (removing _context will touch a bunch of lines, making it harder to read the PR). If @spicyj would like it to be part of the same commit, I'll go ahead and amend.

@sophiebits
Copy link
Collaborator

Having all the cleanup in one commit is preferable to me.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 7, 2015

Ok, easy money, done.

@mridgway
Copy link
Contributor

mridgway commented Apr 7, 2015

If you do the cleanup in a separate commit and revert this change then the reverted changes won't function correctly unless you revert the cleanup as well. Seems like it creates unnecessary dependencies between commits.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 7, 2015

Cherry-picking reverts can be exceedingly difficult anyway, because people inevitably touch the same code (especially when you have huge commits, you're likely to overlap/conflict somewhere, and merging commits exacerbates the problem). So it's a loose-loose situation.

Regardless, it's a style difference, I don't have a strong preference either way, already fixed it as per @spicyj.

@sebmarkbage
Copy link
Collaborator

I kind of prefer functional changes in a separate commit too, although since PRs allow multiple commits they could be merged.

Accepted, but DON'T land until we've solved the layers issue since that will get this reverted.

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2015

What's the issue with layers? Can I read about it somewhere?

@sebmarkbage
Copy link
Collaborator

Nope, it is an internal FB issue.

@sebmarkbage
Copy link
Collaborator

Basically, there is no way to pass a context to a new subtree - which was also true before but some people hacked around it so we have to undo those hacks.

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2015

Ah, the mystical ReactLayer from @chenglou's gists.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 9, 2015

Layer issue is solved, pending a final review and sync with www.

jimfb added a commit that referenced this pull request Apr 9, 2015
Switch to parent-based context.  Fixes #2112.
@jimfb jimfb merged commit 0185c68 into facebook:master Apr 9, 2015
@sebmarkbage
Copy link
Collaborator

Well ideally we should land that (and prepare the callers) first since we won't be able to sync React internally now.

On Apr 9, 2015, at 1:21 PM, Jim [email protected] wrote:

Layer issue is solved, pending a final review and sync with www.


Reply to this email directly or view it on GitHub.

@skiano
Copy link

skiano commented May 29, 2015

@jimfb

I am excited about this feature, so I built react at 7d44917.

On that commit, I can use parent based contexts (as I see in the unit test that was added)

However, I when I try v0.13.3 I can't seem to get it working.

Was this reverted or changed between then and now?

@sophiebits
Copy link
Collaborator

0.13.3 only includes 0.13 plus individually cherry-picked commits; 0.14 will include everything that's currently in master.

@skiano
Copy link

skiano commented May 29, 2015

Thanks for the info,

Sorry if this info is elsewhere, but does anyone have an idea about how soon 0.14 might go out?

@sophiebits
Copy link
Collaborator

After the things in #3220 get done.

@mjackson
Copy link
Contributor

Just wanted to say THANK YOU to @jimfb and everyone else who worked on this change. This feels like the right way to go.

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.

Switch the Context to use the Parent tree instead of the Owner tree
8 participants