Skip to content

Commit

Permalink
Move input valueTracker to DOM nodes (facebook#10208)
Browse files Browse the repository at this point in the history 
* Move input valueTracker to DOM nodes

This moves the storage of the input value tracker to the DOM node
instead of the wrapperState. This makes it easier to support both Stack
and Fiber without out a lot of ugly type casting and logic branches.

related: facebook#10207

* run prettier

* remove instance accepting methods

* rm unused trst method

* address feedback

* fix naming

* fix lint
  • Loading branch information
@jquense
jquense authored Jul 19, 2017
1 parent 357925a commit acd9818
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 130 deletions.
8 changes: 4 additions & 4 deletions scripts/fiber/tests-passing.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1748,17 +1748,17 @@ src/renderers/dom/shared/__tests__/findDOMNode-test.js
* findDOMNode should not throw an error when called within a component that is not mounted

src/renderers/dom/shared/__tests__/inputValueTracking-test.js
* should attach tracker to wrapper state
* should define `value` on the instance node
* should define `checked` on the instance node
* should attach tracker to node
* should define `value` on the node instance
* should define `checked` on the node instance
* should initialize with the current value
* should initialize with the current `checked`
* should track value changes
* should tracked`checked` changes
* should update value manually
* should coerce value to a string
* should update value if it changed and return result
* should track value and return true when updating untracked instance
* should return true when updating untracked instance
* should return tracker from node
* should stop tracking
* does not crash for nodes with custom value property
Expand Down
8 changes: 4 additions & 4 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,13 +509,13 @@ var ReactDOMFiberComponent = {
case 'input':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement: any));
inputValueTracking.track((domElement: any));
ReactDOMFiberInput.postMountWrapper(domElement, rawProps);
break;
case 'textarea':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement: any));
inputValueTracking.track((domElement: any));
ReactDOMFiberTextarea.postMountWrapper(domElement, rawProps);
break;
case 'option':
Expand Down Expand Up @@ -967,13 +967,13 @@ var ReactDOMFiberComponent = {
case 'input':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement: any));
inputValueTracking.track((domElement: any));
ReactDOMFiberInput.postMountWrapper(domElement, rawProps);
break;
case 'textarea':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement: any));
inputValueTracking.track((domElement: any));
ReactDOMFiberTextarea.postMountWrapper(domElement, rawProps);
break;
case 'select':
Expand Down
103 changes: 52 additions & 51 deletions src/renderers/dom/shared/__tests__/inputValueTracking-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,139 +17,140 @@ var ReactTestUtils = require('react-dom/test-utils');
// TODO: can we express this test with only public API?
var inputValueTracking = require('inputValueTracking');

var getTracker = inputValueTracking._getTrackerFromNode;

