Skip to content

Commit

Permalink
Don't report lint error if coercion is safe
Browse files Browse the repository at this point in the history
This commit extends the safe-string-coercion internal ESLint rule to
auto-detect code where coercion can't fail or is correctly preceded
by a DEV check. In those cases, the rule now won't report.

Changes:
* Don't report if the coercion is wrapped in an `if (typeof...) {}`
  block that ensures that the coercion won't throw
* Don't report if there's a valid coercion check call in the
  statement immediately before the coercion. The check call must be in
  the following format:
    if (__DEV__) {
      checkXxxxxStringCoercion(value);
    };
* Renames rule option for clarity. New name: `isProductionUserAppCode`
* Removes now-unnecessary `eslint-disable*` comments
* Improve messages returned from ESLint when the DEV check is required,
  but is missing or invalid. If invalid, message tells what's wrong.
  • Loading branch information
justingrant committed Sep 19, 2021
1 parent 585ed30 commit ab0e0c9
Show file tree
Hide file tree
Showing 35 changed files with 566 additions and 133 deletions.
4 changes: 2 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ module.exports = {
'react-internal/no-primitive-constructors': ERROR,
'react-internal/safe-string-coercion': [
ERROR,
{disallowStringConstructor: true},
{isProductionUserAppCode: true},
],
'react-internal/no-to-warn-dev-within-to-throw': ERROR,
'react-internal/invariant-args': ERROR,
Expand Down Expand Up @@ -181,7 +181,7 @@ module.exports = {
'react-internal/warning-args': OFF,
'react-internal/safe-string-coercion': [
ERROR,
{disallowStringConstructor: false},
{isProductionUserAppCode: false},
],

// Disable accessibility checks
Expand Down
1 change: 0 additions & 1 deletion packages/react-devtools-core/src/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ function getArgumentsForLineNumber(
case 'rmate':
case 'mate':
case 'mine':
// eslint-disable-next-line react-internal/safe-string-coercion
return ['--line', lineNumber + '', filePath];
case 'code':
return ['-g', filePath + ':' + lineNumber];
Expand Down
1 change: 0 additions & 1 deletion packages/react-devtools-extensions/src/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ chrome.runtime.onConnect.addListener(function(port) {
});

function isNumeric(str: string): boolean {
// eslint-disable-next-line react-internal/safe-string-coercion
return +str + '' === str;
}

Expand Down
1 change: 0 additions & 1 deletion packages/react-devtools-extensions/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ function createPanelIfReactLoaded() {

function initBridgeAndStore() {
const port = chrome.runtime.connect({
// eslint-disable-next-line react-internal/safe-string-coercion
name: '' + tabId,
});
// Looks like `port.onDisconnect` does not trigger on in-tab navigation like new URL or back/forward navigation,
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ export function getValueForProperty(
const {propertyName} = propertyInfo;
return (node: any)[propertyName];
} else {
// This check protects multiple uses of `expected`, which is why the
// react-internal/safe-string-coercion rule is disabled in several spots
// below.
if (__DEV__) {
checkAttributeStringCoercion(expected, name);
}
Expand Down Expand Up @@ -130,7 +133,6 @@ export function getValueForAttribute(
if (__DEV__) {
checkAttributeStringCoercion(expected, name);
}
// eslint-disable-next-line react-internal/safe-string-coercion
if (value === '' + (expected: any)) {
return expected;
}
Expand Down Expand Up @@ -170,7 +172,6 @@ export function setValueForProperty(
}
node.setAttribute(
attributeName,
// eslint-disable-next-line react-internal/safe-string-coercion
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
);
}
Expand Down Expand Up @@ -210,7 +211,6 @@ export function setValueForProperty(
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
// eslint-disable-next-line react-internal/safe-string-coercion
attributeValue = '' + (value: any);
}
if (propertyInfo.sanitizeURL) {
Expand Down
5 changes: 0 additions & 5 deletions packages/react-dom/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ if (__DEV__) {
checkHtmlStringCoercion(markup);
}
const markupString =
// eslint-disable-next-line react-internal/safe-string-coercion
typeof markup === 'string' ? markup : '' + (markup: any);
return markupString
.replace(NORMALIZE_NEWLINES_REGEX, '\n')
Expand Down Expand Up @@ -308,7 +307,6 @@ function setInitialDOMProperties(
setTextContent(domElement, nextProp);
}
} else if (typeof nextProp === 'number') {
// eslint-disable-next-line react-internal/safe-string-coercion
setTextContent(domElement, '' + nextProp);
}
} else if (
Expand Down Expand Up @@ -754,7 +752,6 @@ export function diffProperties(
}
} else if (propKey === CHILDREN) {
if (typeof nextProp === 'string' || typeof nextProp === 'number') {
// eslint-disable-next-line react-internal/safe-string-coercion
(updatePayload = updatePayload || []).push(propKey, '' + nextProp);
}
} else if (
Expand Down Expand Up @@ -988,12 +985,10 @@ export function diffHydratedProperties(
updatePayload = [CHILDREN, nextProp];
}
} else if (typeof nextProp === 'number') {
// eslint-disable-next-line react-internal/safe-string-coercion
if (domElement.textContent !== '' + nextProp) {
if (__DEV__ && !suppressHydrationWarning) {
warnForTextDifference(domElement.textContent, nextProp);
}
// eslint-disable-next-line react-internal/safe-string-coercion
updatePayload = [CHILDREN, '' + nextProp];
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ export function createInstance(
typeof props.children === 'string' ||
typeof props.children === 'number'
) {
// eslint-disable-next-line react-internal/safe-string-coercion
const string = '' + props.children;
const ownAncestorInfo = updatedAncestorInfo(
hostContextDev.ancestorInfo,
Expand Down Expand Up @@ -331,7 +330,6 @@ export function prepareUpdate(
(typeof newProps.children === 'string' ||
typeof newProps.children === 'number')
) {
// eslint-disable-next-line react-internal/safe-string-coercion
const string = '' + newProps.children;
const ownAncestorInfo = updatedAncestorInfo(
hostContextDev.ancestorInfo,
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/client/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,6 @@ function updateNamedCousins(rootNode, props) {
checkAttributeStringCoercion(name, 'name');
}
const group = queryRoot.querySelectorAll(
// eslint-disable-next-line react-internal/safe-string-coercion
'input[name=' + JSON.stringify('' + name) + '][type="radio"]',
);

Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/client/ToStringValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export opaque type ToStringValue =
// around this limitation, we use an opaque type that can only be obtained by
// passing the value through getToStringValue first.
export function toString(value: ToStringValue): string {
// The coercion safety check is performed in getToStringValue().
// eslint-disable-next-line react-internal/safe-string-coercion
return '' + (value: any);
}
Expand Down
3 changes: 0 additions & 3 deletions packages/react-dom/src/client/inputValueTracking.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ function trackValueOnNode(node: any): ?ValueTracker {
if (__DEV__) {
checkFormFieldValueStringCoercion(node[valueField]);
}
// eslint-disable-next-line react-internal/safe-string-coercion
let currentValue = '' + node[valueField];

// if someone has already defined a value or Safari, then bail
Expand All @@ -85,7 +84,6 @@ function trackValueOnNode(node: any): ?ValueTracker {
if (__DEV__) {
checkFormFieldValueStringCoercion(value);
}
// eslint-disable-next-line react-internal/safe-string-coercion
currentValue = '' + value;
set.call(this, value);
},
Expand All @@ -106,7 +104,6 @@ function trackValueOnNode(node: any): ?ValueTracker {
if (__DEV__) {
checkFormFieldValueStringCoercion(value);
}
// eslint-disable-next-line react-internal/safe-string-coercion
currentValue = '' + value;
},
stopTracking() {
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/server/DOMMarkupOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export function createMarkupForProperty(name: string, value: mixed): string {
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
// eslint-disable-next-line react-internal/safe-string-coercion
value = '' + (value: any);
sanitizeURL(value);
}
Expand Down
15 changes: 2 additions & 13 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,6 @@ function pushStyle(
checkCSSPropertyStringCoercion(styleValue, styleName);
}
valueChunk = stringToChunk(
// eslint-disable-next-line react-internal/safe-string-coercion
escapeTextForBrowser(('' + styleValue).trim()),
);
} else {
Expand All @@ -335,15 +334,13 @@ function pushStyle(
) {
valueChunk = stringToChunk(styleValue + 'px'); // Presumes implicit 'px' suffix for unitless numbers
} else {
// eslint-disable-next-line react-internal/safe-string-coercion
valueChunk = stringToChunk('' + styleValue);
}
} else {
if (__DEV__) {
checkCSSPropertyStringCoercion(styleValue, styleName);
}
valueChunk = stringToChunk(
// eslint-disable-next-line react-internal/safe-string-coercion
escapeTextForBrowser(('' + styleValue).trim()),
);
}
Expand Down Expand Up @@ -494,7 +491,6 @@ function pushAttribute(
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
// eslint-disable-next-line react-internal/safe-string-coercion
value = '' + (value: any);
sanitizeURL(value);
}
Expand Down Expand Up @@ -579,7 +575,6 @@ function pushInnerHTML(
if (__DEV__) {
checkHtmlStringCoercion(html);
}
// eslint-disable-next-line react-internal/safe-string-coercion
target.push(stringToChunk('' + html));
}
}
Expand Down Expand Up @@ -771,7 +766,6 @@ function pushStartOption(
if (__DEV__) {
checkAttributeStringCoercion(value, 'value');
}
// eslint-disable-next-line react-internal/safe-string-coercion
stringValue = '' + value;
} else {
if (__DEV__) {
Expand All @@ -793,7 +787,6 @@ function pushStartOption(
if (__DEV__) {
checkAttributeStringCoercion(selectedValue[i], 'value');
}
// eslint-disable-next-line react-internal/safe-string-coercion
const v = '' + selectedValue[i];
if (v === stringValue) {
target.push(selectedMarkerAttribute);
Expand Down Expand Up @@ -1006,19 +999,16 @@ function pushStartTextArea(
children.length <= 1,
'<textarea> can only have at most one child.',
);
// TODO: remove the coercion and the DEV check below because it will
// always be overwritten by the coercion several lines below it. #22309
if (__DEV__) {
checkHtmlStringCoercion(children[0]);
}
// TODO: remove the coercion below because it will always be overwritten
// by the coercion several lines below it. #22309
// eslint-disable-next-line react-internal/safe-string-coercion
value = '' + children[0];
}
if (__DEV__) {
checkHtmlStringCoercion(children);
}
// TODO: `else` is missing here. Should fix in later PR.
// eslint-disable-next-line react-internal/safe-string-coercion
value = '' + children;
}

Expand Down Expand Up @@ -1287,7 +1277,6 @@ function pushStartPreformattedElement(
if (__DEV__) {
checkHtmlStringCoercion(html);
}
// eslint-disable-next-line react-internal/safe-string-coercion
target.push(stringToChunk('' + html));
}
}
Expand Down
6 changes: 0 additions & 6 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,6 @@ class ReactDOMServerRenderer {
parentNamespace: string,
): string {
if (typeof child === 'string' || typeof child === 'number') {
// eslint-disable-next-line react-internal/safe-string-coercion
const text = '' + child;
if (text === '') {
return '';
Expand Down Expand Up @@ -1480,7 +1479,6 @@ class ReactDOMServerRenderer {
if (__DEV__) {
checkPropStringCoercion(textareaChildren, 'children');
}
// eslint-disable-next-line react-internal/safe-string-coercion
defaultValue = '' + textareaChildren;
}
if (defaultValue == null) {
Expand All @@ -1494,7 +1492,6 @@ class ReactDOMServerRenderer {
}
props = Object.assign({}, props, {
value: undefined,
// eslint-disable-next-line react-internal/safe-string-coercion
children: '' + initialValue,
});
} else if (tag === 'select') {
Expand Down Expand Up @@ -1551,7 +1548,6 @@ class ReactDOMServerRenderer {
if (__DEV__) {
checkFormFieldValueStringCoercion(props.value);
}
// eslint-disable-next-line react-internal/safe-string-coercion
value = props.value + '';
} else {
if (__DEV__) {
Expand All @@ -1574,7 +1570,6 @@ class ReactDOMServerRenderer {
if (__DEV__) {
checkFormFieldValueStringCoercion(selectValue[j]);
}
// eslint-disable-next-line react-internal/safe-string-coercion
if ('' + selectValue[j] === value) {
selected = true;
break;
Expand All @@ -1584,7 +1579,6 @@ class ReactDOMServerRenderer {
if (__DEV__) {
checkFormFieldValueStringCoercion(selectValue);
}
// eslint-disable-next-line react-internal/safe-string-coercion
selected = '' + selectValue === value;
}

Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/src/server/escapeTextForBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ function escapeHtml(string) {
if (__DEV__) {
checkHtmlStringCoercion(string);
}
// eslint-disable-next-line react-internal/safe-string-coercion
const str = '' + string;
const match = matchHtmlRegExp.exec(str);

Expand Down Expand Up @@ -109,7 +108,6 @@ function escapeTextForBrowser(text) {
// this shortcircuit helps perf for types that we know will never have
// special characters, especially given that this function is used often
// for numeric dom ids.
// eslint-disable-next-line react-internal/safe-string-coercion
return '' + text;
}
return escapeHtml(text);
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/shared/dangerousStyleValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ function dangerousStyleValue(name, value, isCustomProperty) {
if (__DEV__) {
checkCSSPropertyStringCoercion(value, name);
}
// eslint-disable-next-line react-internal/safe-string-coercion
return ('' + value).trim();
}

Expand Down
1 change: 0 additions & 1 deletion packages/react-noop-renderer/flight-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const modules: Map<string, Function> = new Map();

// This simulates what the compiler will do when it replaces render functions with server blocks.
export function saveModule(render: Function): string {
// eslint-disable-next-line react-internal/safe-string-coercion
const idx = '' + moduleIdx++;
modules.set(idx, render);
return idx;
Expand Down
1 change: 0 additions & 1 deletion packages/react-noop-renderer/npm/flight-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ var modules = new Map();

// This simulates what the compiler will do when it replaces render functions with server blocks.
exports.saveModule = function saveModule(render) {
// eslint-disable-next-line react-internal/safe-string-coercion
var idx = '' + moduleIdx++;
modules.set(idx, render);
return idx;
Expand Down
8 changes: 3 additions & 5 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
parent: instance.parent,
children: keepChildren ? instance.children : [],
text: shouldSetTextContent(type, newProps)
? // eslint-disable-next-line react-internal/safe-string-coercion
computeText((newProps.children: any) + '', instance.context)
? computeText((newProps.children: any) + '', instance.context)
: null,
prop: newProps.prop,
hidden: !!newProps.hidden,
Expand Down Expand Up @@ -299,6 +298,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
throw new Error('Error in host config.');
}
if (__DEV__) {
// The `if` statement here prevents auto-disabling of the safe coercion
// ESLint rule, so we must manually disable it below.
if (shouldSetTextContent(type, props)) {
checkPropStringCoercion(props.children, 'children');
}
Expand Down Expand Up @@ -496,7 +497,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
checkPropStringCoercion(newProps.children, 'children');
}
instance.text = computeText(
// eslint-disable-next-line react-internal/safe-string-coercion
(newProps.children: any) + '',
instance.context,
);
Expand Down Expand Up @@ -950,7 +950,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
// TODO: Replace ReactNoop.render with createRoot + root.render
createRoot() {
const container = {
// eslint-disable-next-line react-internal/safe-string-coercion
rootID: '' + idCounter++,
pendingChildren: [],
children: [],
Expand Down Expand Up @@ -978,7 +977,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

createLegacyRoot() {
const container = {
// eslint-disable-next-line react-internal/safe-string-coercion
rootID: '' + idCounter++,
pendingChildren: [],
children: [],
Expand Down
Loading

0 comments on commit ab0e0c9

Please sign in to comment.