Skip to content

Move input valueTracker to DOM nodes #10208

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

Merged
jquense merged 7 commits into from
Jul 19, 2017
Merged

Move input valueTracker to DOM nodes #10208

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9d4104
Move input valueTracker to DOM nodes
jquense Jul 18, 2017
c3b0eac
run prettier
jquense Jul 18, 2017
b15092e
remove instance accepting methods
jquense Jul 18, 2017
12317ad
rm unused trst method
jquense Jul 18, 2017
b4f3fd2
address feedback
jquense Jul 18, 2017
f012837
fix naming
jquense Jul 18, 2017
04ac179
fix lint
jquense Jul 18, 2017
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
2 changes: 1 addition & 1 deletion src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ var ReactDOMFiberComponent = {

// We also check that we haven't missed a value update, such as a
// Radio group shifting the checked value to another named radio input.
inputValueTracking.updateValueIfChanged((domElement: any));
inputValueTracking.updateNodeValueIfChanged((domElement: any));
break;
case 'textarea':
ReactDOMFiberTextarea.updateWrapper(domElement, nextRawProps);
Expand Down
84 changes: 43 additions & 41 deletions src/renderers/dom/shared/__tests__/inputValueTracking-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ 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;

Expand All @@ -28,33 +30,30 @@ describe('inputValueTracking', () => {
mockComponent = {_hostNode: input, _wrapperState: {}};
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

Can we remove mockComponent completely?

});

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(node.hasOwnProperty('_valueTracker')).toBe(true);
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

Can also use getTracker here?

});

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);

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

expect(tracker.getValue()).toEqual('foo');
});
Expand All @@ -64,92 +63,95 @@ describe('inputValueTracking', () => {
checkbox.checked = true;
inputValueTracking.track(mockComponent);

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.updateNodeValueIfChanged(node)).toBe(false);

tracker.setValue('bar');

expect(inputValueTracking.updateValueIfChanged(mockComponent)).toBe(true);
expect(inputValueTracking.updateNodeValueIfChanged(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);

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);

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

inputValueTracking.stopTracking(mockComponent);

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

expect(input.hasOwnProperty('value')).toBe(false);
});
Expand Down
110 changes: 50 additions & 60 deletions src/renderers/dom/shared/inputValueTracking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

'use strict';

var {ELEMENT_NODE} = require('HTMLNodeType');
import type {Fiber} from 'ReactFiber';
import type {ReactInstance} from 'ReactInstanceType';

Expand All @@ -21,16 +20,12 @@ type ValueTracker = {
setValue(value: string): void,
stopTracking(): void,
};
type WrapperState = {_wrapperState: {valueTracker: ?ValueTracker}};
type ElementWithWrapperState = Element & WrapperState;
type InstanceWithWrapperState = ReactInstance & WrapperState;
type SubjectWithWrapperState =
| InstanceWithWrapperState
| ElementWithWrapperState;
type WrapperState = {_valueTracker: ?ValueTracker};
type ElementWithValueTracker = HTMLInputElement & WrapperState;

var ReactDOMComponentTree = require('ReactDOMComponentTree');

