Skip to content

RFC: Should update child context #3973

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

RFC: Should update child context #3973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ebc8e26
Add .idea to .gitignore (common project config directory for jetbrain…
chrisui May 22, 2015
a2d7262
Add quick efficient context update example for reference
chrisui May 22, 2015
d6d71b7
First pass at context parent/child relationship (setup on mount) to u…
chrisui May 25, 2015
4db424c
Added Swapper for some brute-testing to context example
chrisui May 25, 2015
2916aef
Set context parent/child relationship only between components with ma…
chrisui May 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ examples/shared/*.js
test/the-files-to-test.generated.js
*.log*
chrome-user-data
.idea
118 changes: 118 additions & 0 deletions examples/context/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context isn't supported yet; I don't think we want to have an example. For now, we will want to be unit-tests only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was just the branch I was working on for testing my stuff visually and so all this code would need cleaning up a lot before actually going in. Seb opened up a pr for discussions sake :)

It's great to see all the discussion which has been going on though! I've been busy and away over the past week so haven't been able to get involved too much. Hopefully will have a read through everything later!

<html>
<head>
<meta http-equiv='Content-type' content='text/html; charset=utf-8'>
<title>Context Example</title>
<link rel="stylesheet" href="../shared/css/base.css" />
</head>
<body>
<h1>Basic Example</h1>
<div id="container">
<p>
To install React, follow the instructions on
<a href="https://github.com/facebook/react/">GitHub</a>.
</p>
<p>
If you can see this, React is not working right.
If you checked out the source from GitHub make sure to run <code>grunt</code>.
</p>
</div>
<h4>Example Details</h4>
<p>This is written in vanilla JavaScript (without JSX) and transformed in the browser.</p>
<p>
Learn more about React at
<a href="https://facebook.github.io/react" target="_blank">facebook.github.io/react</a>.
</p>
<script src="../shared/thirdparty/es5-shim.min.js"></script>
<script src="../shared/thirdparty/es5-sham.min.js"></script>
<script src="../shared/thirdparty/console-polyfill.js"></script>
<script src="../../build/react.js"></script>
<script>
var Elapsed = React.createClass({
contextTypes: {
elapsed: React.PropTypes.number.isRequired
},
shouldComponentUpdate: function(nextProps, nextState, nextContext) {
return true;
},
render: function() {
var elapsed = Math.round(this.context.elapsed / 100);
var seconds = elapsed / 10 + (elapsed % 10 ? '' : '.0' );
var message =
'React has been successfully running (via context!) for ' + seconds + ' seconds.';

return React.createElement('div', null, React.createElement('p', null, message));
}
});

// Swapper is a brute force tester to make sure context data flow is working correctly
// with:
// 1. intermediate components preventing updates
// 2. intermediate components being mounted/unmounted continuously
var Swapper = React.createClass({
contextTypes: {
elapsed: React.PropTypes.number.isRequired
},
shouldComponentUpdate: function(nextProps, nextState, nextContext) {
var elapsed = Math.round(this.context.elapsed / 100);

return elapsed % 10 < 7;
},
render: function() {
var elapsed = Math.round(this.context.elapsed / 100);

return React.createElement(elapsed % 10 < 7 ? 'div' : 'span', null, React.createElement(Elapsed));
}
});

var Container = React.createClass({
render: function() {
this.__renderNum__ = (this.__renderNum__ || 0) + 1;

return React.createElement('div', null, [
React.createElement('p', null, [
React.createElement('span', null, "Middle container has been updated " + this.__renderNum__ + " times."),
React.createElement('br'),
React.createElement('span', null, "Hopefully the above should only ever say 1 if we are efficiently handling context updates! And the counter should be doing it's thing below!")
]),
React.createElement(Swapper)
]);
}
});

var ExampleApplication = React.createClass({
propTypes: {
elapsed: React.PropTypes.number.isRequired
},
shouldComponentUpdate: function() {
return false;
},
childContextTypes: {
elapsed: React.PropTypes.number
},
shouldUpdateChildContext: function(nextProps, nextState, nextContext) {
return true;
},
getChildContext: function() {
return {
elapsed: this.props.elapsed
};
},
render: function() {
return React.createElement(Container);
}
});

var start = new Date().getTime();
var iterations = 0;
setInterval(function() {
var elapsed = new Date().getTime() - start;

React.render(
React.createElement(ExampleApplication, {elapsed: elapsed, iterations: ++iterations}),
document.getElementById('container')
);
}, 50);
</script>
</body>
</html>
114 changes: 99 additions & 15 deletions src/renderers/shared/reconciler/ReactCompositeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ var ReactCompositeComponentMixin = {

// See ReactUpdates and ReactUpdateQueue.
this._pendingCallbacks = null;

// See ReactContext
this._contextChildren = null; // or array
this._contextParent = null;
this._isContextParent = false;
this._isContextChild = false;
},

/**
Expand All @@ -131,6 +137,11 @@ var ReactCompositeComponentMixin = {
this._currentElement
);

// This component can be classed as a "context parent" if it modified
// it's child context
this._isContextParent = !!Component.childContextTypes;
this._isContextChild = !!Component.contextTypes || !!Component.childContextTypes;

// Initialize the public class
var inst = new Component(publicProps, publicContext);

Expand Down Expand Up @@ -158,6 +169,19 @@ var ReactCompositeComponentMixin = {
// Store a reference from the instance back to the internal representation
ReactInstanceMap.set(inst, this);

// Setup context parent/child relationship
var previousContextParent = ReactContext.currentParent;
if (this._isContextChild) {
// set two way ref
ReactContext.parentChild(this, ReactContext.currentParent);
if (this._isContextParent) {
ReactContext.currentParent = this;
}
} else {
// set one way ref
this._isContextParent = ReactContext.currentParent;
}

if (__DEV__) {
// Since plain JS classes are defined without any special initialization
// logic, we can not catch common errors early. Therefore, we have to
Expand Down Expand Up @@ -238,12 +262,16 @@ var ReactCompositeComponentMixin = {
this._currentElement.type // The wrapping type
);

var markup = ReactReconciler.mountComponent(
this._renderedComponent,
rootID,
transaction,
this._processChildContext(context)
);
try {
var markup = ReactReconciler.mountComponent(
this._renderedComponent,
rootID,
transaction,
this._processChildContext(context)
);
} finally {
ReactContext.currentParent = previousContextParent;
}
if (inst.componentDidMount) {
transaction.getReactMountReady().enqueue(inst.componentDidMount, inst);
}
Expand Down Expand Up @@ -290,6 +318,9 @@ var ReactCompositeComponentMixin = {
// leaks a reference to the public instance.
ReactInstanceMap.remove(inst);

// Dispose of any context parent/child relationship
ReactContext.orphanChild(this);

// Some existing components rely on inst.props even after they've been
// destroyed (in event handlers).
// TODO: inst.props = null;
Expand Down Expand Up @@ -497,6 +528,10 @@ var ReactCompositeComponentMixin = {
);
},

receiveContext: function(transaction, nextContext) {
this.receiveComponent(this._currentElement, transaction, nextContext);
},

/**
* If any of `_pendingElement`, `_pendingStateQueue`, or `_pendingForceUpdate`
* is set, update the component.
Expand Down Expand Up @@ -558,9 +593,19 @@ var ReactCompositeComponentMixin = {
var nextContext = inst.context;
var nextProps = inst.props;

// Update context parent/child relationship
var previousContextParent = ReactContext.currentParent;
if (this._isContextParent) {
ReactContext.currentParent = this;
} else {
ReactContext.currentParent = this._contextParent;
}

// TODO: Maybe use some _receivingContext flag?
nextContext = this._processContext(nextUnmaskedContext);

// Distinguish between a props update versus a simple state update
if (prevParentElement !== nextParentElement) {
nextContext = this._processContext(nextUnmaskedContext);
nextProps = this._processProps(nextParentElement.props);

// An update here will schedule an update but immediately set
Expand Down Expand Up @@ -591,14 +636,18 @@ var ReactCompositeComponentMixin = {
if (shouldUpdate) {
this._pendingForceUpdate = false;
// Will set `this.props`, `this.state` and `this.context`.
this._performComponentUpdate(
nextParentElement,
nextProps,
nextState,
nextContext,
transaction,
nextUnmaskedContext
);
try {
this._performComponentUpdate(
nextParentElement,
nextProps,
nextState,
nextContext,
transaction,
nextUnmaskedContext
);
} finally {
ReactContext.currentParent = previousContextParent;
}
} else {
// If it's determined that a component should not update, we still want
// to set props and state but we shortcut the rest of the update.
Expand All @@ -607,6 +656,41 @@ var ReactCompositeComponentMixin = {
inst.props = nextProps;
inst.state = nextState;
inst.context = nextContext;

// We will also, potentially, want to update components down the tree
// if the context has updated
var shouldUpdateChildContext = !inst.shouldUpdateChildContext ||
inst.shouldUpdateChildContext(nextProps, nextState, nextContext);

if (shouldUpdateChildContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developers will be tempted to just always return true for shouldUpdateChildContext, since it's the simplest thing to do. At the very least, we should warn if they return true but the object that getChildContext() returns is shallow equal to the previous context they returned. We might even want to make shouldUpdateChildContext mandatory for any context provider. Thoughts @sebmarkbage?

this._performContextUpdate(
nextUnmaskedContext,
transaction
)
}
}
},

/**
* Notifies delegate methods of update and performs update.
*
* @param {?object} nextUnmaskedContext unmasked context
* @param {ReactReconcileTransaction} transaction
* @private
*/
_performContextUpdate: function(
nextUnmaskedContext,
transaction
) {
var childContext = this._processChildContext(nextUnmaskedContext);
this._updateContextChildren(transaction, childContext);
},

_updateContextChildren: function(transaction, childContext) {
if (this._contextChildren) {
this._contextChildren.forEach(function(child) {
child.receiveContext(transaction, childContext);
});
}
},

Expand Down
Loading