Skip to content

Commit

Permalink
Improve soundness of ReactDOMFiberInput typings
Browse files Browse the repository at this point in the history
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](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.
  • Loading branch information
philipp-spiess committed Aug 11, 2018
1 parent 725e499 commit b2b714b
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 23 deletions.
36 changes: 13 additions & 23 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ import warning from 'shared/warning';

import * as DOMPropertyOperations from './DOMPropertyOperations';
import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree';
import {getSafeValue, safeValueToString} from './safeValue';
import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';
import * as inputValueTracking from './inputValueTracking';

import type {SafeValue} from './SafeValue';

type InputWithWrapperState = HTMLInputElement & {
_wrapperState: {
initialValue: string,
initialValue: SafeValue,
initialChecked: ?boolean,
controlled?: boolean,
},
Expand Down Expand Up @@ -174,13 +177,14 @@ export function updateWrapper(element: Element, props: Object) {
if (props.type === 'number') {
if (
(value === 0 && node.value === '') ||
// We explicitly want to coerce to number here if possible.
// eslint-disable-next-line
node.value != value
node.value != (value: any)
) {
node.value = '' + value;
node.value = safeValueToString(value);
}
} else if (node.value !== '' + value) {
node.value = '' + value;
} else if (node.value !== safeValueToString(value)) {
node.value = safeValueToString(value);
}
}

Expand All @@ -203,7 +207,7 @@ export function postMountWrapper(
const node = ((element: any): InputWithWrapperState);

if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
const initialValue = '' + node._wrapperState.initialValue;
const initialValue = safeValueToString(node._wrapperState.initialValue);
const currentValue = node.value;

// Do not assign value if it is already set. This prevents user text input
Expand Down Expand Up @@ -312,23 +316,9 @@ export function setDefaultValue(
node.ownerDocument.activeElement !== node
) {
if (value == null) {
node.defaultValue = '' + node._wrapperState.initialValue;
} else if (node.defaultValue !== '' + value) {
node.defaultValue = '' + value;
node.defaultValue = safeValueToString(node._wrapperState.initialValue);
} else if (node.defaultValue !== safeValueToString(value)) {
node.defaultValue = safeValueToString(value);
}
}
}

function getSafeValue(value: *): * {
switch (typeof value) {
case 'boolean':
case 'number':
case 'object':
case 'string':
case 'undefined':
return value;
default:
// function, symbol are assigned as empty strings
return '';
}
}
31 changes: 31 additions & 0 deletions packages/react-dom/src/client/SafeValue.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

export opaque type SafeValue = boolean | number | Object | string | null | void;

// Flow does not allow string concatenation of most non-string types. To work
// around this limitation, we use an opaque type that can only be obtained by
// passing the value through getSafeValue first.
export function safeValueToString(value: SafeValue): string {
return '' + (value: any);
}

export function getSafeValue(value: mixed): SafeValue {
switch (typeof value) {
case 'boolean':
case 'number':
case 'object':
case 'string':
case 'undefined':
return value;
default:
// function, symbol are assigned as empty strings
return '';
}
}

0 comments on commit b2b714b

Please sign in to comment.