From c88fb49d37fd01024e0a254a37b7810d107bdd1d Mon Sep 17 00:00:00 2001 From: Justin Grant Date: Mon, 27 Sep 2021 10:05:07 -0700 Subject: [PATCH] Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) (#22064) * Revise ESLint rules for string coercion Currently, react uses `'' + value` to coerce mixed values to strings. This code will throw for Temporal objects or symbols. To make string-coercion safer and to improve user-facing error messages, This commit adds a new ESLint rule called `safe-string-coercion`. This rule has two modes: a production mode and a non-production mode. * If the `isProductionUserAppCode` option is true, then `'' + value` coercions are allowed (because they're faster, although they may throw) and `String(value)` coercions are disallowed. Exception: when building error messages or running DEV-only code in prod files, `String()` should be used because it won't throw. * If the `isProductionUserAppCode` option is false, then `'' + value` coercions are disallowed (because they may throw, and in non-prod code it's not worth the risk) and `String(value)` are allowed. Production mode is used for all files which will be bundled with developers' userland apps. Non-prod mode is used for all other React code: tests, DEV blocks, devtools extension, etc. In production mode, in addiiton to flagging `String(value)` calls, the rule will also flag `'' + value` or `value + ''` coercions that may throw. The rule is smart enough to silence itself in the following "will never throw" cases: * When the coercion is wrapped in a `typeof` test that restricts to safe (non-symbol, non-object) types. Example: if (typeof value === 'string' || typeof value === 'number') { thisWontReport('' + value); } * When what's being coerced is a unary function result, because unary functions never return an object or a symbol. * When the coerced value is a commonly-used numeric identifier: `i`, `idx`, or `lineNumber`. * When the statement immeidately before the coercion is a DEV-only call to a function from shared/CheckStringCoercion.js. This call is a no-op in production, but in DEV it will show a console error explaining the problem, then will throw right after a long explanatory code comment so that debugger users will have an idea what's going on. The check function call must be in the following format: if (__DEV__) { checkXxxxxStringCoercion(value); }; Manually disabling the rule is usually not necessary because almost all prod use of the `'' + value` pattern falls into one of the categories above. But in the rare cases where the rule isn't smart enough to detect safe usage (e.g. when a coercion is inside a nested ternary operator), manually disabling the rule will be needed. The rule should also be manually disabled in prod error handling code where `String(value)` should be used for coercions, because it'd be bad to throw while building an error message or stack trace! The prod and non-prod modes have differentiated error messages to explain how to do a proper coercion in that mode. If a production check call is needed but is missing or incorrect (e.g. not in a DEV block or not immediately before the coercion), then a context-sensitive error message will be reported so that developers can figure out what's wrong and how to fix the problem. Because string coercions are now handled by the `safe-string-coercion` rule, the `no-primitive-constructor` rule no longer flags `String()` usage. It still flags `new String(value)` because that usage is almost always a bug. * Add DEV-only string coercion check functions This commit adds DEV-only functions to check whether coercing values to strings using the `'' + value` pattern will throw. If it will throw, these functions will: 1. Display a console error with a friendly error message describing the problem and the developer can fix it. 2. Perform the coercion, which will throw. Right before the line where the throwing happens, there's a long code comment that will help debugger users (or others looking at the exception call stack) figure out what happened and how to fix the problem. One of these check functions should be called before all string coercion of user-provided values, except when the the coercion is guaranteed not to throw, e.g. * if inside a typeof check like `if (typeof value === 'string')` * if coercing the result of a unary function like `+value` or `value++` * if coercing a variable named in a whitelist of numeric identifiers: `i`, `idx`, or `lineNumber`. The new `safe-string-coercion` internal ESLint rule enforces that these check functions are called when they are required. Only use these check functions in production code that will be bundled with user apps. For non-prod code (and for production error-handling code), use `String(value)` instead which may be a little slower but will never throw. * Add failing tests for string coercion Added failing tests to verify: * That input, select, and textarea elements with value and defaultValue set to Temporal-like objects which will throw when coerced to string using the `'' + value` pattern. * That text elements will throw for Temporal-like objects * That dangerouslySetInnerHTML will *not* throw for Temporal-like objects because this value is not cast to a string before passing to the DOM. * That keys that are Temporal-like objects will throw All tests above validate the friendly error messages thrown. * Use `String(value)` for coercion in non-prod files This commit switches non-production code from `'' + value` (which throws for Temporal objects and symbols) to instead use `String(value)` which won't throw for these or other future plus-phobic types. "Non-produciton code" includes anything not bundled into user apps: * Tests and test utilities. Note that I didn't change legacy React test fixtures because I assumed it was good for those files to act just like old React, including coercion behavior. * Build scripts * Dev tools package - In addition to switching to `String`, I also removed special-case code for coercing symbols which is now unnecessary. * Add DEV-only string coercion checks to prod files This commit adds DEV-only function calls to to check if string coercion using `'' + value` will throw, which it will if the value is a Temporal object or a symbol because those types can't be added with `+`. If it will throw, then in DEV these checks will show a console error to help the user undertsand what went wrong and how to fix the problem. After emitting the console error, the check functions will retry the coercion which will throw with a call stack that's easy (or at least easier!) to troubleshoot because the exception happens right after a long comment explaining the issue. So whether the user is in a debugger, looking at the browser console, or viewing the in-browser DEV call stack, it should be easy to understand and fix the problem. In most cases, the safe-string-coercion ESLint rule is smart enough to detect when a coercion is safe. But in rare cases (e.g. when a coercion is inside a ternary) this rule will have to be manually disabled. This commit also switches error-handling code to use `String(value)` for coercion, because it's bad to crash when you're trying to build an error message or a call stack! Because `String()` is usually disallowed by the `safe-string-coercion` ESLint rule in production code, the rule must be disabled when `String()` is used. --- .eslintrc.js | 13 +- dangerfile.js | 4 +- .../fixtures/error-handling/index.js | 2 +- .../ReactHooksInspectionIntegration-test.js | 4 +- .../react-devtools-extensions/src/main.js | 2 +- .../src/backend/legacy/renderer.js | 2 +- .../src/backend/renderer.js | 2 +- .../src/backend/utils.js | 13 +- .../views/ErrorBoundary/ErrorBoundary.js | 2 +- .../src/devtools/views/utils.js | 4 +- packages/react-devtools-shared/src/hook.js | 12 +- .../src/hooks/SourceMapMetadataConsumer.js | 1 - packages/react-devtools-shared/src/utils.js | 2 +- .../src/__tests__/ReactDOMAttribute-test.js | 21 ++ .../src/__tests__/ReactDOMComponent-test.js | 48 ++- .../src/__tests__/ReactDOMInput-test.js | 98 ++++- .../src/__tests__/ReactDOMSelect-test.js | 264 ++++++++++++++ .../ReactDOMServerIntegrationHooks-test.js | 10 +- ...erIntegrationUntrustedURL-test.internal.js | 5 + .../__tests__/ReactDOMTextComponent-test.js | 22 ++ .../src/__tests__/ReactDOMTextarea-test.js | 32 +- .../src/__tests__/ReactIdentity-test.js | 29 ++ .../src/__tests__/ReactMultiChildText-test.js | 4 +- .../src/client/DOMPropertyOperations.js | 20 + .../react-dom/src/client/ReactDOMComponent.js | 4 + .../react-dom/src/client/ReactDOMInput.js | 4 + .../react-dom/src/client/ToStringValue.js | 10 +- .../src/client/inputValueTracking.js | 11 + .../src/server/DOMMarkupOperations.js | 4 + .../src/server/ReactDOMServerFormatConfig.js | 35 ++ .../src/server/ReactPartialRenderer.js | 19 + .../src/server/escapeTextForBrowser.js | 5 + .../src/shared/dangerousStyleValue.js | 4 + .../src/test-utils/ReactTestUtils.js | 2 +- .../src/createReactNoop.js | 17 +- .../src/ReactChildFiber.new.js | 4 + .../src/ReactChildFiber.old.js | 4 + packages/react-reconciler/src/ReactPortal.js | 4 + .../src/__tests__/ReactCache-test.js | 2 +- .../ReactHooksWithNoopRenderer-test.js | 16 +- .../src/__tests__/ReactIncremental-test.js | 4 +- .../src/__tests__/ReactMemo-test.js | 2 +- .../src/__tests__/ReactPersistent-test.js | 2 +- .../react-server/src/ReactFlightServer.js | 8 +- .../src/ReactTestRenderer.js | 4 + packages/react/src/ReactChildren.js | 16 +- packages/react/src/ReactElement.js | 19 + packages/react/src/jsx/ReactJSXElement.js | 13 + packages/shared/CheckStringCoercion.js | 167 +++++++++ packages/shared/consoleWithStackDev.js | 3 +- ...no-primitive-constructors-test.internal.js | 8 +- .../safe-string-coercion-test.internal.js | 265 ++++++++++++++ scripts/eslint-rules/index.js | 1 + .../eslint-rules/no-primitive-constructors.js | 13 +- scripts/eslint-rules/safe-string-coercion.js | 344 ++++++++++++++++++ scripts/flow/runFlow.js | 4 +- scripts/merge-fork/merge-fork.js | 4 +- scripts/rollup/build-all-release-channels.js | 8 +- 58 files changed, 1561 insertions(+), 85 deletions(-) create mode 100644 packages/shared/CheckStringCoercion.js create mode 100644 scripts/eslint-rules/__tests__/safe-string-coercion-test.internal.js create mode 100644 scripts/eslint-rules/safe-string-coercion.js diff --git a/.eslintrc.js b/.eslintrc.js index a155becfafb67..23d5ab76c32bc 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -108,6 +108,10 @@ module.exports = { // CUSTOM RULES // the second argument of warning/invariant should be a literal string 'react-internal/no-primitive-constructors': ERROR, + 'react-internal/safe-string-coercion': [ + ERROR, + {isProductionUserAppCode: true}, + ], 'react-internal/no-to-warn-dev-within-to-throw': ERROR, 'react-internal/invariant-args': ERROR, 'react-internal/warning-args': ERROR, @@ -168,10 +172,17 @@ module.exports = { 'packages/*/npm/**/*.js', 'packages/dom-event-testing-library/**/*.js', 'packages/react-devtools*/**/*.js', + 'dangerfile.js', + 'fixtures', + 'packages/react-dom/src/test-utils/*.js', ], rules: { 'react-internal/no-production-logging': OFF, 'react-internal/warning-args': OFF, + 'react-internal/safe-string-coercion': [ + ERROR, + {isProductionUserAppCode: false}, + ], // Disable accessibility checks 'jsx-a11y/aria-role': OFF, @@ -185,7 +196,7 @@ module.exports = { { files: [ 'scripts/eslint-rules/*.js', - 'packages/eslint-plugin-react-hooks/src/*.js' + 'packages/eslint-plugin-react-hooks/src/*.js', ], plugins: ['eslint-plugin'], rules: { diff --git a/dangerfile.js b/dangerfile.js index e6a6a82c87086..f612c80a7e545 100644 --- a/dangerfile.js +++ b/dangerfile.js @@ -102,8 +102,8 @@ function row(result) { let headSha; let baseSha; try { - headSha = (readFileSync(HEAD_DIR + '/COMMIT_SHA') + '').trim(); - baseSha = (readFileSync(BASE_DIR + '/COMMIT_SHA') + '').trim(); + headSha = String(readFileSync(HEAD_DIR + '/COMMIT_SHA')).trim(); + baseSha = String(readFileSync(BASE_DIR + '/COMMIT_SHA')).trim(); } catch { warn( "Failed to read build artifacts. It's possible a build configuration " + diff --git a/fixtures/dom/src/components/fixtures/error-handling/index.js b/fixtures/dom/src/components/fixtures/error-handling/index.js index 5bbeec7de447e..904bef4788c56 100644 --- a/fixtures/dom/src/components/fixtures/error-handling/index.js +++ b/fixtures/dom/src/components/fixtures/error-handling/index.js @@ -41,7 +41,7 @@ class ErrorBoundary extends React.Component { if (this.state.error) { return

Captured an error: {this.state.error.message}

; } else { - return

Captured an error: {'' + this.state.error}

; + return

Captured an error: {String(this.state.error)}

; } } if (this.state.shouldThrow) { diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js index b13bec22d213c..013caecb11ba0 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js @@ -615,7 +615,7 @@ describe('ReactHooksInspectionIntegration', () => { expect(tree[0].id).toEqual(0); expect(tree[0].isStateEditable).toEqual(false); expect(tree[0].name).toEqual('OpaqueIdentifier'); - expect((tree[0].value + '').startsWith('c_')).toBe(true); + expect(String(tree[0].value).startsWith('c_')).toBe(true); expect(tree[1]).toEqual({ id: 1, @@ -646,7 +646,7 @@ describe('ReactHooksInspectionIntegration', () => { expect(tree[0].id).toEqual(0); expect(tree[0].isStateEditable).toEqual(false); expect(tree[0].name).toEqual('OpaqueIdentifier'); - expect((tree[0].value + '').startsWith('c_')).toBe(true); + expect(String(tree[0].value).startsWith('c_')).toBe(true); expect(tree[1]).toEqual({ id: 1, diff --git a/packages/react-devtools-extensions/src/main.js b/packages/react-devtools-extensions/src/main.js index 9d5b4da02e409..eb093fc51b785 100644 --- a/packages/react-devtools-extensions/src/main.js +++ b/packages/react-devtools-extensions/src/main.js @@ -94,7 +94,7 @@ function createPanelIfReactLoaded() { function initBridgeAndStore() { const port = chrome.runtime.connect({ - name: '' + tabId, + name: String(tabId), }); // Looks like `port.onDisconnect` does not trigger on in-tab navigation like new URL or back/forward navigation, // so it makes no sense to handle it here. diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 24f30b87bc78a..1f0aed404087a 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -63,7 +63,7 @@ function getData(internalInstance: InternalInstance) { // != used deliberately here to catch undefined and null if (internalInstance._currentElement != null) { if (internalInstance._currentElement.key) { - key = '' + internalInstance._currentElement.key; + key = String(internalInstance._currentElement.key); } const elementType = internalInstance._currentElement.type; diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 78953e72979a4..24a00583e7e68 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1848,7 +1848,7 @@ export function attach( // This check is a guard to handle a React element that has been modified // in such a way as to bypass the default stringification of the "key" property. - const keyString = key === null ? null : '' + key; + const keyString = key === null ? null : String(key); const keyStringID = getStringID(keyString); pushOperation(TREE_OPERATION_ADD); diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index 4b40645595d1c..09f821f4e1877 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -166,11 +166,7 @@ export function format( ): string { const args = inputArgs.slice(); - // Symbols cannot be concatenated with Strings. - let formatted: string = - typeof maybeMessage === 'symbol' - ? maybeMessage.toString() - : '' + maybeMessage; + let formatted: string = String(maybeMessage); // If the first argument is a string, check for substitutions. if (typeof maybeMessage === 'string') { @@ -203,17 +199,14 @@ export function format( // Arguments that remain after formatting. if (args.length) { for (let i = 0; i < args.length; i++) { - const arg = args[i]; - - // Symbols cannot be concatenated with Strings. - formatted += ' ' + (typeof arg === 'symbol' ? arg.toString() : arg); + formatted += ' ' + String(args[i]); } } // Update escaped %% values. formatted = formatted.replace(/%{2,2}/g, '%'); - return '' + formatted; + return String(formatted); } export function isSynchronousXHRSupported(): boolean { diff --git a/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js b/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js index c7aa20dcd5902..ea4f54b1a9b8b 100644 --- a/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js +++ b/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js @@ -46,7 +46,7 @@ export default class ErrorBoundary extends Component { error !== null && error.hasOwnProperty('message') ? error.message - : '' + error; + : String(error); const callStack = typeof error === 'object' && diff --git a/packages/react-devtools-shared/src/devtools/views/utils.js b/packages/react-devtools-shared/src/devtools/views/utils.js index b326996daa05a..035d9f83a3bc5 100644 --- a/packages/react-devtools-shared/src/devtools/views/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/utils.js @@ -19,8 +19,8 @@ export function alphaSortEntries( ): number { const a = entryA[0]; const b = entryB[0]; - if ('' + +a === a) { - if ('' + +b !== b) { + if (String(+a) === a) { + if (String(+b) !== b) { return -1; } return +a < +b ? -1 : 1; diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 152b1dce8f8e4..6f34f86132ab6 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -180,10 +180,7 @@ export function installHook(target: any): DevToolsHook | null { const args = inputArgs.slice(); // Symbols cannot be concatenated with Strings. - let formatted: string = - typeof maybeMessage === 'symbol' - ? maybeMessage.toString() - : '' + maybeMessage; + let formatted = String(maybeMessage); // If the first argument is a string, check for substitutions. if (typeof maybeMessage === 'string') { @@ -216,17 +213,14 @@ export function installHook(target: any): DevToolsHook | null { // Arguments that remain after formatting. if (args.length) { for (let i = 0; i < args.length; i++) { - const arg = args[i]; - - // Symbols cannot be concatenated with Strings. - formatted += ' ' + (typeof arg === 'symbol' ? arg.toString() : arg); + formatted += ' ' + String(args[i]); } } // Update escaped %% values. formatted = formatted.replace(/%{2,2}/g, '%'); - return '' + formatted; + return String(formatted); } let unpatchFn = null; diff --git a/packages/react-devtools-shared/src/hooks/SourceMapMetadataConsumer.js b/packages/react-devtools-shared/src/hooks/SourceMapMetadataConsumer.js index 250192c7fa4b3..4882afba9a9fb 100644 --- a/packages/react-devtools-shared/src/hooks/SourceMapMetadataConsumer.js +++ b/packages/react-devtools-shared/src/hooks/SourceMapMetadataConsumer.js @@ -38,7 +38,6 @@ function normalizeSourcePath( const {sourceRoot} = map; let source = sourceInput; - // eslint-disable-next-line react-internal/no-primitive-constructors source = String(source); // Some source maps produce relative source paths like "./foo.js" instead of // "foo.js". Normalize these first so that future comparisons will succeed. diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 7c9e2fbd25f8a..9ec19e3afde41 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -834,7 +834,7 @@ export function formatDataForPreview( return data; default: try { - return truncateForDisplay('' + data); + return truncateForDisplay(String(data)); } catch (error) { return 'unserializable'; } diff --git a/packages/react-dom/src/__tests__/ReactDOMAttribute-test.js b/packages/react-dom/src/__tests__/ReactDOMAttribute-test.js index aa91857b48009..e240708667f21 100644 --- a/packages/react-dom/src/__tests__/ReactDOMAttribute-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMAttribute-test.js @@ -89,6 +89,27 @@ describe('ReactDOM unknown attribute', () => { testUnknownAttributeAssignment(lol, 'lol'); }); + it('throws with Temporal-like objects', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const test = () => + testUnknownAttributeAssignment(new TemporalLike(), null); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Warning: The provided `unknown` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + it('removes symbols and warns', () => { expect(() => testUnknownAttributeRemoval(Symbol('foo'))).toErrorDev( 'Warning: Invalid value for prop `unknown` on
tag. Either remove it ' + diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index 2911099e8e932..fb7d7cf41ada3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -254,6 +254,28 @@ describe('ReactDOMComponent', () => { ReactDOM.render(, div); }); + it('throws with Temporal-like objects as style values', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const style = {fontSize: new TemporalLike()}; + const div = document.createElement('div'); + const test = () => ReactDOM.render(, div); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Warning: The provided `fontSize` CSS property is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + it('should update styles if initially null', () => { let styles = null; const container = document.createElement('div'); @@ -1130,7 +1152,7 @@ describe('ReactDOMComponent', () => { describe('createOpenTagMarkup', () => { function quoteRegexp(str) { - return (str + '').replace(/([.?*+\^$\[\]\\(){}|-])/g, '\\$1'); + return String(str).replace(/([.?*+\^$\[\]\\(){}|-])/g, '\\$1'); } function expectToHaveAttribute(actual, expected) { @@ -1164,7 +1186,7 @@ describe('ReactDOMComponent', () => { describe('createContentMarkup', () => { function quoteRegexp(str) { - return (str + '').replace(/([.?*+\^$\[\]\\(){}|-])/g, '\\$1'); + return String(str).replace(/([.?*+\^$\[\]\\(){}|-])/g, '\\$1'); } function genMarkup(props) { @@ -2412,6 +2434,28 @@ describe('ReactDOMComponent', () => { expect(el.getAttribute('whatever')).toBe('[object Object]'); }); + it('allows Temporal-like objects as HTML (they are not coerced to strings first)', function() { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + + // `dangerouslySetInnerHTML` is never coerced to a string, so won't throw + // even with a Temporal-like object. + const container = document.createElement('div'); + ReactDOM.render( +
, + container, + ); + expect(container.firstChild.innerHTML).toEqual('2020-01-01'); + }); + it('allows cased data attributes', function() { let el; expect(() => { diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 018dd14697f08..fb2c4bbc380c5 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -544,6 +544,102 @@ describe('ReactDOMInput', () => { expect(node.value).toBe('foobar'); }); + it('should throw for date inputs if `defaultValue` is an object where valueOf() throws', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const test = () => + ReactDOM.render( + , + container, + ); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props) must be ' + + 'strings, not TemporalLike. This value must be coerced to a string before before using it here.', + ); + }); + + it('should throw for text inputs if `defaultValue` is an object where valueOf() throws', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const test = () => + ReactDOM.render( + , + container, + ); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props) must be ' + + 'strings, not TemporalLike. This value must be coerced to a string before before using it here.', + ); + }); + + it('should throw for date inputs if `value` is an object where valueOf() throws', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const test = () => + ReactDOM.render( + {}} />, + container, + ); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props) must be ' + + 'strings, not TemporalLike. This value must be coerced to a string before before using it here.', + ); + }); + + it('should throw for text inputs if `value` is an object where valueOf() throws', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const test = () => + ReactDOM.render( + {}} />, + container, + ); + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props) must be ' + + 'strings, not TemporalLike. This value must be coerced to a string before before using it here.', + ); + }); + it('should display `value` of number 0', () => { const stub = ; const node = ReactDOM.render(stub, container); @@ -1575,7 +1671,7 @@ describe('ReactDOMInput', () => { return value; }, set: function(val) { - value = '' + val; + value = String(val); log.push('set property value'); }, }); diff --git a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js index 3368f09f585ac..08353e11da70c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js @@ -1019,4 +1019,268 @@ describe('ReactDOMSelect', () => { expect(node.value).toBe(''); }); }); + + describe('When given a Temporal.PlainDate-like value', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + + it('throws when given a Temporal.PlainDate-like value (select)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props)' + + ' must be strings, not TemporalLike. ' + + 'This value must be coerced to a string before before using it here.', + ); + }); + + it('throws when given a Temporal.PlainDate-like value (option)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws when given a Temporal.PlainDate-like value (both)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws with updated Temporal.PlainDate-like value (select)', () => { + ReactTestUtils.renderIntoDocument( + , + ); + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props)' + + ' must be strings, not TemporalLike. ' + + 'This value must be coerced to a string before before using it here.', + ); + }); + + it('throws with updated Temporal.PlainDate-like value (option)', () => { + ReactTestUtils.renderIntoDocument( + , + ); + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws with updated Temporal.PlainDate-like value (both)', () => { + ReactTestUtils.renderIntoDocument( + , + ); + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws when given a Temporal.PlainDate-like defaultValue (select)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props)' + + ' must be strings, not TemporalLike. ' + + 'This value must be coerced to a string before before using it here.', + ); + }); + + it('throws when given a Temporal.PlainDate-like defaultValue (option)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws when given a Temporal.PlainDate-like value (both)', () => { + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + + it('throws with updated Temporal.PlainDate-like defaultValue (select)', () => { + ReactTestUtils.renderIntoDocument( + , + ); + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'Form field values (value, checked, defaultValue, or defaultChecked props)' + + ' must be strings, not TemporalLike. ' + + 'This value must be coerced to a string before before using it here.', + ); + }); + + it('throws with updated Temporal.PlainDate-like defaultValue (both)', () => { + ReactTestUtils.renderIntoDocument( + , + ); + const test = () => { + ReactTestUtils.renderIntoDocument( + , + ); + }; + expect(() => + expect(test).toThrowError(new TypeError('prod message')), + ).toErrorDev( + 'The provided `value` attribute is an unsupported type TemporalLike.' + + ' This value must be coerced to a string before before using it here.', + ); + }); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 53a34b25ffd55..a27954785f7ea 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -1745,7 +1745,7 @@ describe('ReactDOMServerHooks', () => { it('useOpaqueIdentifier warns when there is a hydration error and we are using ID as a string', async () => { function Child({appId}) { - return
; + return
; } function App() { const id = useOpaqueIdentifier(); @@ -1769,7 +1769,7 @@ describe('ReactDOMServerHooks', () => { it('useOpaqueIdentifier warns when there is a hydration error and we are using ID as a string', async () => { function Child({appId}) { - return
; + return
; } function App() { const id = useOpaqueIdentifier(); @@ -1793,7 +1793,7 @@ describe('ReactDOMServerHooks', () => { it('useOpaqueIdentifier warns if you try to use the result as a string in a child component', async () => { function Child({appId}) { - return
; + return
; } function App() { const id = useOpaqueIdentifier(); @@ -1817,7 +1817,7 @@ describe('ReactDOMServerHooks', () => { it('useOpaqueIdentifier warns if you try to use the result as a string', async () => { function App() { const id = useOpaqueIdentifier(); - return
; + return
; } const container = document.createElement('div'); @@ -1836,7 +1836,7 @@ describe('ReactDOMServerHooks', () => { it('useOpaqueIdentifier warns if you try to use the result as a string in a child component wrapped in a Suspense', async () => { function Child({appId}) { - return
; + return
; } function App() { const id = useOpaqueIdentifier(); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.internal.js index 9fb48b3bce24c..72291c9959ee0 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.internal.js @@ -242,6 +242,11 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', ( // consistency but the code structure makes that hard right now. expectedToStringCalls = 2; } + if (__DEV__) { + // Checking for string coercion problems results in double the + // toString calls in DEV + expectedToStringCalls *= 2; + } let toStringCalls = 0; const firstIsSafe = { diff --git a/packages/react-dom/src/__tests__/ReactDOMTextComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMTextComponent-test.js index ddc3a979597e3..24ff139085d96 100644 --- a/packages/react-dom/src/__tests__/ReactDOMTextComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMTextComponent-test.js @@ -284,4 +284,26 @@ describe('ReactDOMTextComponent', () => { ReactDOM.render(
, el); expect(el.innerHTML).toBe('
'); }); + + it('throws for Temporal-like text nodes', () => { + const el = document.createElement('div'); + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + expect(() => + ReactDOM.render(
{new TemporalLike()}
, el), + ).toThrowError( + new Error( + 'Objects are not valid as a React child (found: object with keys {}).' + + ' If you meant to render a collection of children, use an array instead.', + ), + ); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js index 4d56f53d9e40c..f8bec6c4ba97d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js @@ -142,7 +142,7 @@ describe('ReactDOMTextarea', () => { return value; }, set: function(val) { - value = '' + val; + value = String(val); counter++; }, }); @@ -219,6 +219,36 @@ describe('ReactDOMTextarea', () => { expect(node.value).toEqual('foo'); }); + it('should throw when value is set to a Temporal-like object', () => { + class TemporalLike { + valueOf() { + // Throwing here is the behavior of ECMAScript "Temporal" date/time API. + // See https://tc39.es/proposal-temporal/docs/plaindate.html#valueOf + throw new TypeError('prod message'); + } + toString() { + return '2020-01-01'; + } + } + const container = document.createElement('div'); + const stub =