From 738fede8798daa090f113099bf799ffad5461363 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Thu, 25 Jul 2019 15:53:08 -0700 Subject: [PATCH 1/7] added feature flags for export const warnAboutStringRefs = false; --- .../src/__tests__/ReactDeprecationWarnings-test.internal.js | 2 +- packages/shared/ReactFeatureFlags.js | 1 + packages/shared/forks/ReactFeatureFlags.native-oss.js | 1 + packages/shared/forks/ReactFeatureFlags.persistent.js | 1 + packages/shared/forks/ReactFeatureFlags.test-renderer.js | 1 + packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 2 ++ 7 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js index d4413dd4e87..91d4163c801 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js @@ -26,7 +26,7 @@ describe('ReactDeprecationWarnings', () => { ReactFeatureFlags.warnAboutDefaultPropsOnFunctionComponents = false; }); - it('should warn when given defaultProps', () => { + ift('should warn when given defaultProps', () => { function FunctionalComponent(props) { return null; } diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index ac92811f095..0573280a947 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -88,3 +88,4 @@ export const enableSuspenseCallback = false; // from React.createElement to React.jsx // https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index e1bb4ecae4b..76253f7bcbb 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -34,6 +34,7 @@ export const revertPassiveEffectsChange = false; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index c392336fc51..7b72539abfb 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -34,6 +34,7 @@ export const revertPassiveEffectsChange = false; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index a38d1f5ff82..deb19080879 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -34,6 +34,7 @@ export const revertPassiveEffectsChange = false; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index c8f1beb3e62..ddc8bbcf856 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -34,6 +34,7 @@ export const warnAboutMissingMockScheduler = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 284416c8255..2970cf4a5df 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -80,6 +80,8 @@ export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; + // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars type Check<_X, Y: _X, X: Y = _X> = null; From 67687d604bef5436ceb2f836bc19e1eb2155f31c Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Thu, 25 Jul 2019 18:12:13 -0700 Subject: [PATCH 2/7] modified existing strict mode string ref warning to work for everyone if feature flag is turned on --- .../ReactDeprecationWarnings-test.internal.js | 40 ++++++++++++++++--- .../react-reconciler/src/ReactChildFiber.js | 34 +++++++++++----- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js index 91d4163c801..fbae1bce6c7 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js @@ -10,23 +10,27 @@ 'use strict'; let React; -let ReactTestUtils; let ReactFeatureFlags; +let ReactNoop; +let Scheduler; describe('ReactDeprecationWarnings', () => { beforeEach(() => { jest.resetModules(); React = require('react'); ReactFeatureFlags = require('shared/ReactFeatureFlags'); - ReactTestUtils = require('react-dom/test-utils'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); ReactFeatureFlags.warnAboutDefaultPropsOnFunctionComponents = true; + ReactFeatureFlags.warnAboutStringRefs = true; }); afterEach(() => { ReactFeatureFlags.warnAboutDefaultPropsOnFunctionComponents = false; + ReactFeatureFlags.warnAboutStringRefs = false; }); - ift('should warn when given defaultProps', () => { + it('should warn when given defaultProps', () => { function FunctionalComponent(props) { return null; } @@ -35,13 +39,37 @@ describe('ReactDeprecationWarnings', () => { testProp: true, }; - expect(() => - ReactTestUtils.renderIntoDocument(), - ).toWarnDev( + ReactNoop.render(); + expect(() => expect(Scheduler).toFlushWithoutYielding()).toWarnDev( 'Warning: FunctionalComponent: Support for defaultProps ' + 'will be removed from function components in a future major ' + 'release. Use JavaScript default parameters instead.', {withoutStack: true}, ); }); + + it('should warn when given string refs', () => { + class RefComponent extends React.Component { + render() { + return null; + } + } + class Component extends React.Component { + render() { + return ; + } + } + + ReactNoop.render(); + expect(() => expect(Scheduler).toFlushWithoutYielding()).toWarnDev( + 'Warning: Component "Component" contains the string ref "refComponent". ' + + 'Support for string refs will be removed in a future major release. ' + + 'We recommend using createRef() instead.' + + '\n\n' + + ' in Component (at **)' + + '\n\n' + + 'Learn more about using refs safely here:\n' + + 'https://fb.me/react-strict-mode-string-ref', + ); + }); }); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index c0aeb7ccdd2..514fe7d9749 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -30,6 +30,7 @@ import { import invariant from 'shared/invariant'; import warning from 'shared/warning'; import warningWithoutStack from 'shared/warningWithoutStack'; +import {warnAboutStringRefs} from 'shared/ReactFeatureFlags'; import { createWorkInProgress, @@ -49,7 +50,7 @@ import {StrictMode} from './ReactTypeOfMode'; let didWarnAboutMaps; let didWarnAboutGenerators; -let didWarnAboutStringRefInStrictMode; +let didWarnAboutStringRefs; let ownerHasKeyUseWarning; let ownerHasFunctionTypeWarning; let warnForMissingKey = (child: mixed) => {}; @@ -57,7 +58,7 @@ let warnForMissingKey = (child: mixed) => {}; if (__DEV__) { didWarnAboutMaps = false; didWarnAboutGenerators = false; - didWarnAboutStringRefInStrictMode = {}; + didWarnAboutStringRefs = {}; /** * Warn if there's no key explicitly set on dynamic arrays of children or @@ -114,21 +115,36 @@ function coerceRef( typeof mixedRef !== 'object' ) { if (__DEV__) { - if (returnFiber.mode & StrictMode) { + // TODO: Clean this up once we turn on the string ref warning for + // everyone, because strict mode case will no longer be relevant + if (returnFiber.mode & StrictMode || warnAboutStringRefs) { const componentName = getComponentName(returnFiber.type) || 'Component'; - if (!didWarnAboutStringRefInStrictMode[componentName]) { + if (!didWarnAboutStringRefs[componentName]) { + let deprecationWarning; + if (warnAboutStringRefs) { + deprecationWarning = + 'Component "' + + componentName + + '" contains the string ref "' + + mixedRef + + '". Support for string refs will be removed in a future major release.'; + } else { + deprecationWarning = + 'A string ref, "' + + mixedRef + + '", has been found within a strict mode tree. String refs are a source of ' + + 'potential bugs and should be avoided.'; + } warningWithoutStack( false, - 'A string ref, "%s", has been found within a strict mode tree. ' + - 'String refs are a source of potential bugs and should be avoided. ' + - 'We recommend using createRef() instead.' + + '%s We recommend using createRef() instead.' + '\n%s' + '\n\nLearn more about using refs safely here:' + '\nhttps://fb.me/react-strict-mode-string-ref', - mixedRef, + deprecationWarning, getStackByFiberInDevAndProd(returnFiber), ); - didWarnAboutStringRefInStrictMode[componentName] = true; + didWarnAboutStringRefs[componentName] = true; } } } From 4ecbd40504f89167fb3ed292662b6729486a77e0 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Thu, 25 Jul 2019 19:23:08 -0700 Subject: [PATCH 3/7] rename warning --- packages/react-reconciler/src/ReactChildFiber.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 514fe7d9749..7653836ff20 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -116,20 +116,20 @@ function coerceRef( ) { if (__DEV__) { // TODO: Clean this up once we turn on the string ref warning for - // everyone, because strict mode case will no longer be relevant + // everyone, because the strict mode case will no longer be relevant if (returnFiber.mode & StrictMode || warnAboutStringRefs) { const componentName = getComponentName(returnFiber.type) || 'Component'; if (!didWarnAboutStringRefs[componentName]) { - let deprecationWarning; + let stringRefWarning; if (warnAboutStringRefs) { - deprecationWarning = + stringRefWarning = 'Component "' + componentName + '" contains the string ref "' + mixedRef + '". Support for string refs will be removed in a future major release.'; } else { - deprecationWarning = + stringRefWarning = 'A string ref, "' + mixedRef + '", has been found within a strict mode tree. String refs are a source of ' + @@ -141,7 +141,7 @@ function coerceRef( '\n%s' + '\n\nLearn more about using refs safely here:' + '\nhttps://fb.me/react-strict-mode-string-ref', - deprecationWarning, + stringRefWarning, getStackByFiberInDevAndProd(returnFiber), ); didWarnAboutStringRefs[componentName] = true; From c177c8963cb877cd9ed750b9ef665de1cbb47de9 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Thu, 25 Jul 2019 19:34:20 -0700 Subject: [PATCH 4/7] added warnAboutStringRefs to more files --- packages/shared/forks/ReactFeatureFlags.native-fb.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 08baee4c30c..e48e84795de 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -39,6 +39,7 @@ export const revertPassiveEffectsChange = false; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const warnAboutStringRefs = false; // Only used in www builds. export function addUserTimingListener() { From 3cb6c9e78ba8d1f1b54699e9e8966dc5d1e24c25 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Fri, 26 Jul 2019 09:39:11 -0700 Subject: [PATCH 5/7] refactored warning into 2 calls --- .../react-reconciler/src/ReactChildFiber.js | 44 ++++++++++--------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 7653836ff20..035f6fe385b 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -120,30 +120,32 @@ function coerceRef( if (returnFiber.mode & StrictMode || warnAboutStringRefs) { const componentName = getComponentName(returnFiber.type) || 'Component'; if (!didWarnAboutStringRefs[componentName]) { - let stringRefWarning; if (warnAboutStringRefs) { - stringRefWarning = - 'Component "' + - componentName + - '" contains the string ref "' + - mixedRef + - '". Support for string refs will be removed in a future major release.'; + warningWithoutStack( + false, + 'Component "%s" contains the string ref "%s". Support for string refs ' + + 'will be removed in a future major release. We recommend using ' + + 'createRef() instead.' + + '\n%s' + + '\n\nLearn more about using refs safely here:' + + '\nhttps://fb.me/react-strict-mode-string-ref', + componentName, + mixedRef, + getStackByFiberInDevAndProd(returnFiber), + ); } else { - stringRefWarning = - 'A string ref, "' + - mixedRef + - '", has been found within a strict mode tree. String refs are a source of ' + - 'potential bugs and should be avoided.'; + warningWithoutStack( + false, + 'A string ref, "%s", has been found within a strict mode tree. ' + + 'String refs are a source of potential bugs and should be avoided. ' + + 'We recommend using createRef() instead.' + + '\n%s' + + '\n\nLearn more about using refs safely here:' + + '\nhttps://fb.me/react-strict-mode-string-ref', + mixedRef, + getStackByFiberInDevAndProd(returnFiber), + ); } - warningWithoutStack( - false, - '%s We recommend using createRef() instead.' + - '\n%s' + - '\n\nLearn more about using refs safely here:' + - '\nhttps://fb.me/react-strict-mode-string-ref', - stringRefWarning, - getStackByFiberInDevAndProd(returnFiber), - ); didWarnAboutStringRefs[componentName] = true; } } From a5998f9a945de3604913951b85ee0006d1fed9fc Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Mon, 29 Jul 2019 17:23:20 -0700 Subject: [PATCH 6/7] changed text for warning --- packages/react-reconciler/src/ReactChildFiber.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 035f6fe385b..af113294a94 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -125,7 +125,7 @@ function coerceRef( false, 'Component "%s" contains the string ref "%s". Support for string refs ' + 'will be removed in a future major release. We recommend using ' + - 'createRef() instead.' + + 'useRef() or createRef() instead.' + '\n%s' + '\n\nLearn more about using refs safely here:' + '\nhttps://fb.me/react-strict-mode-string-ref', @@ -138,7 +138,7 @@ function coerceRef( false, 'A string ref, "%s", has been found within a strict mode tree. ' + 'String refs are a source of potential bugs and should be avoided. ' + - 'We recommend using createRef() instead.' + + 'We recommend using useRef() or createRef() instead.' + '\n%s' + '\n\nLearn more about using refs safely here:' + '\nhttps://fb.me/react-strict-mode-string-ref', From 54d40ee6941fbc80b450bf8b17336ff8b53023d5 Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Mon, 29 Jul 2019 17:27:55 -0700 Subject: [PATCH 7/7] fix tests --- .../src/__tests__/ReactDeprecationWarnings-test.internal.js | 2 +- packages/react/src/__tests__/ReactStrictMode-test.internal.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js index fbae1bce6c7..9d29ef5a34c 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js @@ -64,7 +64,7 @@ describe('ReactDeprecationWarnings', () => { expect(() => expect(Scheduler).toFlushWithoutYielding()).toWarnDev( 'Warning: Component "Component" contains the string ref "refComponent". ' + 'Support for string refs will be removed in a future major release. ' + - 'We recommend using createRef() instead.' + + 'We recommend using useRef() or createRef() instead.' + '\n\n' + ' in Component (at **)' + '\n\n' + diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index 5b6afee5581..b631690ffdb 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -693,7 +693,7 @@ Please update the following components: Parent`, }).toWarnDev( 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + 'String refs are a source of potential bugs and should be avoided. ' + - 'We recommend using createRef() instead.\n\n' + + 'We recommend using useRef() or createRef() instead.\n\n' + ' in StrictMode (at **)\n' + ' in OuterComponent (at **)\n\n' + 'Learn more about using refs safely here:\n' + @@ -735,7 +735,7 @@ Please update the following components: Parent`, }).toWarnDev( 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + 'String refs are a source of potential bugs and should be avoided. ' + - 'We recommend using createRef() instead.\n\n' + + 'We recommend using useRef() or createRef() instead.\n\n' + ' in InnerComponent (at **)\n' + ' in StrictMode (at **)\n' + ' in OuterComponent (at **)\n\n' +