Ensure value and defaultValue do not assign functions and symbols #11741

nhunzaker · 2017-12-01T13:50:35Z

I think this addresses #11734. I need to verify with the attribute table.

nhunzaker · 2017-12-01T13:53:20Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+      // Use hasOwnProperty instead of checking for '' to ensure properties
+      // are set for the first time. This is important for defaultValue, which
+      // otherwise does not set an empty value attribute
+      if (!node.hasOwnProperty(property) || node[property] !== '' + value) {


We must only compare the stringified value property/attribute after we've confirmed that it isn't a symbol. Otherwise you get a run time exception:

Maybe a safer way to do this would be to wrap the value in a string constructor?

AFAIK String() is slower, at least in V8.

Sure enough:

Should we add a TODO: to remove this on a major bump? It's not clear to me why ignoring other types vs throwing or defering to whatever the browser does is more corrrect

I think the constraint we care about is that objects get stringified. Since we depend on this a lot. Therefore we can't use browser APIs that don't attempt to stringify them.

Sure that' makes ense, tho I think "treating the as null" vs an error is maybe less ideal. In, the error case it's easier track down and fix vs inputs clearing and maybe not noticing

I believe the unknown property hook will still raise a warning.

nhunzaker · 2017-12-01T13:58:54Z

The attribute table seems correct, though now I wonder if we should share this behavior with textarea and select:

Attribute Table:

AttributeTableSnapshot.md.zip

gaearon · 2017-12-01T14:04:07Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+  value: *,
+) {
+  // TODO: This should use DOMProperty.shouldSetAttribute, however the current
+  // behavior is such that invalid attributes do not update the current attribute,


This sounds like a bug to me. I'd like to always treat them as null instead of showing stale values. I'd be fine changing behavior even if that's what we did in 16 because I think it's more likely to cause product bugs.

gaearon · 2017-12-01T14:10:23Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+        // Cast `value` to a string to ensure the value is set correctly. While
+        // browsers typically do this as necessary, jsdom doesn't.
+        // TODO: Is this still true with reasonably modern JSDOM?
+        node[property] = '' + value;


I'd expect dynamic property access to be slower than just setting a property normally.

gaearon

I'm not a fan of the assignProperty indirection. I'd like to keep direct access.

Can we doctor the value a bit earlier instead? e.g.

if (itsbad) {
  value = null
}

or something like it?

I don't mind changing behavior to "always reset to null for bad inputs" since keeping stale values seems worse to me.

nhunzaker · 2017-12-01T14:22:08Z

I don't mind changing behavior to "always reset to null for bad inputs" since keeping stale values seems worse to me.

I think that makes things a bit easier.

nhunzaker · 2017-12-01T14:56:42Z

packages/react-dom/src/events/ChangeEventPlugin.js

@@ -228,17 +229,15 @@ function handleControlledInputBlur(inst, node) {
  }

  // Fiber and ReactDOM keep wrapper state in separate places
+  // TODO: Is this still necessary now that Stack is gone?
  let state = inst._wrapperState || node._wrapperState;


Left a note to follow up here.

this should not be necessary anymore :) 👍

nhunzaker · 2017-12-01T14:57:01Z

packages/react-dom/src/shared/DOMProperty.js

  defaultChecked: true,
+  // TODO: This prevents the assignment of defaultValue to regular
+  // elements (not just inputs). Now that ReactDOMInput assigns to the
+  // defaultValue property -- do we need this?


Left a note to follow up here.

nhunzaker · 2017-12-01T14:58:26Z

packages/react-dom/src/events/ChangeEventPlugin.js

-  if (node.getAttribute('value') !== value) {
-    node.setAttribute('value', value);
-  }
+  synchronizeDefaultValue(node, node.type, node.value);


@gaearon new here: I forgot about the number behavior on blur when we change inputs. This is so that chrome doesn't validate number inputs when we assign the default value. The DOM Operations are now consistent.

