From 178cb7d3392505cd393b68e4e061c6ef860344a1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 10 Aug 2016 19:52:46 +0100 Subject: [PATCH] Prevent performance regression in DEV due to warning arguments (#7461) * Prevent internal performance regression This only affects Facebook website, not open source version of React. On the Facebook website, we don't have a transform for warnings and invariants. Therefore, expensive arguments will be calculated even if the warning doesn't fire. This fixes a few cases where that calculation might be more expensive than usually. In my testing, this brings down average row click time in Power Editor from ~300ms to ~220ms in __DEV__ (vs ~40ms in prod). * Put warning() that shows up in profile behind condition --- .../classic/element/ReactElementValidator.js | 16 +++++++++------- .../dom/client/wrappers/ReactDOMSelect.js | 9 +++++---- .../hooks/ReactDOMUnknownPropertyHook.js | 4 ++-- src/renderers/shared/ReactDebugTool.js | 4 +++- .../hooks/ReactChildrenMutationWarningHook.js | 12 +++++++----- .../stack/reconciler/ReactChildReconciler.js | 18 ++++++++++-------- src/shared/utils/flattenChildren.js | 18 ++++++++++-------- 7 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 227af93995594..4305d15990221 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -188,13 +188,15 @@ var ReactElementValidator = { (type !== null && typeof type === 'object'); // We warn in this case but don't throw. We expect the element creation to // succeed and there will likely be errors in render. - warning( - validType, - 'React.createElement: type should not be null, undefined, boolean, or ' + - 'number. It should be a string (for DOM elements) or a ReactClass ' + - '(for composite components).%s', - getDeclarationErrorAddendum() - ); + if (!validType) { + warning( + false, + 'React.createElement: type should not be null, undefined, boolean, or ' + + 'number. It should be a string (for DOM elements) or a ReactClass ' + + '(for composite components).%s', + getDeclarationErrorAddendum() + ); + } var element = ReactElement.createElement.apply(this, arguments); diff --git a/src/renderers/dom/client/wrappers/ReactDOMSelect.js b/src/renderers/dom/client/wrappers/ReactDOMSelect.js index c693fb7036e05..31027536f3fed 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMSelect.js +++ b/src/renderers/dom/client/wrappers/ReactDOMSelect.js @@ -71,17 +71,18 @@ function checkSelectPropTypes(inst, props) { if (props[propName] == null) { continue; } - if (props.multiple) { + var isArray = Array.isArray(props[propName]); + if (props.multiple && !isArray) { warning( - Array.isArray(props[propName]), + false, 'The `%s` prop supplied to must be a scalar ' + 'value if `multiple` is false.%s', propName, diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index 6df617e261b1f..ef0937a7fc555 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -69,7 +69,7 @@ if (__DEV__) { if (standardName != null) { warning( - standardName == null, + false, 'Unknown DOM property %s. Did you mean %s?%s', name, standardName, @@ -78,7 +78,7 @@ if (__DEV__) { return true; } else if (registrationName != null) { warning( - registrationName == null, + false, 'Unknown event handler property %s. Did you mean `%s`?%s', name, registrationName, diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js index 4882f47a20510..e573ee748856e 100644 --- a/src/renderers/shared/ReactDebugTool.js +++ b/src/renderers/shared/ReactDebugTool.js @@ -104,7 +104,9 @@ function resetMeasurements() { } function checkDebugID(debugID) { - warning(debugID, 'ReactDebugTool: debugID may not be empty.'); + if (!debugID) { + warning(false, 'ReactDebugTool: debugID may not be empty.'); + } } function beginLifeCycleTimer(debugID, timerType) { diff --git a/src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js b/src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js index e8d063d58a9a6..3a1f2cd67bba5 100644 --- a/src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js +++ b/src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js @@ -37,11 +37,13 @@ function handleElement(debugID, element) { isMutated = true; } } - warning( - Array.isArray(element._shadowChildren) && !isMutated, - 'Component\'s children should not be mutated.%s', - ReactComponentTreeHook.getStackAddendumByID(debugID), - ); + if (!Array.isArray(element._shadowChildren) || isMutated) { + warning( + false, + 'Component\'s children should not be mutated.%s', + ReactComponentTreeHook.getStackAddendumByID(debugID), + ); + } } var ReactChildrenMutationWarningHook = { diff --git a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js index fc1f32c73abc6..2ae4c0a51e26f 100644 --- a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js +++ b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js @@ -41,14 +41,16 @@ function instantiateChild(childInstances, child, name, selfDebugID) { if (!ReactComponentTreeHook) { ReactComponentTreeHook = require('ReactComponentTreeHook'); } - warning( - keyUnique, - 'flattenChildren(...): Encountered two children with the same key, ' + - '`%s`. Child keys must be unique; when two children share a key, only ' + - 'the first child will be used.%s', - KeyEscapeUtils.unescape(name), - ReactComponentTreeHook.getStackAddendumByID(selfDebugID) - ); + if (!keyUnique) { + warning( + false, + 'flattenChildren(...): Encountered two children with the same key, ' + + '`%s`. Child keys must be unique; when two children share a key, only ' + + 'the first child will be used.%s', + KeyEscapeUtils.unescape(name), + ReactComponentTreeHook.getStackAddendumByID(selfDebugID) + ); + } } if (child != null && keyUnique) { childInstances[name] = instantiateReactComponent(child, true); diff --git a/src/shared/utils/flattenChildren.js b/src/shared/utils/flattenChildren.js index b4ef7b3ce98e0..d3625bc4b0052 100644 --- a/src/shared/utils/flattenChildren.js +++ b/src/shared/utils/flattenChildren.js @@ -51,14 +51,16 @@ function flattenSingleChildIntoContext( if (!ReactComponentTreeHook) { ReactComponentTreeHook = require('ReactComponentTreeHook'); } - warning( - keyUnique, - 'flattenChildren(...): Encountered two children with the same key, ' + - '`%s`. Child keys must be unique; when two children share a key, only ' + - 'the first child will be used.%s', - KeyEscapeUtils.unescape(name), - ReactComponentTreeHook.getStackAddendumByID(selfDebugID) - ); + if (!keyUnique) { + warning( + false, + 'flattenChildren(...): Encountered two children with the same key, ' + + '`%s`. Child keys must be unique; when two children share a key, only ' + + 'the first child will be used.%s', + KeyEscapeUtils.unescape(name), + ReactComponentTreeHook.getStackAddendumByID(selfDebugID) + ); + } } if (keyUnique && child != null) { result[name] = child;