function isCheckable(elem: any) {
function isCheckable(elem: HTMLInputElement) {
var type = elem.type;
var nodeName = elem.nodeName;
return (
Expand All @@ -40,26 +35,30 @@ function isCheckable(elem: any) {
);
}

function getTracker(inst: any) {
if (typeof inst.tag === 'number') {
inst = inst.stateNode;
}
return inst._wrapperState.valueTracker;
function getTracker(node: ElementWithValueTracker) {
return node._valueTracker;
}

function detachTracker(subject: SubjectWithWrapperState) {
subject._wrapperState.valueTracker = null;
function detachTracker(subject: ElementWithValueTracker) {
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

subject => node?

subject._valueTracker = null;
}

function getValueFromNode(node: any) {
var value;
if (node) {
value = isCheckable(node) ? '' + node.checked : node.value;
function getValueFromNode(node: HTMLInputElement): string {
var value = '';
if (!node) {
return value;
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 19, 2017

Choose a reason for hiding this comment

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

It used to return undefined in this code path but now returns ''. Does this matter?

Copy link
Copy Markdown
Contributor

@aweary aweary Jul 19, 2017

Choose a reason for hiding this comment

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

@gaearon I think it's fine, getValueFromNode is just used to compare next and last values, both of which are returned from getValueFromNode. So the equality check should still hold.

Copy link
Copy Markdown
Contributor Author

@jquense jquense Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

yeah the value is only ever used to compare to itself...and actually node can't be falsely here, since it's only called below after a if (node) check

}

if (isCheckable(node)) {
value = node.checked ? 'true' : 'false';
Copy link
Copy Markdown
Contributor

@aweary aweary Jul 18, 2017

Choose a reason for hiding this comment

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

Is this just a stylistic change or does this affect the semantics at all?

Copy link
Copy Markdown
Contributor Author

@jquense jquense Jul 18, 2017

Choose a reason for hiding this comment

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

was a fight between eslint and flow. flow wants String(bool) and eslint prohibits primitive constructors

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

What about '' + bool?

Copy link
Copy Markdown
Contributor Author

@jquense jquense Jul 18, 2017

Choose a reason for hiding this comment

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

flow complains about an invalid coercion, its being a bit over protective

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

I see. I don't mind it being explicit.

} else {
value = node.value;
}

return value;
}

function trackValueOnNode(node: any, inst: any): ?ValueTracker {
function trackValueOnNode(node: any): ?ValueTracker {
var valueField = isCheckable(node) ? 'checked' : 'value';
var descriptor = Object.getOwnPropertyDescriptor(
node.constructor.prototype,
Expand Down Expand Up @@ -100,7 +99,7 @@ function trackValueOnNode(node: any, inst: any): ?ValueTracker {
currentValue = '' + value;
},
stopTracking() {
detachTracker(inst);
detachTracker(node);
delete node[valueField];
},
};
Expand All @@ -109,70 +108,61 @@ function trackValueOnNode(node: any, inst: any): ?ValueTracker {

var inputValueTracking = {
// exposed for testing
_getTrackerFromNode(node: ElementWithWrapperState) {
return getTracker(ReactDOMComponentTree.getInstanceFromNode(node));
_getTrackerFromNode(inst: ElementWithValueTracker) {
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

This seems like it should be node. We can also just pass it directly: _getTrackerFromNode: getTracker

return getTracker(inst);
},

trackNode(node: ElementWithWrapperState) {
_getTrackerFromInstance(inst: ReactInstance | Fiber) {
return getTracker(ReactDOMComponentTree.getNodeFromInstance(inst));
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

Can we not expose it? Since only tests use it and they already have access to DOM node.

Copy link
Copy Markdown
Contributor Author

@jquense jquense Jul 18, 2017

Choose a reason for hiding this comment

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

sure 👍

},

trackNode(node: ElementWithValueTracker) {
if (getTracker(node)) {
return;
}
node._wrapperState.valueTracker = trackValueOnNode(node, node);

// TODO: Once it's just Fiber we can move this to node._wrapperState
node._valueTracker = trackValueOnNode(node);
},

track(inst: InstanceWithWrapperState) {
if (getTracker(inst)) {
return;
}
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
track(inst: ReactInstance | Fiber) {
inputValueTracking.trackNode(
ReactDOMComponentTree.getNodeFromInstance(inst),
);
},

// TODO: in practice "subject" can currently be Stack instance,
// Fiber, or DOM node. This is hard to understand. We should either
// make it accept only DOM nodes, or only Fiber/Stack instances.
updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
if (!subject) {
updateNodeValueIfChanged(node: ElementWithValueTracker) {
if (!node) {
return false;
}

var isNode = (subject: any).nodeType === ELEMENT_NODE;
var tracker = getTracker(subject);
var tracker = getTracker(node);
// if there is no tracker at this point it's unlikely
// that trying again will succeed
if (!tracker) {
if (isNode) {
// DOM node
inputValueTracking.trackNode((subject: any));
} else if (typeof (subject: any).tag === 'number') {
// Fiber
inputValueTracking.trackNode((subject: any).stateNode);
} else {
// Stack
inputValueTracking.track((subject: any));
}
return true;
}

var node;
if (isNode) {
// DOM node
node = subject;
} else {
// Fiber and Stack
node = ReactDOMComponentTree.getNodeFromInstance(subject);
}

var lastValue = tracker.getValue();
var nextValue = getValueFromNode(node);
if (nextValue !== lastValue) {
tracker.setValue(nextValue);
return true;
}

return false;
},

stopTracking(inst: InstanceWithWrapperState | Fiber) {
var tracker = getTracker(inst);
updateValueIfChanged(instOrFiber: ReactInstance | Fiber) {
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Jul 18, 2017

Choose a reason for hiding this comment

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

What do you think about always accepting nodes instead? Since we immediately get the node anyway.

Copy link
Copy Markdown
Contributor Author

@jquense jquense Jul 18, 2017

Choose a reason for hiding this comment

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

That's fine with me, it just means we need to call getNode at the call sites: ReactDOMComponent and ChangeEventPlugin

if (!instOrFiber) {
return false;
}
return inputValueTracking.updateNodeValueIfChanged(
ReactDOMComponentTree.getNodeFromInstance(instOrFiber),
);
},

stopTracking(inst: ReactInstance) {
var tracker = getTracker(ReactDOMComponentTree.getNodeFromInstance(inst));
if (tracker) {
tracker.stopTracking();
}
Expand Down