nhunzaker · 2017-12-01T14:59:01Z

Pushed a bit of a work in progress pass. There's too much branching. I think we can eliminate a few conditions.

gaearon · 2017-12-01T15:25:17Z

Let me know when you’re satisfied :-)

nhunzaker · 2017-12-01T15:28:37Z

@gaearon pushed up some final bits. I'd appreciate a very critical eye. I'm a bit tapped for time to invest in this, but I can pick it up later today and answer any questions.

nhunzaker · 2017-12-01T15:40:05Z

Basic browser testing checks out. It looks like this really does fix #8395:

outdated

gaearon · 2017-12-01T16:07:24Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+    });
+
+    it('does not update the value to a Symbol', function() {
+      var container = document.createElement('div');


Shouldn't it display a warning?

gaearon · 2017-12-01T17:16:12Z

Mind rebasing now?

gaearon · 2017-12-01T17:16:18Z

Or I could.

nhunzaker · 2017-12-01T17:26:53Z

@gaearon I can get it if you haven't already started.

nhunzaker · 2017-12-01T17:51:05Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+      expect(node.value).toBe('');
+      expect(node.getAttribute('value')).toBe('');
+    });
+  });


@gaearon assigning defaultValue does not raise a warning. This is because defaultValue is considered a reserved prop:
https://github.com/facebook/react/blob/master/packages/react-dom/src/shared/DOMProperty.js#L12-L21

Removing it from the reserved props list addresses the issue, but then a ton of server integration tests fail. The current 16.2.0 behavior also does not raise a warning:

Do we want to address the lack of a warning for defaultValue in this PR?

No, it's okay.

aweary · 2017-12-01T17:52:57Z

packages/react-dom/src/client/ReactDOMFiberInput.js

  var node = ((element: any): InputWithWrapperState);
