From 06478a670008c003e853888b608aa58eb9ae0a09 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 12 Dec 2019 17:39:44 +0000 Subject: [PATCH] Replace console.warn/error with a custom wrapper at build time --- .eslintrc.js | 15 ++- .../legacy-events/ResponderEventPlugin.js | 8 +- .../ResponderTouchHistoryStore.js | 30 +++-- packages/react-dom/src/client/ReactDOM.js | 1 + .../src/test-utils/ReactTestUtilsAct.js | 1 + .../src/dom/testing-library/domEnvironment.js | 4 +- packages/react-is/src/ReactIs.js | 3 +- .../src/NativeMethodsMixinUtils.js | 24 ++-- .../src/createReactNoop.js | 3 + .../src/ReactFiberErrorLogger.js | 10 +- .../src/ReactTestRendererAct.js | 1 + .../__tests__/ReactClassEquivalence-test.js | 3 +- packages/scheduler/src/SchedulerProfiling.js | 3 +- .../src/__tests__/SchedulerProfiling-test.js | 2 +- .../src/forks/SchedulerHostConfig.default.js | 9 +- packages/shared/consoleWithStackDev.js | 63 ++++++++++ packages/shared/enqueueTask.js | 4 +- .../shared/forks/consoleWithStackDev.www.js | 22 ++++ .../lowPriorityWarningWithoutStack.www.js | 13 -- .../shared/forks/warningWithoutStack.www.js | 13 -- packages/shared/lowPriorityWarning.js | 43 ------- .../shared/lowPriorityWarningWithoutStack.js | 51 -------- packages/shared/warning.js | 43 ------- packages/shared/warningWithoutStack.js | 50 -------- .../babel/transform-replace-console-calls.js | 66 +++++++++++ .../no-production-logging-test.internal.js | 111 ++++++++++-------- .../__tests__/warning-args-test.internal.js | 39 +++--- scripts/eslint-rules/no-production-logging.js | 45 ++++--- scripts/eslint-rules/warning-args.js | 21 ++-- scripts/jest/preprocessor.js | 21 ++-- scripts/print-warnings/print-warnings.js | 17 +-- scripts/rollup/build.js | 26 +++- scripts/rollup/forks.js | 23 +--- scripts/shared/evalToString.js | 1 + 34 files changed, 407 insertions(+), 382 deletions(-) create mode 100644 packages/shared/consoleWithStackDev.js create mode 100644 packages/shared/forks/consoleWithStackDev.www.js delete mode 100644 packages/shared/forks/lowPriorityWarningWithoutStack.www.js delete mode 100644 packages/shared/forks/warningWithoutStack.www.js delete mode 100644 packages/shared/lowPriorityWarning.js delete mode 100644 packages/shared/lowPriorityWarningWithoutStack.js delete mode 100644 packages/shared/warning.js delete mode 100644 packages/shared/warningWithoutStack.js create mode 100644 scripts/babel/transform-replace-console-calls.js diff --git a/.eslintrc.js b/.eslintrc.js index fd7aec7db8bd..82115d52504e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -33,7 +33,8 @@ module.exports = { 'comma-dangle': [ERROR, 'always-multiline'], 'consistent-return': OFF, 'dot-location': [ERROR, 'property'], - 'dot-notation': ERROR, + // We use console['error']() as a signal to not transform it: + 'dot-notation': [ERROR, {allowPattern: '^(error|warn)$'}], 'eol-last': ERROR, eqeqeq: [ERROR, 'allow-null'], indent: OFF, @@ -135,6 +136,18 @@ module.exports = { 'jest/valid-expect-in-promise': ERROR, }, }, + { + files: [ + '**/__tests__/**/*.js', + 'scripts/**/*.js', + 'packages/*/npm/**/*.js', + 'packages/react-devtools*/**/*.js' + ], + rules: { + 'react-internal/no-production-logging': OFF, + 'react-internal/warning-args': OFF, + }, + }, { files: ['packages/react-native-renderer/**/*.js'], globals: { diff --git a/packages/legacy-events/ResponderEventPlugin.js b/packages/legacy-events/ResponderEventPlugin.js index f58323aa9a8b..ff736b8aeac0 100644 --- a/packages/legacy-events/ResponderEventPlugin.js +++ b/packages/legacy-events/ResponderEventPlugin.js @@ -515,9 +515,11 @@ const ResponderEventPlugin = { if (trackedTouchCount >= 0) { trackedTouchCount -= 1; } else { - console.warn( - 'Ended a touch event which was not counted in `trackedTouchCount`.', - ); + if (__DEV__) { + console.warn( + 'Ended a touch event which was not counted in `trackedTouchCount`.', + ); + } return null; } } diff --git a/packages/legacy-events/ResponderTouchHistoryStore.js b/packages/legacy-events/ResponderTouchHistoryStore.js index 79479740899f..1706f9af17fa 100644 --- a/packages/legacy-events/ResponderTouchHistoryStore.js +++ b/packages/legacy-events/ResponderTouchHistoryStore.js @@ -129,12 +129,15 @@ function recordTouchMove(touch: Touch): void { touchRecord.currentTimeStamp = timestampForTouch(touch); touchHistory.mostRecentTimeStamp = timestampForTouch(touch); } else { - console.warn( - 'Cannot record touch move without a touch start.\n' + 'Touch Move: %s\n', - 'Touch Bank: %s', - printTouch(touch), - printTouchBank(), - ); + if (__DEV__) { + console.warn( + 'Cannot record touch move without a touch start.\n' + + 'Touch Move: %s\n' + + 'Touch Bank: %s', + printTouch(touch), + printTouchBank(), + ); + } } } @@ -150,12 +153,15 @@ function recordTouchEnd(touch: Touch): void { touchRecord.currentTimeStamp = timestampForTouch(touch); touchHistory.mostRecentTimeStamp = timestampForTouch(touch); } else { - console.warn( - 'Cannot record touch end without a touch start.\n' + 'Touch End: %s\n', - 'Touch Bank: %s', - printTouch(touch), - printTouchBank(), - ); + if (__DEV__) { + console.warn( + 'Cannot record touch end without a touch start.\n' + + 'Touch End: %s\n' + + 'Touch Bank: %s', + printTouch(touch), + printTouchBank(), + ); + } } } diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 0c7649270ef9..818d0eef3a94 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -211,6 +211,7 @@ if (__DEV__) { const protocol = window.location.protocol; // Don't warn in exotic cases like chrome-extension://. if (/^(https?|file):$/.test(protocol)) { + // eslint-disable-next-line react-internal/no-production-logging console.info( '%cDownload the React DevTools ' + 'for a better development experience: ' + diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js index 4753c084ff94..7aa5b916820d 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js @@ -79,6 +79,7 @@ function act(callback: () => Thenable) { if (!__DEV__) { if (didWarnAboutUsingActInProd === false) { didWarnAboutUsingActInProd = true; + // eslint-disable-next-line react-internal/no-production-logging console.error( 'act(...) is not supported in production builds of React, and might not behave as expected.', ); diff --git a/packages/react-interactions/events/src/dom/testing-library/domEnvironment.js b/packages/react-interactions/events/src/dom/testing-library/domEnvironment.js index 5042f3e81eee..2c87c783e7cc 100644 --- a/packages/react-interactions/events/src/dom/testing-library/domEnvironment.js +++ b/packages/react-interactions/events/src/dom/testing-library/domEnvironment.js @@ -22,7 +22,9 @@ export function hasPointerEvent() { export function setPointerEvent(bool) { const pointerCaptureFn = name => id => { if (typeof id !== 'number') { - console.error(`A pointerId must be passed to "${name}"`); + if (__DEV__) { + console.error('A pointerId must be passed to "%s"', name); + } } }; global.PointerEvent = bool ? emptyFunction : undefined; diff --git a/packages/react-is/src/ReactIs.js b/packages/react-is/src/ReactIs.js index b6ee4f592e7e..99d457599a59 100644 --- a/packages/react-is/src/ReactIs.js +++ b/packages/react-is/src/ReactIs.js @@ -25,7 +25,6 @@ import { REACT_SUSPENSE_TYPE, } from 'shared/ReactSymbols'; import isValidElementType from 'shared/isValidElementType'; -import lowPriorityWarningWithoutStack from 'shared/lowPriorityWarningWithoutStack'; export function typeOf(object: any) { if (typeof object === 'object' && object !== null) { @@ -88,7 +87,7 @@ export function isAsyncMode(object: any) { if (__DEV__) { if (!hasWarnedAboutDeprecatedIsAsyncMode) { hasWarnedAboutDeprecatedIsAsyncMode = true; - lowPriorityWarningWithoutStack( + console.warn( 'The ReactIs.isAsyncMode() alias has been deprecated, ' + 'and will be removed in React 17+. Update your code to use ' + 'ReactIs.isConcurrentMode() instead. It has the exact same API.', diff --git a/packages/react-native-renderer/src/NativeMethodsMixinUtils.js b/packages/react-native-renderer/src/NativeMethodsMixinUtils.js index 639b39c67db3..1f6611523fbd 100644 --- a/packages/react-native-renderer/src/NativeMethodsMixinUtils.js +++ b/packages/react-native-renderer/src/NativeMethodsMixinUtils.js @@ -66,17 +66,19 @@ export function throwOnStylesProp(component: any, props: any) { } export function warnForStyleProps(props: any, validAttributes: any) { - for (const key in validAttributes.style) { - if (!(validAttributes[key] || props[key] === undefined)) { - console.error( - 'You are setting the style `{ ' + - key + - ': ... }` as a prop. You ' + - 'should nest it in a style object. ' + - 'E.g. `{ style: { ' + - key + - ': ... } }`', - ); + if (__DEV__) { + for (const key in validAttributes.style) { + if (!(validAttributes[key] || props[key] === undefined)) { + console.error( + 'You are setting the style `{ %s' + + ': ... }` as a prop. You ' + + 'should nest it in a style object. ' + + 'E.g. `{ style: { %s' + + ': ... } }`', + key, + key, + ); + } } } } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index cab5180da745..a21febff88f0 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -638,6 +638,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (!__DEV__) { if (didWarnAboutUsingActInProd === false) { didWarnAboutUsingActInProd = true; + // eslint-disable-next-line react-internal/no-production-logging console.error( 'act(...) is not supported in production builds of React, and might not behave as expected.', ); @@ -1106,6 +1107,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { const root = roots.get(rootID); const rootContainer = rootContainers.get(rootID); if (!root || !rootContainer) { + // eslint-disable-next-line react-internal/no-production-logging console.log('Nothing rendered yet.'); return; } @@ -1208,6 +1210,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { log('FIBERS:'); logFiber(root.current, 0); + // eslint-disable-next-line react-internal/no-production-logging console.log(...bufferedLog); }, diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index 758c22720a33..4ca2e5e9b310 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -20,9 +20,6 @@ export function logCapturedError(capturedError: CapturedError): void { return; } - // Prevent it from being seen by Babel transform. - const rawConsoleError = console.error; - const error = (capturedError.error: any); if (__DEV__) { const { @@ -47,7 +44,7 @@ export function logCapturedError(capturedError: CapturedError): void { // been accidental, we'll surface it anyway. // However, the browser would have silenced the original error // so we'll print it first, and then print the stack addendum. - rawConsoleError(error); + console['error'](error); // Don't transform to our wrapper // For a more detailed description of this block, see: // https://github.com/facebook/react/pull/13384 } @@ -81,11 +78,12 @@ export function logCapturedError(capturedError: CapturedError): void { // We don't include the original error message and JS stack because the browser // has already printed it. Even if the application swallows the error, it is still // displayed by the browser thanks to the DEV-only fake event trick in ReactErrorUtils. - rawConsoleError(combinedMessage); + console['error'](combinedMessage); // Don't transform to our wrapper } else { // In production, we print the error directly. // This will include the message, the JS stack, and anything the browser wants to show. // We pass the error object instead of custom message so that the browser displays the error natively. - rawConsoleError(error); + // eslint-disable-next-line react-internal/no-production-logging + console['error'](error); // Don't transform to our wrapper } } diff --git a/packages/react-test-renderer/src/ReactTestRendererAct.js b/packages/react-test-renderer/src/ReactTestRendererAct.js index 4cf12812226f..362de4e2b22a 100644 --- a/packages/react-test-renderer/src/ReactTestRendererAct.js +++ b/packages/react-test-renderer/src/ReactTestRendererAct.js @@ -60,6 +60,7 @@ function act(callback: () => Thenable) { if (!__DEV__) { if (didWarnAboutUsingActInProd === false) { didWarnAboutUsingActInProd = true; + // eslint-disable-next-line react-internal/no-production-logging console.error( 'act(...) is not supported in production builds of React, and might not behave as expected.', ); diff --git a/packages/react/src/__tests__/ReactClassEquivalence-test.js b/packages/react/src/__tests__/ReactClassEquivalence-test.js index 963403e53519..14352f71ca0d 100644 --- a/packages/react/src/__tests__/ReactClassEquivalence-test.js +++ b/packages/react/src/__tests__/ReactClassEquivalence-test.js @@ -28,7 +28,8 @@ describe('ReactClassEquivalence', () => { function runJest(testFile) { const cwd = process.cwd(); const extension = process.platform === 'win32' ? '.cmd' : ''; - const result = spawnSync('yarn' + extension, ['test', testFile], { + const command = __DEV__ ? 'test' : 'test-prod'; + const result = spawnSync('yarn' + extension, [command, testFile], { cwd, env: Object.assign({}, process.env, { REACT_CLASS_EQUIVALENCE_TEST: 'true', diff --git a/packages/scheduler/src/SchedulerProfiling.js b/packages/scheduler/src/SchedulerProfiling.js index b2e7781101a7..41c6003942c3 100644 --- a/packages/scheduler/src/SchedulerProfiling.js +++ b/packages/scheduler/src/SchedulerProfiling.js @@ -69,7 +69,8 @@ function logEvent(entries) { if (eventLogIndex + 1 > eventLogSize) { eventLogSize *= 2; if (eventLogSize > MAX_EVENT_LOG_SIZE) { - console.error( + // Using console['error'] to evade Babel and ESLint + console['error']( "Scheduler Profiling: Event log exceeded maximum size. Don't " + 'forget to call `stopLoggingProfilingEvents()`.', ); diff --git a/packages/scheduler/src/__tests__/SchedulerProfiling-test.js b/packages/scheduler/src/__tests__/SchedulerProfiling-test.js index df1a3a8f9b38..cf9f9c5ea8cd 100644 --- a/packages/scheduler/src/__tests__/SchedulerProfiling-test.js +++ b/packages/scheduler/src/__tests__/SchedulerProfiling-test.js @@ -544,7 +544,7 @@ Task 1 [Normal] │ █████████ } expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.error.calls.argsFor(0)[0]).toBe( "Scheduler Profiling: Event log exceeded maximum size. Don't forget " + 'to call `stopLoggingProfilingEvents()`.', ); diff --git a/packages/scheduler/src/forks/SchedulerHostConfig.default.js b/packages/scheduler/src/forks/SchedulerHostConfig.default.js index c5b28c84bbf0..00e3ff745c59 100644 --- a/packages/scheduler/src/forks/SchedulerHostConfig.default.js +++ b/packages/scheduler/src/forks/SchedulerHostConfig.default.js @@ -81,14 +81,16 @@ if ( const cancelAnimationFrame = window.cancelAnimationFrame; // TODO: Remove fb.me link if (typeof requestAnimationFrame !== 'function') { - console.error( + // Using console['error'] to evade Babel and ESLint + console['error']( "This browser doesn't support requestAnimationFrame. " + 'Make sure that you load a ' + 'polyfill in older browsers. https://fb.me/react-polyfills', ); } if (typeof cancelAnimationFrame !== 'function') { - console.error( + // Using console['error'] to evade Babel and ESLint + console['error']( "This browser doesn't support cancelAnimationFrame. " + 'Make sure that you load a ' + 'polyfill in older browsers. https://fb.me/react-polyfills', @@ -169,7 +171,8 @@ if ( forceFrameRate = function(fps) { if (fps < 0 || fps > 125) { - console.error( + // Using console['error'] to evade Babel and ESLint + console['error']( 'forceFrameRate takes a positive int between 0 and 125, ' + 'forcing framerates higher than 125 fps is not unsupported', ); diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js new file mode 100644 index 000000000000..22bbfee86766 --- /dev/null +++ b/packages/shared/consoleWithStackDev.js @@ -0,0 +1,63 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import ReactSharedInternals from 'shared/ReactSharedInternals'; + +// In DEV, calls to console.warn and console.error get replaced +// by calls to these methods by a Babel plugin. +// +// In PROD (or in packages without access to React internals), +// they are left as they are instead. + +export function warn(format, ...args) { + if (__DEV__) { + printWarning('warn', format, args); + } +} + +export function error(format, ...args) { + if (__DEV__) { + printWarning('error', format, args); + } +} + +function printWarning(level, format, args) { + if (__DEV__) { + const hasExistingStack = + args.length > 0 && + typeof args[args.length - 1] === 'string' && + args[args.length - 1].indexOf('\n in') === 0; + + if (!hasExistingStack) { + const ReactDebugCurrentFrame = + ReactSharedInternals.ReactDebugCurrentFrame; + const stack = ReactDebugCurrentFrame.getStackAddendum(); + if (stack !== '') { + format += '%s'; + args = args.concat([stack]); + } + } + + const argsWithFormat = args.map(item => '' + item); + // Careful: RN currently depends on this prefix + argsWithFormat.unshift('Warning: ' + format); + // We intentionally don't use spread (or .apply) directly because it + // breaks IE9: https://github.com/facebook/react/issues/13610 + // eslint-disable-next-line react-internal/no-production-logging + Function.prototype.apply.call(console[level], console, argsWithFormat); + + try { + // --- Welcome to debugging React --- + // This error was thrown as a convenience so that you can use this stack + // to find the callsite that caused this warning to fire. + let argIndex = 0; + const message = + 'Warning: ' + format.replace(/%s/g, () => args[argIndex++]); + throw new Error(message); + } catch (x) {} + } +} diff --git a/packages/shared/enqueueTask.js b/packages/shared/enqueueTask.js index ec2e53d0ae8c..5bf23c18f3a0 100644 --- a/packages/shared/enqueueTask.js +++ b/packages/shared/enqueueTask.js @@ -7,8 +7,6 @@ * @flow */ -import warningWithoutStack from 'shared/warningWithoutStack'; - let didWarnAboutMessageChannel = false; let enqueueTask; try { @@ -28,7 +26,7 @@ try { if (didWarnAboutMessageChannel === false) { didWarnAboutMessageChannel = true; if (typeof MessageChannel === 'undefined') { - warningWithoutStack( + console.error( 'This browser does not have a MessageChannel implementation, ' + 'so enqueuing tasks via await act(async () => ...) will fail. ' + 'Please file an issue at https://github.com/facebook/react/issues ' + diff --git a/packages/shared/forks/consoleWithStackDev.www.js b/packages/shared/forks/consoleWithStackDev.www.js new file mode 100644 index 000000000000..09c8f7fba7f7 --- /dev/null +++ b/packages/shared/forks/consoleWithStackDev.www.js @@ -0,0 +1,22 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// This refers to a WWW module. +const warningWWW = require('warning'); + +export function warn() { + // TODO: use different level for "warn". + const args = Array.prototype.slice.call(arguments); + args.unshift(false); + warningWWW.apply(null, args); +} + +export function error() { + const args = Array.prototype.slice.call(arguments); + args.unshift(false); + warningWWW.apply(null, args); +} diff --git a/packages/shared/forks/lowPriorityWarningWithoutStack.www.js b/packages/shared/forks/lowPriorityWarningWithoutStack.www.js deleted file mode 100644 index dd6f1ff82f33..000000000000 --- a/packages/shared/forks/lowPriorityWarningWithoutStack.www.js +++ /dev/null @@ -1,13 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -// This refers to a WWW module. -const lowPriorityWarningWWW = require('lowPriorityWarning'); - -export default function lowPriorityWarningWithoutStack(format, ...args) { - return lowPriorityWarningWWW(false, format, ...args); -} diff --git a/packages/shared/forks/warningWithoutStack.www.js b/packages/shared/forks/warningWithoutStack.www.js deleted file mode 100644 index 9f8c56c78ee7..000000000000 --- a/packages/shared/forks/warningWithoutStack.www.js +++ /dev/null @@ -1,13 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -// This refers to a WWW module. -const warningWWW = require('warning'); - -export default function warningWithoutStack(format, ...args) { - return warningWWW(false, format, ...args); -} diff --git a/packages/shared/lowPriorityWarning.js b/packages/shared/lowPriorityWarning.js deleted file mode 100644 index 6c81a82a3914..000000000000 --- a/packages/shared/lowPriorityWarning.js +++ /dev/null @@ -1,43 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import lowPriorityWarningWithoutStack from 'shared/lowPriorityWarningWithoutStack'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; - -/** - * Similar to invariant but only logs a warning if the condition is not met. - * This can be used to log issues in development environments in critical - * paths. Removing the logging code for production environments will keep the - * same logic and follow the same code paths. - */ - -let lowPriorityWarning = lowPriorityWarningWithoutStack; - -if (__DEV__) { - lowPriorityWarning = function(format, ...args) { - let finalFormat = format; - let finalArgs = args; - - const hasExistingStack = - args.length > 0 && - typeof args[args.length - 1] === 'string' && - args[args.length - 1].indexOf('\n in') === 0; - - if (!hasExistingStack) { - const ReactDebugCurrentFrame = - ReactSharedInternals.ReactDebugCurrentFrame; - const stack = ReactDebugCurrentFrame.getStackAddendum(); - finalFormat += '%s'; - finalArgs.push(stack); - } - - // eslint-disable-next-line react-internal/warning-args - lowPriorityWarningWithoutStack(finalFormat, ...finalArgs); - }; -} - -export default lowPriorityWarning; diff --git a/packages/shared/lowPriorityWarningWithoutStack.js b/packages/shared/lowPriorityWarningWithoutStack.js deleted file mode 100644 index 35cdd4de1acb..000000000000 --- a/packages/shared/lowPriorityWarningWithoutStack.js +++ /dev/null @@ -1,51 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -/** - * Forked from fbjs/warning: - * https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js - * - * Only change is we use console.warn instead of console.error, - * and do nothing when 'console' is not supported. - * This really simplifies the code. - * --- - * Similar to invariant but only logs a warning if the condition is not met. - * This can be used to log issues in development environments in critical - * paths. Removing the logging code for production environments will keep the - * same logic and follow the same code paths. - */ - -let lowPriorityWarningWithoutStack = function() {}; - -if (__DEV__) { - const printWarning = function(format, ...args) { - let argIndex = 0; - const message = 'Warning: ' + format.replace(/%s/g, () => args[argIndex++]); - if (typeof console !== 'undefined') { - console.warn(message); - } - try { - // --- Welcome to debugging React --- - // This error was thrown as a convenience so that you can use this stack - // to find the callsite that caused this warning to fire. - throw new Error(message); - } catch (x) {} - }; - - lowPriorityWarningWithoutStack = function(format, ...args) { - if (format === undefined) { - throw new Error( - '`lowPriorityWarningWithoutStack(condition, format, ...args)` requires a warning ' + - 'message argument', - ); - } - - printWarning(format, ...args); - }; -} - -export default lowPriorityWarningWithoutStack; diff --git a/packages/shared/warning.js b/packages/shared/warning.js deleted file mode 100644 index 189b651e5e65..000000000000 --- a/packages/shared/warning.js +++ /dev/null @@ -1,43 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import warningWithoutStack from 'shared/warningWithoutStack'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; - -/** - * Similar to invariant but only logs a warning if the condition is not met. - * This can be used to log issues in development environments in critical - * paths. Removing the logging code for production environments will keep the - * same logic and follow the same code paths. - */ - -let warning = warningWithoutStack; - -if (__DEV__) { - warning = function(format, ...args) { - let finalFormat = format; - let finalArgs = args; - - const hasExistingStack = - args.length > 0 && - typeof args[args.length - 1] === 'string' && - args[args.length - 1].indexOf('\n in') === 0; - - if (!hasExistingStack) { - const ReactDebugCurrentFrame = - ReactSharedInternals.ReactDebugCurrentFrame; - const stack = ReactDebugCurrentFrame.getStackAddendum(); - finalFormat += '%s'; - finalArgs.push(stack); - } - - // eslint-disable-next-line react-internal/warning-args - warningWithoutStack(finalFormat, ...finalArgs); - }; -} - -export default warning; diff --git a/packages/shared/warningWithoutStack.js b/packages/shared/warningWithoutStack.js deleted file mode 100644 index 934de64e01f2..000000000000 --- a/packages/shared/warningWithoutStack.js +++ /dev/null @@ -1,50 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -/** - * This can be used to log issues in development environments in critical - * paths. Removing the logging code for production environments will keep the - * same logic and follow the same code paths. - */ - -let warningWithoutStack = () => {}; - -if (__DEV__) { - warningWithoutStack = function(format, ...args) { - if (format === undefined) { - throw new Error( - '`warningWithoutStack(condition, format, ...args)` requires a warning ' + - 'message argument', - ); - } - if (args.length > 8) { - // Check before the condition to catch violations early. - throw new Error( - 'warningWithoutStack() currently supports at most 8 arguments.', - ); - } - if (typeof console !== 'undefined') { - const argsWithFormat = args.map(item => '' + item); - argsWithFormat.unshift('Warning: ' + format); - - // We intentionally don't use spread (or .apply) directly because it - // breaks IE9: https://github.com/facebook/react/issues/13610 - Function.prototype.apply.call(console.error, console, argsWithFormat); - } - try { - // --- Welcome to debugging React --- - // This error was thrown as a convenience so that you can use this stack - // to find the callsite that caused this warning to fire. - let argIndex = 0; - const message = - 'Warning: ' + format.replace(/%s/g, () => args[argIndex++]); - throw new Error(message); - } catch (x) {} - }; -} - -export default warningWithoutStack; diff --git a/scripts/babel/transform-replace-console-calls.js b/scripts/babel/transform-replace-console-calls.js new file mode 100644 index 000000000000..a1255be0ee6a --- /dev/null +++ b/scripts/babel/transform-replace-console-calls.js @@ -0,0 +1,66 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +'use strict'; + +const helperModuleImports = require('@babel/helper-module-imports'); + +module.exports = function replaceConsoleCalls(babel) { + let consoleErrors = new WeakMap(); + function getConsoleError(path, file, opts) { + if (!consoleErrors.has(file)) { + consoleErrors.set( + file, + helperModuleImports.addNamed( + path, + 'error', + 'shared/consoleWithStackDev', + {nameHint: 'consoleError'} + ) + ); + } + return babel.types.cloneDeep(consoleErrors.get(file)); + } + + let consoleWarns = new WeakMap(); + function getConsoleWarn(path, file, opts) { + if (!consoleWarns.has(file)) { + consoleWarns.set( + file, + helperModuleImports.addNamed( + path, + 'warn', + 'shared/consoleWithStackDev', + {nameHint: 'consoleWarn'} + ) + ); + } + return babel.types.cloneDeep(consoleWarns.get(file)); + } + + return { + visitor: { + CallExpression: function(path, pass) { + if (path.node.callee.type !== 'MemberExpression') { + return; + } + if (path.node.callee.property.type !== 'Identifier') { + // Don't process calls like console['error'](...) + // because they serve as an escape hatch. + return; + } + if (path.get('callee').matchesPattern('console.error')) { + const id = getConsoleError(path, pass.file, this.opts); + path.node.callee = id; + } + if (path.get('callee').matchesPattern('console.warn')) { + const id = getConsoleWarn(path, pass.file, this.opts); + path.node.callee = id; + } + }, + }, + }; +}; diff --git a/scripts/eslint-rules/__tests__/no-production-logging-test.internal.js b/scripts/eslint-rules/__tests__/no-production-logging-test.internal.js index 0c4ecbfe4ad2..c2d0d5572731 100644 --- a/scripts/eslint-rules/__tests__/no-production-logging-test.internal.js +++ b/scripts/eslint-rules/__tests__/no-production-logging-test.internal.js @@ -18,28 +18,35 @@ ruleTester.run('no-production-logging', rule, { { code: ` if (__DEV__) { - warning(test, 'Oh no'); + console.error('Oh no'); } `, }, { code: ` if (__DEV__) { - warningWithoutStack(test, 'Oh no'); + console.error('Hello %s', foo) } `, }, { code: ` if (__DEV__) { - lowPriorityWarning(test, 'Oh no'); + console.error('Hello %s %s', foo, bar) } `, }, { code: ` if (__DEV__) { - lowPriorityWarningWithoutStack(test, 'Oh no'); + console.warn('Oh no'); + } + `, + }, + { + code: ` + if (__DEV__) { + console.warn('Oh no'); } `, }, @@ -49,7 +56,7 @@ ruleTester.run('no-production-logging', rule, { if (__DEV__) { if (potato) { while (true) { - warning(test, 'Oh no'); + console.error('Oh no'); } } }`, @@ -61,7 +68,7 @@ ruleTester.run('no-production-logging', rule, { f = function() { if (potato) { while (true) { - warning(test, 'Oh no'); + console.error('Oh no'); } } }; @@ -88,116 +95,102 @@ ruleTester.run('no-production-logging', rule, { if (foo) { if (__DEV__) { } else { - warning(test, 'Oh no'); + console.error('Oh no'); } } }`, }, - ], - invalid: [ - { - code: 'warning(test);', - errors: [ - { - message: `Wrap warning() in an "if (__DEV__) {}" check`, - }, - ], - }, - { - code: 'warningWithoutStack(test)', - errors: [ - { - message: `Wrap warningWithoutStack() in an "if (__DEV__) {}" check`, - }, - ], - }, { + // This is an escape hatch that makes it fire in production. code: ` - if (potato) { - warningWithoutStack(test); - } + console['error']('Oh no'); `, + }, + ], + invalid: [ + { + code: "console.error('Oh no');", errors: [ { - message: `Wrap warningWithoutStack() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, { - code: 'lowPriorityWarning(test);', + code: "console.warn('Oh no');", errors: [ { - message: `Wrap lowPriorityWarning() in an "if (__DEV__) {}" check`, + message: `Wrap console.warn() in an "if (__DEV__) {}" check`, }, ], }, { - code: 'lowPriorityWarningWithoutStack(test)', + code: "console.warn('Oh no')", errors: [ { - message: `Wrap lowPriorityWarningWithoutStack() in an "if (__DEV__) {}" check`, + message: `Wrap console.warn() in an "if (__DEV__) {}" check`, }, ], }, { code: ` if (potato) { - lowPriorityWarningWithoutStack(test); + console.warn('Oh no'); } `, errors: [ { - message: `Wrap lowPriorityWarningWithoutStack() in an "if (__DEV__) {}" check`, + message: `Wrap console.warn() in an "if (__DEV__) {}" check`, }, ], }, { code: ` if (__DEV__ || potato && true) { - warning(test); + console.error('Oh no'); } `, errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, { code: ` if (banana && __DEV__ && potato && kitten) { - warning(test); + console.error('Oh no'); } `, // Technically this code is valid but we prefer // explicit standalone __DEV__ blocks that stand out. errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, { code: ` if (!__DEV__) { - warning(test); + console.error('Oh no'); } `, errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, { code: ` if (foo || x && __DEV__) { - warning(test); + console.error('Oh no'); } `, errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, @@ -205,12 +198,12 @@ ruleTester.run('no-production-logging', rule, { code: ` if (__DEV__) { } else { - warning(test); + console.error('Oh no'); } `, errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, }, ], }, @@ -220,13 +213,37 @@ ruleTester.run('no-production-logging', rule, { } else { if (__DEV__) { } else { - warning(test); + console.error('Oh no'); } } `, errors: [ { - message: `Wrap warning() in an "if (__DEV__) {}" check`, + message: `Wrap console.error() in an "if (__DEV__) {}" check`, + }, + ], + }, + { + code: ` + if (__DEV__) { + console.log('Oh no'); + } + `, + errors: [ + { + message: 'Unexpected use of console', + }, + ], + }, + { + code: ` + if (__DEV__) { + console.log.apply(console, 'Oh no'); + } + `, + errors: [ + { + message: 'Unexpected use of console', }, ], }, diff --git a/scripts/eslint-rules/__tests__/warning-args-test.internal.js b/scripts/eslint-rules/__tests__/warning-args-test.internal.js index cd50e2927a3f..66f51fc717c5 100644 --- a/scripts/eslint-rules/__tests__/warning-args-test.internal.js +++ b/scripts/eslint-rules/__tests__/warning-args-test.internal.js @@ -15,63 +15,74 @@ const ruleTester = new RuleTester(); ruleTester.run('eslint-rules/warning-args', rule, { valid: [ - "warning('hello, world');", - "warning('expected %s, got %s', 42, 24);", + "console.error('hello, world');", + "console.error('expected %s, got %s', 42, 24);", 'arbitraryFunction(a, b)', ], invalid: [ { - code: 'warning(null);', + code: 'console.error(null);', errors: [ { - message: 'The first argument to warning must be a string literal', + message: + 'The first argument to console.error must be a string literal', }, ], }, { - code: 'var g = 5; warning(g);', + code: 'console.warn(null);', errors: [ { - message: 'The first argument to warning must be a string literal', + message: + 'The first argument to console.warn must be a string literal', + }, + ], + }, + { + code: 'var g = 5; console.error(g);', + errors: [ + { + message: + 'The first argument to console.error must be a string literal', }, ], }, { - code: "warning('expected %s, got %s');", + code: "console.error('expected %s, got %s');", errors: [ { message: - 'Expected 3 arguments in call to warning based on the number of ' + + 'Expected 3 arguments in call to console.error based on the number of ' + '"%s" substitutions, but got 1', }, ], }, { - code: "warning('foo is a bar under foobar', 'junk argument');", + code: "console.error('foo is a bar under foobar', 'junk argument');", errors: [ { message: - 'Expected 1 arguments in call to warning based on the number of ' + + 'Expected 1 arguments in call to console.error based on the number of ' + '"%s" substitutions, but got 2', }, ], }, { - code: "warning('error!');", + code: "console.error('error!');", errors: [ { message: - 'The warning format should be able to uniquely identify this ' + + 'The console.error format should be able to uniquely identify this ' + 'warning. Please, use a more descriptive format than: error!', }, ], }, { - code: "warning('%s %s, %s %s: %s (%s)', 1, 2, 3, 4, 5, 6);", + code: "console.error('%s %s, %s %s: %s (%s)', 1, 2, 3, 4, 5, 6);", errors: [ { message: - 'The warning format should be able to uniquely identify this ' + + 'The console.error format should be able to uniquely identify this ' + 'warning. Please, use a more descriptive format than: ' + '%s %s, %s %s: %s (%s)', }, diff --git a/scripts/eslint-rules/no-production-logging.js b/scripts/eslint-rules/no-production-logging.js index 3732cf2387b5..c6a134951262 100644 --- a/scripts/eslint-rules/no-production-logging.js +++ b/scripts/eslint-rules/no-production-logging.js @@ -9,13 +9,6 @@ 'use strict'; -const LOGGER_FN_NAMES = [ - 'warning', - 'warningWithoutStack', - 'lowPriorityWarning', - 'lowPriorityWarningWithoutStack', -]; - module.exports = function(context) { function isInDEVBlock(node) { let done = false; @@ -38,12 +31,12 @@ module.exports = function(context) { } } - function report(node) { + function reportWrapInDEV(node) { context.report({ node: node, - message: `Wrap {{identifier}}() in an "if (__DEV__) {}" check`, + message: `Wrap console.{{identifier}}() in an "if (__DEV__) {}" check`, data: { - identifier: node.callee.name, + identifier: node.property.name, }, fix: function(fixer) { return [ @@ -54,18 +47,36 @@ module.exports = function(context) { }); } - const isLoggerFunctionName = name => LOGGER_FN_NAMES.includes(name); + function reportUnexpectedConsole(node) { + context.report({ + node: node, + message: `Unexpected use of console`, + }); + } return { meta: { fixable: 'code', }, - CallExpression: function(node) { - if (!isLoggerFunctionName(node.callee.name)) { - return; - } - if (!isInDEVBlock(node)) { - report(node); + MemberExpression: function(node) { + if ( + node.object.type === 'Identifier' && + node.object.name === 'console' && + node.property.type === 'Identifier' + ) { + switch (node.property.name) { + case 'error': + case 'warn': { + if (!isInDEVBlock(node)) { + reportWrapInDEV(node); + } + break; + } + default: { + reportUnexpectedConsole(node); + break; + } + } } }, }; diff --git a/scripts/eslint-rules/warning-args.js b/scripts/eslint-rules/warning-args.js index 59345b0a8a13..5804e0ca35ed 100644 --- a/scripts/eslint-rules/warning-args.js +++ b/scripts/eslint-rules/warning-args.js @@ -44,17 +44,18 @@ module.exports = function(context) { // This could be a little smarter by checking context.getScope() to see // how warning/invariant was defined. const isWarning = - node.callee.type === 'Identifier' && - (node.callee.name === 'warning' || - node.callee.name === 'warningWithoutStack' || - node.callee.name === 'lowPriorityWarning' || - node.callee.name === 'lowPriorityWarningWithoutStack'); + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'Identifier' && + node.callee.property.type === 'Identifier' && + (node.callee.property.name === 'error' || + node.callee.property.name === 'warn'); if (!isWarning) { return; } + const name = 'console.' + node.callee.property.name; if (node.arguments.length < 1) { context.report(node, '{{name}} takes at least one argument', { - name: node.callee.name, + name, }); return; } @@ -63,7 +64,7 @@ module.exports = function(context) { context.report( node, 'The first argument to {{name}} must be a string literal', - {name: node.callee.name} + {name} ); return; } @@ -71,8 +72,8 @@ module.exports = function(context) { context.report( node, 'The {{name}} format should be able to uniquely identify this ' + - '{{name}}. Please, use a more descriptive format than: {{format}}', - {name: node.callee.name, format: format} + 'warning. Please, use a more descriptive format than: {{format}}', + {name, format} ); return; } @@ -85,7 +86,7 @@ module.exports = function(context) { 'the number of "%s" substitutions, but got {{length}}', { expectedNArgs: expectedNArgs, - name: node.callee.name, + name, length: node.arguments.length, } ); diff --git a/scripts/jest/preprocessor.js b/scripts/jest/preprocessor.js index 44f96c91e28f..9be5f5bbd451 100644 --- a/scripts/jest/preprocessor.js +++ b/scripts/jest/preprocessor.js @@ -16,6 +16,9 @@ const pathToBabel = path.join( const pathToBabelPluginDevWithCode = require.resolve( '../error-codes/transform-error-messages' ); +const pathToBabelPluginReplaceConsoleCalls = require.resolve( + '../babel/transform-replace-console-calls' +); const pathToBabelPluginAsyncToGenerator = require.resolve( '@babel/plugin-transform-async-to-generator' ); @@ -65,18 +68,22 @@ module.exports = { // for test files, we also apply the async-await transform, but we want to // make sure we don't accidentally apply that transform to product code. const isTestFile = !!filePath.match(/\/__tests__\//); + const testOnlyPlugins = [pathToBabelPluginAsyncToGenerator]; + const sourceOnlyPlugins = + process.env.NODE_ENV === 'development' + ? [pathToBabelPluginReplaceConsoleCalls] + : []; + const plugins = (isTestFile ? testOnlyPlugins : sourceOnlyPlugins).concat( + babelOptions.plugins + ); return babel.transform( src, Object.assign( {filename: path.relative(process.cwd(), filePath)}, babelOptions, - isTestFile - ? { - plugins: [pathToBabelPluginAsyncToGenerator].concat( - babelOptions.plugins - ), - } - : {} + { + plugins, + } ) ); } diff --git a/scripts/print-warnings/print-warnings.js b/scripts/print-warnings/print-warnings.js index 4ac866b03d8b..2e61598ad935 100644 --- a/scripts/print-warnings/print-warnings.js +++ b/scripts/print-warnings/print-warnings.js @@ -51,13 +51,16 @@ function transform(file, enc, cb) { exit: function(astPath) { const callee = astPath.get('callee'); if ( - callee.isIdentifier({name: 'warning'}) || - callee.isIdentifier({name: 'warningWithoutStack'}) || - callee.isIdentifier({name: 'lowPriorityWarning'}) || - callee.isIdentifier({name: 'lowPriorityWarningWithoutStack'}) + callee.matchesPattern('console.warn') || + callee.matchesPattern('console.error') ) { const node = astPath.node; - + if (node.callee.type !== 'MemberExpression') { + return; + } + if (node.callee.property.type !== 'Identifier') { + return; + } // warning messages can be concatenated (`+`) at runtime, so here's // a trivial partial evaluator that interprets the literal value try { @@ -82,8 +85,8 @@ function transform(file, enc, cb) { gs([ 'packages/**/*.js', - '!packages/shared/warning.js', - '!packages/shared/lowPriorityWarning.js', + '!packages/*/npm/**/*.js', + '!packages/shared/consoleWithStackDev.js', '!packages/react-devtools*/**/*.js', '!**/__tests__/**/*.js', '!**/__mocks__/**/*.js', diff --git a/scripts/rollup/build.js b/scripts/rollup/build.js index e527f755adc9..80a7c18e1f07 100644 --- a/scripts/rollup/build.js +++ b/scripts/rollup/build.js @@ -107,12 +107,26 @@ const closureOptions = { rewrite_polyfills: false, }; -function getBabelConfig(updateBabelOptions, bundleType, filename) { +function getBabelConfig( + updateBabelOptions, + bundleType, + packageName, + externals, + isDevelopment +) { + const canAccessReactObject = + packageName === 'react' || externals.indexOf('react') !== -1; let options = { exclude: '/**/node_modules/**', presets: [], plugins: [], }; + if (isDevelopment && canAccessReactObject) { + options.plugins.push( + // Turn console.error/warn() into a custom wrapper + require('../babel/transform-replace-console-calls') + ); + } if (updateBabelOptions) { options = updateBabelOptions(options); } @@ -352,7 +366,15 @@ function getPlugins( exclude: 'node_modules/**/*', }), // Compile to ES5. - babel(getBabelConfig(updateBabelOptions, bundleType)), + babel( + getBabelConfig( + updateBabelOptions, + bundleType, + packageName, + externals, + !isProduction + ) + ), // Remove 'use strict' from individual source files. { transform(source) { diff --git a/scripts/rollup/forks.js b/scripts/rollup/forks.js index 8c94593ec838..b6d2fe5caf55 100644 --- a/scripts/rollup/forks.js +++ b/scripts/rollup/forks.js @@ -57,9 +57,7 @@ const forks = Object.freeze({ 'from "' + entry + '" because it does not declare "react" in the package ' + - 'dependencies or peerDependencies. For example, this can happen if you use ' + - 'warning() instead of warningWithoutStack() in a package that does not ' + - 'depend on React.' + 'dependencies or peerDependencies.' ); } return null; @@ -180,25 +178,10 @@ const forks = Object.freeze({ return 'scheduler/src/forks/SchedulerHostConfig.default'; }, - // This logic is forked on www to ignore some warnings. - 'shared/lowPriorityWarningWithoutStack': (bundleType, entry) => { + 'shared/consoleWithStackDev': (bundleType, entry) => { switch (bundleType) { case FB_WWW_DEV: - case FB_WWW_PROD: - case FB_WWW_PROFILING: - return 'shared/forks/lowPriorityWarningWithoutStack.www.js'; - default: - return null; - } - }, - - // This logic is forked on www to ignore some warnings. - 'shared/warningWithoutStack': (bundleType, entry) => { - switch (bundleType) { - case FB_WWW_DEV: - case FB_WWW_PROD: - case FB_WWW_PROFILING: - return 'shared/forks/warningWithoutStack.www.js'; + return 'shared/forks/consoleWithStackDev.www.js'; default: return null; } diff --git a/scripts/shared/evalToString.js b/scripts/shared/evalToString.js index f98e2f234cce..aad199da5d75 100644 --- a/scripts/shared/evalToString.js +++ b/scripts/shared/evalToString.js @@ -11,6 +11,7 @@ function evalToString(ast /* : Object */) /* : string */ { switch (ast.type) { case 'StringLiteral': + case 'Literal': // ESLint return ast.value; case 'BinaryExpression': // `+` if (ast.operator !== '+') {