describe('inputValueTracking', () => {
var input, checkbox, mockComponent;
var input;

beforeEach(() => {
input = document.createElement('input');
input.type = 'text';
checkbox = document.createElement('input');
checkbox.type = 'checkbox';
mockComponent = {_hostNode: input, _wrapperState: {}};
});

it('should attach tracker to wrapper state', () => {
inputValueTracking.track(mockComponent);
it('should attach tracker to node', () => {
var node = ReactTestUtils.renderIntoDocument(<input type="text" />);

expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
true,
);
expect(getTracker(node)).toBeDefined();
});

it('should define `value` on the instance node', () => {
inputValueTracking.track(mockComponent);
it('should define `value` on the node instance', () => {
var node = ReactTestUtils.renderIntoDocument(<input type="text" />);

expect(input.hasOwnProperty('value')).toBe(true);
expect(node.hasOwnProperty('value')).toBe(true);
});

it('should define `checked` on the instance node', () => {
mockComponent._hostNode = checkbox;
inputValueTracking.track(mockComponent);
it('should define `checked` on the node instance', () => {
var node = ReactTestUtils.renderIntoDocument(<input type="checkbox" />);

expect(checkbox.hasOwnProperty('checked')).toBe(true);
expect(node.hasOwnProperty('checked')).toBe(true);
});

it('should initialize with the current value', () => {
input.value = 'foo';

inputValueTracking.track(mockComponent);
inputValueTracking.track(input);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(input);

expect(tracker.getValue()).toEqual('foo');
});

it('should initialize with the current `checked`', () => {
mockComponent._hostNode = checkbox;
const checkbox = document.createElement('input');

checkbox.type = 'checkbox';
checkbox.checked = true;
inputValueTracking.track(mockComponent);
inputValueTracking.track(checkbox);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(checkbox);

expect(tracker.getValue()).toEqual('true');
});

it('should track value changes', () => {
input.value = 'foo';

inputValueTracking.track(mockComponent);
var node = ReactTestUtils.renderIntoDocument(
<input type="text" defaultValue="foo" />,
);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(node);

input.value = 'bar';
node.value = 'bar';
expect(tracker.getValue()).toEqual('bar');
});

it('should tracked`checked` changes', () => {
mockComponent._hostNode = checkbox;
checkbox.checked = true;
inputValueTracking.track(mockComponent);
var node = ReactTestUtils.renderIntoDocument(
<input type="checkbox" defaultChecked={true} />,
);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(node);

checkbox.checked = false;
node.checked = false;
expect(tracker.getValue()).toEqual('false');
});

it('should update value manually', () => {
input.value = 'foo';
inputValueTracking.track(mockComponent);
var node = ReactTestUtils.renderIntoDocument(
<input type="text" defaultValue="foo" />,
);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(node);

tracker.setValue('bar');
expect(tracker.getValue()).toEqual('bar');
});

it('should coerce value to a string', () => {
input.value = 'foo';
inputValueTracking.track(mockComponent);
var node = ReactTestUtils.renderIntoDocument(
<input type="text" defaultValue="foo" />,
);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(node);

tracker.setValue(500);
expect(tracker.getValue()).toEqual('500');
});

it('should update value if it changed and return result', () => {
inputValueTracking.track(mockComponent);
input.value = 'foo';
var node = ReactTestUtils.renderIntoDocument(
<input type="text" defaultValue="foo" />,
);

var tracker = mockComponent._wrapperState.valueTracker;
var tracker = getTracker(node);

expect(inputValueTracking.updateValueIfChanged(mockComponent)).toBe(false);
expect(inputValueTracking.updateValueIfChanged(node)).toBe(false);

tracker.setValue('bar');

expect(inputValueTracking.updateValueIfChanged(mockComponent)).toBe(true);
expect(inputValueTracking.updateValueIfChanged(node)).toBe(true);

expect(tracker.getValue()).toEqual('foo');
});

it('should track value and return true when updating untracked instance', () => {
it('should return true when updating untracked instance', () => {
input.value = 'foo';

expect(inputValueTracking.updateValueIfChanged(mockComponent)).toBe(true);
expect(inputValueTracking.updateValueIfChanged(input)).toBe(true);

var tracker = mockComponent._wrapperState.valueTracker;
expect(tracker.getValue()).toEqual('foo');
expect(getTracker(input)).not.toBeDefined();
});

it('should return tracker from node', () => {
var div = document.createElement('div');
var node = ReactDOM.render(<input type="text" defaultValue="foo" />, div);
var tracker = inputValueTracking._getTrackerFromNode(node);

var tracker = getTracker(node);
expect(tracker.getValue()).toEqual('foo');
});

it('should stop tracking', () => {
inputValueTracking.track(mockComponent);
inputValueTracking.track(input);

expect(mockComponent._wrapperState.valueTracker).not.toEqual(null);
expect(getTracker(input)).not.toEqual(null);

inputValueTracking.stopTracking(mockComponent);
inputValueTracking.stopTracking(input);

expect(mockComponent._wrapperState.valueTracker).toEqual(null);
expect(getTracker(input)).toEqual(null);

expect(input.hasOwnProperty('value')).toBe(false);
});
Expand Down
3 changes: 2 additions & 1 deletion src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ function runEventInBatch(event) {
}

function getInstIfValueChanged(targetInst) {
if (inputValueTracking.updateValueIfChanged(targetInst)) {
const targetNode = ReactDOMComponentTree.getNodeFromInstance(targetInst);
if (inputValueTracking.updateValueIfChanged(targetNode)) {
return targetInst;
}
}
Expand Down
Loading

0 comments on commit acd9818

Please sign in to comment.