+
+  if (!shouldSetAttribute('value', initialValue)) {


Not a blocker, but it might be worth using a method specific to value since shouldSetAttribute does a few unnecessary checks in this case. It might also call shouldAttributeAcceptBooleanValue which also does more work than needed

I don't think this is a really hot path, so it's probably fine the way it is, but just worth noting.

Yeah. I had an assignProperty method earlier, but we thought it was too much indirection. I'd be happy to revisit this though.

Let's fork shouldSetAttribute here to shouldSetValue and remove the unneeded branches? I'm not against a special method per se, I'm against a special method for doing the assignment.

gaearon

Let's separate the two unrelated changes here (symbol/function and synchronizing refactoring). Also let's ensure we treat it as empty string rather than as null.

outdated

5: Update react monorepo to v16.3.2 r=mythmon a=renovate[bot] This Pull Request renovates the package group "react monorepo". - [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.2.0` to `16.3.2` - [react](https://github.com/facebook/react) (`dependencies`): from `16.2.0` to `16.3.2` # Release Notes <details> <summary>facebook/react</summary> ### [`v16.3.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1630-March-29-2018) ##### React * Add a new officially supported context API. ([@acdlite] in [#11818](`facebook/react#11818)) * Add a new `React.createRef()` API as an ergonomic alternative to callback refs. ([@trueadm] in [#12162](`facebook/react#12162)) * Add a new `React.forwardRef()` API to let components forward their refs to a child. ([@bvaughn] in [#12346](`facebook/react#12346)) * Fix a false positive warning in IE11 when using `React.Fragment`. ([@XaveScor] in [#11823](`facebook/react#11823)) * Replace `React.unstable_AsyncComponent` with `React.unstable_AsyncMode`. ([@acdlite] in [#12117](`facebook/react#12117)) * Improve the error message when calling `setState()` on an unmounted component. ([@sophiebits] in [#12347](`facebook/react#12347)) ##### React DOM * Add a new `getDerivedStateFromProps()` lifecycle and `UNSAFE_` aliases for the legacy lifecycles. ([@bvaughn] in [#12028](`facebook/react#12028)) * Add a new `getSnapshotBeforeUpdate()` lifecycle. ([@bvaughn] in [#12404](`facebook/react#12404)) * Add a new `<React.StrictMode>` wrapper to help prepare apps for async rendering. ([@bvaughn] in [#12083](`facebook/react#12083)) * Add support for `onLoad` and `onError` events on the `<link>` tag. ([@roderickhsiao] in [#11825](`facebook/react#11825)) * Add support for `noModule` boolean attribute on the `<script>` tag. ([@aweary] in [#11900](`facebook/react#11900)) * Fix minor DOM input bugs in IE and Safari. ([@nhunzaker] in [#11534](`facebook/react#11534)) * Correctly detect Ctrl + Enter in `onKeyPress` in more browsers. ([@nstraub] in [#10514](`facebook/react#10514)) * Fix containing elements getting focused on SSR markup mismatch. ([@koba04] in [#11737](`facebook/react#11737)) * Fix `value` and `defaultValue` to ignore Symbol values. ([@nhunzaker] in [#11741](`facebook/react#11741)) * Fix refs to class components not getting cleaned up when the attribute is removed. ([@bvaughn] in [#12178](`facebook/react#12178)) * Fix an IE/Edge issue when rendering inputs into a different window. ([@M-ZubairAhmed] in [#11870](`facebook/react#11870)) * Throw with a meaningful message if the component runs after jsdom has been destroyed. ([@gaearon] in [#11677](`facebook/react#11677)) * Don't crash if there is a global variable called `opera` with a `null` value. [@alisherdavronov] in [#11854](`facebook/react#11854)) * Don't check for old versions of Opera. ([@skiritsis] in [#11921](`facebook/react#11921)) * Deduplicate warning messages about `<option selected>`. ([@watadarkstar] in [#11821](`facebook/react#11821)) * Deduplicate warning messages about invalid callback. ([@yenshih] in [#11833](`facebook/react#11833)) * Deprecate `ReactDOM.unstable_createPortal()` in favor of `ReactDOM.createPortal()`. ([@prometheansacrifice] in [#11747](`facebook/react#11747)) * Don't emit User Timing entries for context types. ([@abhaynikam] in [#12250](`facebook/react#12250)) * Improve the error message when context consumer child isn't a function. ([@raunofreiberg] in [#12267](`facebook/react#12267)) * Improve the error message when adding a ref to a functional component. ([@skiritsis] in [#11782](`facebook/react#11782)) ##### React DOM Server * Prevent an infinite loop when attempting to render portals with SSR. ([@gaearon] in [#11709](`facebook/react#11709)) * Warn if a class doesn't extend `React.Component`. ([@wyze] in [#11993](`facebook/react#11993)) * Fix an issue with `this.state` of different components getting mixed up. ([@sophiebits] in [#12323](`facebook/react#12323)) * Provide a better message when component type is undefined. ([@HeroProtagonist] in [#11966](`facebook/react#11966)) --- ### [`v16.3.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1631-April-3-2018) ##### React * Fix a false positive warning in IE11 when using `Fragment`. ([@heikkilamarko] in [#12504](`facebook/react#12504)) * Prefix a private API. ([@Andarist] in [#12501](`facebook/react#12501)) * Improve the warning when calling `setState()` in constructor. ([@gaearon] in [#12532](`facebook/react#12532)) ##### React DOM * Fix `getDerivedStateFromProps()` not getting applied in some cases. ([@acdlite] in [#12528](`facebook/react#12528)) * Fix a performance regression in development mode. ([@gaearon] in [#12510](`facebook/react#12510)) * Fix error handling bugs in development mode. ([@gaearon] and [@acdlite] in [#12508](`facebook/react#12508)) * Improve user timing API messages for profiling. ([@flarnie] in [#12384](`facebook/react#12384)) ##### Create Subscription * Set the package version to be in sync with React releases. ([@bvaughn] in [#12526](`facebook/react#12526)) * Add a peer dependency on React 16.3+. ([@NMinhNguyen] in [#12496](`facebook/react#12496)) --- ### [`v16.3.2`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1632-April-16-2018) ##### React * Improve the error message when passing `null` or `undefined` to `React.cloneElement`. ([@nicolevy] in [#12534](`facebook/react#12534)) ##### React DOM * Fix an IE crash in development when using `<StrictMode>`. ([@bvaughn] in [#12546](`facebook/react#12546)) * Fix labels in User Timing measurements for new component types. ([@bvaughn] in [#12609](`facebook/react#12609)) * Improve the warning about wrong component type casing. ([@nicolevy] in [#12533](`facebook/react#12533)) * Improve general performance in development mode. ([@gaearon] in [#12537](`facebook/react#12537)) * Improve performance of the experimental `unstable_observedBits` API with nesting. ([@gaearon] in [#12543](`facebook/react#12543)) ##### React Test Renderer * Add a UMD build. ([@bvaughn] in [#12594](`facebook/react#12594)) --- </details> # Commits <details> <summary>facebook/react</summary> #### v16.3.0 - [`c2c3c0c`](facebook/react@c2c3c0cc36878cd6f020a480b83ff1c03b62fd28)Fix build script to handle react-is (no peer deps) (#12471) - [`488ad5a`](facebook/react@488ad5a6b94ac4b71ff587ecde05e48a218aba62)Fix typo in create-subscription readme - [`c1b21a7`](facebook/react@c1b21a746c7d08554eed8bf55030a4049380c32c)Added DEV warning if getSnapshotBeforeUpdate is defined as a static method (#12475) - [`268a3f6`](facebook/react@268a3f60dfe67c4f6439fc37b569a2d81c81a53a)Add unstable APIs for async rendering to test renderer (#12478) - [`c44665e`](facebook/react@c44665e83278becfe7a3afdf788789536d63387b)Fix bug when fatal error is thrown as a result of `batch.commit` (#12480) - [`7a833da`](facebook/react@7a833dad95b3059ebfdfba44d3fa68e1301d8e6a)setState() in componentDidMount() should flush synchronously even with createBatch() (#12466) - [`5855e9f`](facebook/react@5855e9f2158b31d945f3fcc5bc582389dbecc88e)Improve warning message for setState-on-unmounted (#12347) - [`15e3dff`](facebook/react@15e3dffb4c9ca9b9466f4ef1a6b8b2293d41e9d6)Don&#x27;t bail out on referential equality of Consumer's props.children function (#12470) - [`125dd16`](facebook/react@125dd16ba0b3fa74767b1cf417a3116a4a2b251a)Update user timing to record the timeout deadline with 'waiting' events (#12479) - [`96fe3b1`](facebook/react@96fe3b1be2fe74e83c9a25d7511f23dbef15ac99)Add React.isValidElementType() (#12483) - [`53fdc19`](facebook/react@53fdc19df092bbc4bd736aea4ef8e0f12d692ee6)Updated react-is README to show new isValidElementType() - [`8650d2a`](facebook/react@8650d2a1357985958c2738da55ea349406482721)Disable createRoot for open source builds (#12486) - [`6294b67`](facebook/react@6294b67a406d21cc6b65162e47497c1e8afe398f)unstable_createRoot (#12487) - [`b2379d4`](facebook/react@b2379d4cbe82653da931ccb128916707bc53d28a)Updating package versions for release 16.3.0 - [`9778873`](facebook/react@9778873143072635a795fec2ad0e1ac0bb7d8b91)Updating dependencies for react-noop-renderer - [`8e3d94f`](facebook/react@8e3d94ffa1d2e19a5bf4b9f8030973b65b0fc854)Update bundle sizes for 16.3.0 release #### v16.3.1 - [`2c3f5fb`](facebook/react@2c3f5fb97b6ea077f3e9aae6c6587bfe8328036d)Add React 16.3.0 changelog (#12488) - [`4304475`](facebook/react@43044757e55eca13ae788056b59f94788fc15050)Fix links - [`18ba36d`](facebook/react@18ba36d89165ec15655f2606b0a6ba2e709ce641)Move context API in Changelog to "React" section - [`59b3905`](facebook/react@59b39056d91787f6a3e4e0dfc0825c8687bd0af9)Fix method name in changelog - [`fa8e678`](facebook/react@fa8e67893fca1b3902637129972032bca248a584)Change create-subscription's peerDep on react to ^16.3.0 (#12496) - [`0c80977`](facebook/react@0c80977061ba576cee9ae0891245be233929d2ed)Validate React.Fragment props without Map. (#12504) - [`59dac9d`](facebook/react@59dac9d7a6a2f0b66003cf717d71b5587265423f)Fix DEV performance regression by avoiding Object.assign on Fibers (#12510) - [`6b99c6f`](facebook/react@6b99c6f9d376bacbb769264d743c405b495b03ad)Add missing changelog item - [`7a27ebd`](facebook/react@7a27ebd52a3025a946c67eaf84d2646fd307cb44)Update user timing to record when we are about to commit (#12384) - [`4ccf58a`](facebook/react@4ccf58a94dce323718540b8185a32070ded6094b)Fix context stack misalignment caused by error replay (#12508) - [`6f2ea73`](facebook/react@6f2ea73978168372f33a6dfad6c049afddc4aef3)Extract throw to separate function so performUnitOfWork does not deopt (#12521) - [`ba245f6`](facebook/react@ba245f6f9b0bf31c2ebff5c087c21bcae111e6c3)Prefix _context property on returned ReactContext from createContext - it's private (#12501) - [`eb6e752`](facebook/react@eb6e752cabafed0b72e1d0a38819ff156557d537)Bumped create-subscription package version (#12526) - [`da4e855`](facebook/react@da4e85567b411a180c2cfa1ef6573cf3cc9257f1)Remove @providesModule in www bundles (#12529) - [`0f2f90b`](https://github.com/facebook/react/commit/0f2f90bd9a9daf241d691bf4af3ea2e3a263c0e3)getDerivedStateFrom{Props,Catch} should update updateQueue.baseState (#12528) - [`36c2939`](facebook/react@36c29393720157a3966ce1d50449a33a35bdf14c)Improve not-yet-mounted setState warning (#12531) - [`a2cc3c3`](facebook/react@a2cc3c38e214c16ff6805312d4353c1faab9ff95)Follow up: make new warning less wordy (#12532) - [`2279843`](facebook/react@2279843ef966ea2e0460986efa1f91513cd50623)Updating yarn.lock file for 16.3.1 release - [`787b343`](facebook/react@787b343f674c72837209bdffd55c59682910d807)Updating package versions for release 16.3.1 - [`dc05957`](facebook/react@dc059579c3e56ca338a999b86d146d2341ee6f64)Update bundle sizes for 16.3.1 release - [`b15b165`](facebook/react@b15b165e0798dca03492e354ebd5bcf87b711184)Changelog for 16.3.1 #### v16.3.2 - [`1c2876d`](facebook/react@1c2876d5b558b8591feb335d8d7204bc46f7da8a)Add a build step to hoist warning conditions (#12537) - [`5e3706c`](facebook/react@5e3706cca0fe0da462c771d14a271cd2961e5718)Don&#x27;t render bitmask-bailing consumers even if there's a deeper matching child (#12543) - [`e932e32`](https://github.com/facebook/react/commit/e932e321a88e07935224701bc4580e3dc9889afe)facebook.github.io/react -> reactjs.org (#12545) - [`d328e36`](facebook/react@d328e362e86a6af4a0664e004b8f97f18ce972c8)Removed duplicate typeof check (#12541) - [`8ec0e4a`](facebook/react@8ec0e4a99df76c0ff1779cac4f2eaaaf35a6b5bb)Removed Array.from() usage (#12546) - [`27535e7`](facebook/react@27535e7bfcb63e8a4d65f273311e380b4ca12eff)Clarify ReactDOM's case warning for html tags (#12533) - [`7a3416f`](facebook/react@7a3416f27532ac25849dfbc505300d469b43bbcc)Expose component stack from reactTag to React Native renderer (#12549) - [`cf649b4`](facebook/react@cf649b40a56dc5c0ffe2595b963847f0ff8de245)Move TouchHistoryMath to React Native repo (#12557) - [`5b16b39`](facebook/react@5b16b39508ec33f2f374a5a12aa71647e1728d08)Bug fix - [`6bf2797`](facebook/react@6bf2797d6cf76676791424afc93b76dd60d7074c)Remove flushSync from React Native (#12565) - [`bc753a7`](facebook/react@bc753a716e185c31d8eb7404ab5dd6ee7467b7cb)Support findNodeHandle in Fabric (#12573) - [`181747a`](https://github.com/facebook/react/commit/181747a6cc25f3020b8561f475eca4ad2824256b)[RN] Move takeSnapshot to RN (#12574) - [`20c5d97`](facebook/react@20c5d97bb6c6a4af76d66a7e5134952989f9a9b2)Keep consistency in the comment (#12579) - [`ea37545`](facebook/react@ea3754503742afc3d5c5de2140717817794870ec)Must be *a* before PlacementAndUpdate (#12580) - [`76b4ba0`](facebook/react@76b4ba01290f446f4313adf3846954412c6051b8)Preserve error codes for invariants on www (#12539) - [`8dfb057`](facebook/react@8dfb0578816435a1a72f04506ee20d3c55d0f9bc)Unfork invariant and instead use it from reactProdInvariant (#12585) - [`f88deda`](facebook/react@f88deda83bab316385f39e8479850527cda90607)Throw more specific error if passed undefined in React.cloneElement (#12534) - [`2f7bca0`](facebook/react@2f7bca0eb2487955e71a45e288e5847b5af522a5)Allocate unique reactTags for RN and Fabric (#12587) - [`933f882`](facebook/react@933f882a9df728662befe558005f2ea3fe827a1d)Remove ReactNativePropRegistry (#12559) - [`40d0772`](https://github.com/facebook/react/commit/40d07724fcc801ad69e17b295b68ebea753d5977)[RN] Remove unstable_batchedUpdates and unmountComponentAtNodeAndRemoveContainer from Fabric (#12571) - [`b6e0512`](facebook/react@b6e0512a81524d397ff4fbfb892372ecc84c6b02)Consolidate eventTypes registry with view configs (#12556) - [`b99d0b1`](https://github.com/facebook/react/commit/b99d0b14160150c566e091bd10b634beec9a58c3)[RN] Move view config registry to shims (#12569) - [`725c054`](facebook/react@725c054d4d5d07c5c553a1ca724b01f2e6a43c5d)Refactor findHostInstance and findNodeHandle (#12575) - [`52afbe0`](facebook/react@52afbe0ebb6fca0fe480e77c6fa8482870ddb2c9)createReactNativeComponentClass needs to be CommonJS - [`3eae866`](facebook/react@3eae866e03a96c4f46e257cba73ca158b049ab05)Fixes language in error message. (#12590) - [`b846152`](facebook/react@b8461524db6d3e016fabf001ad8fa086b4918ef9)Added UMD build to test renderer package (#12594) - [`3e9515e`](facebook/react@3e9515eedebe0c19f047391605c5b3c71d13fbc2)Remove @providesModule in www shims - [`915bb53`](facebook/react@915bb5321a8db3435eb36ef1cf9414c15333b447)Bump expiration for interactive updates to 150ms in production (#12599) - [`c27a998`](https://github.com/facebook/react/commit/c27a99812e75e73d9fad88c97ac8b8db452012c1)[Danger] Minor fixes (#12606) - [`5dfbfe9`](facebook/react@5dfbfe9da740398c0a2cf4d897a0085000d06b7b)Fixed debug performance labels for new component types (#12609) - [`1591c8e`](facebook/react@1591c8ebab6151f3cae59ad42e3c15acc52cd67b)Update GCC (#12618) - [`a4cef29`](facebook/react@a4cef2970341c08e5c16a2406fbf532fc8053d12)tests: add regression test for reading ReactCurrentOwner stateNode (#12412) - [`2e1cc28`](facebook/react@2e1cc2802709877fb2454163ba30e52a91feac8e)Fix small typos in create-subscription readme (#12399) - [`1e97a71`](facebook/react@1e97a71a829e698ddac0a5e15fbdec97d35ed2bc)Fix documentation of the release process (#12337) - [`66c44a7`](facebook/react@66c44a7bc34cb3fcb3c788dcce3f3345a5bd9f58)Updating yarn.lock file for 16.3.2 release - [`82f67d6`](facebook/react@82f67d65fd4584c4528352e6b9166ca4da282382)Updating package versions for release 16.3.2 - [`6494f6b`](facebook/react@6494f6b6b8e1cfa5df9f72b4d94cf9ed582805cd)Update error codes for 16.3.2 release - [`3232616`](facebook/react@32326163480b5028ee16f6b4e4ea4426f3c5e95c)Update bundle sizes for 16.3.2 release - [`01402f4`](facebook/react@01402f4ad922b5467812586567519e9e5bbd595f)Add 16.3.2 changelog (#12621) </details> --- This PR has been generated by [Renovate Bot](https://renovateapp.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>

This is an attempt in improving the soundness for the safe value cast that was added in facebook#11741. We want this to avoid situations like [this one](https://github.com/facebook/react/pull/13362/files/e2dacd1976a70de09c65a4822e190463f08c4feb#r209380079) where we need to remember why we have certain type casts. Additionally we can be sure that we only case safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This works because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good.

This is an attempt in improving the soundness for the safe value cast that was added in facebook#11741. We want this to avoid situations like [this one](https://github.com/facebook/react/pull/13362/files/e2dacd1976a70de09c65a4822e190463f08c4feb#r209380079) where we need to remember why we have certain type casts. Additionally we can be sure that we only cast safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This is sound because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good.

This is an attempt in improving the soundness for the safe value cast that was added in facebook#11741. We want this to avoid situations like [this one](facebook#13362 (comment)) where we need to remember why we have certain type casts. Additionally we can be sure that we only cast safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This is sound because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good.

* Improve soundness of ReactDOMFiberInput typings This is an attempt in improving the soundness for the safe value cast that was added in #11741. We want this to avoid situations like [this one](#13362 (comment)) where we need to remember why we have certain type casts. Additionally we can be sure that we only cast safe values to string. The problem was `getSafeValue()`. It used the (deprecated) `*` type to infer the type which resulted in a passing-through of the implicit `any` of the props `Object`. So `getSafeValue()` was effectively returning `any`. Once I fixed this, I found out that Flow does not allow concatenating all possible types to a string (e.g `"" + false` fails in Flow). To fix this as well, I've opted into making the SafeValue type opaque and added a function that can be used to get the string value. This is sound because we know that SafeValue is already checked. I've verified that the interim function is inlined by the compiler and also looked at a diff of the compiled react-dom bundles to see if I've regressed anything. Seems like we're good. * Fix typo

nhunzaker commented Dec 1, 2017

View reviewed changes

nhunzaker requested review from gaearon, jquense and aweary December 1, 2017 13:59

gaearon reviewed Dec 1, 2017

View reviewed changes

gaearon previously requested changes Dec 1, 2017

View reviewed changes

nhunzaker commented Dec 1, 2017

View reviewed changes

facebook-github-bot added the CLA Signed label Dec 1, 2017

gaearon reviewed Dec 1, 2017

View reviewed changes

nhunzaker force-pushed the fix-value-assignment branch from 38d7bc5 to f057809 Compare December 1, 2017 17:47

nhunzaker commented Dec 1, 2017

View reviewed changes

aweary reviewed Dec 1, 2017

View reviewed changes

gaearon mentioned this pull request Dec 1, 2017

value|defaultValue={Symbol|Function} should be ignored, not stringified #11734

Open

gaearon previously requested changes Dec 1, 2017

View reviewed changes

nhunzaker added 2 commits December 2, 2017 12:10

Ensure value and defaultValue do not assign functions and symbols

e7ed5d6

Eliminate assignProperty method from ReactDOMInput

bf28fa3

renovate bot mentioned this pull request Apr 2, 2018

Update react monorepo packages to v16.3.0 mozilla/ensemble#74

Merged

This was referenced Apr 2, 2018

react-test-renderer versions available: 16.3.0 serlo/serlo-abc#306

Closed

Update react-dom in / from 16.2.0 to 16.3.0 severest/retrobot#88

Merged

Update react in / from 16.2.0 to 16.3.0 severest/retrobot#90

Merged

Update yarn.lock dropseed/jump#97

Closed

This was referenced Apr 17, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

philipp-spiess mentioned this pull request Aug 10, 2018

Fix passing symbols and functions to textarea #13362

Merged

philipp-spiess mentioned this pull request Aug 11, 2018

Improve soundness of ReactDOMFiberInput typings #13367

Merged

sebmarkbage mentioned this pull request Mar 14, 2022

test: Update attribute fixture snapshot #24083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure value and defaultValue do not assign functions and symbols #11741

Ensure value and defaultValue do not assign functions and symbols #11741

nhunzaker commented Dec 1, 2017

nhunzaker Dec 1, 2017

gaearon Dec 1, 2017

nhunzaker Dec 1, 2017 •

edited

jquense Dec 1, 2017

gaearon Dec 1, 2017

jquense Dec 1, 2017 •

edited

nhunzaker Dec 1, 2017

nhunzaker commented Dec 1, 2017

gaearon Dec 1, 2017

gaearon Dec 1, 2017

gaearon left a comment •

edited

nhunzaker commented Dec 1, 2017

nhunzaker Dec 1, 2017

jquense Dec 1, 2017 •

edited

nhunzaker Dec 1, 2017

nhunzaker Dec 1, 2017

nhunzaker commented Dec 1, 2017

gaearon commented Dec 1, 2017

nhunzaker commented Dec 1, 2017

nhunzaker commented Dec 1, 2017

gaearon Dec 1, 2017

gaearon commented Dec 1, 2017

gaearon commented Dec 1, 2017

nhunzaker commented Dec 1, 2017

nhunzaker Dec 1, 2017

gaearon Dec 1, 2017

aweary Dec 1, 2017

nhunzaker Dec 1, 2017

gaearon Dec 1, 2017 •

edited

gaearon left a comment

		var node = ((element: any): InputWithWrapperState);

		if (!shouldSetAttribute('value', initialValue)) {

Ensure value and defaultValue do not assign functions and symbols #11741

Ensure value and defaultValue do not assign functions and symbols #11741

Conversation

nhunzaker commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment • edited

Choose a reason for hiding this comment

nhunzaker commented Dec 1, 2017

Choose a reason for hiding this comment

jquense Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Dec 1, 2017

gaearon commented Dec 1, 2017

nhunzaker commented Dec 1, 2017

nhunzaker commented Dec 1, 2017

Choose a reason for hiding this comment

gaearon commented Dec 1, 2017

gaearon commented Dec 1, 2017

nhunzaker commented Dec 1, 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

gaearon Dec 1, 2017 • edited

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

nhunzaker Dec 1, 2017 •

edited

jquense Dec 1, 2017 •

edited

gaearon left a comment •

edited

jquense Dec 1, 2017 •

edited

gaearon Dec 1, 2017 •

edited