Move input valueTracker to DOM nodes #10208

jquense · 2017-07-18T16:40:29Z

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: #10207

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

gaearon · 2017-07-18T16:42:09Z

src/renderers/dom/shared/inputValueTracking.js

  },

-  trackNode(node: ElementWithWrapperState) {
+  _getTrackerFromInstance(inst: ReactInstance | Fiber) {
+    return getTracker(ReactDOMComponentTree.getNodeFromInstance(inst));


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

gaearon · 2017-07-18T16:42:16Z

src/renderers/dom/shared/inputValueTracking.js

 }

-function detachTracker(subject: SubjectWithWrapperState) {
-  subject._wrapperState.valueTracker = null;
+function detachTracker(subject: ElementWithValueTracker) {


subject => node?

gaearon · 2017-07-18T16:42:45Z

src/renderers/dom/shared/inputValueTracking.js

    return false;
  },

-  stopTracking(inst: InstanceWithWrapperState | Fiber) {
-    var tracker = getTracker(inst);
+  updateValueIfChanged(instOrFiber: ReactInstance | Fiber) {


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

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

gaearon · 2017-07-18T18:06:02Z

src/renderers/dom/shared/__tests__/inputValueTracking-test.js

-    expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
-      true,
-    );
+    expect(node.hasOwnProperty('_valueTracker')).toBe(true);


Can also use getTracker here?

gaearon · 2017-07-18T18:06:32Z

src/renderers/dom/shared/__tests__/inputValueTracking-test.js

@@ -28,128 +30,128 @@ describe('inputValueTracking', () => {
    mockComponent = {_hostNode: input, _wrapperState: {}};


Can we remove mockComponent completely?

gaearon · 2017-07-18T18:23:59Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

-  if (inputValueTracking.updateValueIfChanged(targetInst)) {
+  if (
+    inputValueTracking.updateValueIfChanged(
+      ReactDOMComponentTree.getNodeFromInstance(targetInst),


Minor nit but it might look a bit cleaner if you extract inner expression into const targetNode = ...

gaearon · 2017-07-18T18:24:40Z

src/renderers/dom/shared/inputValueTracking.js

@@ -109,70 +108,42 @@ function trackValueOnNode(node: any, inst: any): ?ValueTracker {

 var inputValueTracking = {
  // exposed for testing
-  _getTrackerFromNode(node: ElementWithWrapperState) {
-    return getTracker(ReactDOMComponentTree.getInstanceFromNode(node));
+  _getTrackerFromNode(inst: ElementWithValueTracker) {


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

gaearon · 2017-07-18T18:39:14Z

Lint reports unused stuff:

/home/ubuntu/react/src/renderers/dom/shared/inputValueTracking.js
  15:14  error  'Fiber' is defined but never used                           no-unused-vars
  16:14  error  'ReactInstance' is defined but never used                   no-unused-vars
  26:5   error  'ReactDOMComponentTree' is assigned a value but never used  no-unused-vars

And you'll need to run scripts/fiber/record-tests to update test names.

jquense · 2017-07-18T18:40:06Z

oops sorry, my local lint is being odd.

aweary · 2017-07-18T18:50:11Z

src/renderers/dom/shared/inputValueTracking.js

+  }
+
+  if (isCheckable(node)) {
+    value = node.checked ? 'true' : 'false';


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

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

What about '' + bool?

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

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

gaearon · 2017-07-18T19:44:34Z

Seems like we need to get CI passing and this is good to go? I'll take another look tomorrow unless someone gets ahead of me.

gaearon · 2017-07-19T15:40:50Z

src/renderers/dom/shared/inputValueTracking.js

+function getValueFromNode(node: HTMLInputElement): string {
+  var value = '';
+  if (!node) {
+    return value;


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

@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.

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

gaearon

lgtm

gaearon · 2017-07-19T16:08:21Z

(Please "squash and merge")

gaearon · 2017-07-19T16:11:02Z

Congratulations on first merge btw. 🥇

Move input valueTracker to DOM nodes

a9d4104

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

facebook-github-bot added the CLA Signed label Jul 18, 2017

run prettier

c3b0eac

jquense requested a review from gaearon July 18, 2017 16:41

gaearon reviewed Jul 18, 2017

View reviewed changes

jquense added 2 commits July 18, 2017 12:53

remove instance accepting methods

b15092e

rm unused trst method

12317ad

gaearon reviewed Jul 18, 2017

View reviewed changes

address feedback

b4f3fd2

gaearon reviewed Jul 18, 2017

View reviewed changes

fix naming

f012837

fix lint

04ac179

aweary reviewed Jul 18, 2017

View reviewed changes

jquense mentioned this pull request Jul 18, 2017

Fix uncontrolled radios #10186

Merged

gaearon reviewed Jul 19, 2017

View reviewed changes

gaearon approved these changes Jul 19, 2017

View reviewed changes

jquense merged commit acd9818 into facebook:master Jul 19, 2017

jquense deleted the input-value-tracking-refactor branch July 19, 2017 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move input valueTracker to DOM nodes #10208

Move input valueTracker to DOM nodes #10208

jquense commented Jul 18, 2017

gaearon Jul 18, 2017

jquense Jul 18, 2017

gaearon Jul 18, 2017

gaearon Jul 18, 2017

jquense Jul 18, 2017

gaearon Jul 18, 2017

gaearon Jul 18, 2017

gaearon Jul 18, 2017

gaearon Jul 18, 2017

gaearon commented Jul 18, 2017

jquense commented Jul 18, 2017

aweary Jul 18, 2017

jquense Jul 18, 2017

gaearon Jul 18, 2017

jquense Jul 18, 2017

gaearon Jul 18, 2017

gaearon commented Jul 18, 2017

gaearon Jul 19, 2017

aweary Jul 19, 2017

jquense Jul 19, 2017 •

edited

Loading

gaearon left a comment

gaearon commented Jul 19, 2017

gaearon commented Jul 19, 2017

		@@ -28,128 +30,128 @@ describe('inputValueTracking', () => {
		mockComponent = {_hostNode: input, _wrapperState: {}};

Move input valueTracker to DOM nodes #10208

Move input valueTracker to DOM nodes #10208

Conversation

jquense commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jul 18, 2017

jquense commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Jul 19, 2017

gaearon commented Jul 19, 2017

jquense Jul 19, 2017 •

edited

Loading