From 146e77f6480caf48880f8f09b622bc3445e45e65 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 23 May 2017 09:35:42 -0700 Subject: [PATCH 1/3] Downgrade deprecation warnings from errors to warnings (#9650) * Downgrade deprecation warnings from errors to warnings **what is the change?:** Swapping out `warning` module for a fork that uses `console.warn`. It looks like we were using the `warning` module for deprecation notices, *but* there is also a 'deprecated' module designed specifically for deprecation notices. However, we could not find any place that it was currently used. Since React's build process is not 100% clear to me, I assume it could still be used somewhere by something and just updated it along with other deprecation notices. We might consider a follow-up diff that does some clean up here; - remove 'deprecated' module if it's unused, OR - use 'deprecated' module for all our current deprecation warnings **why make this change?:** - We have had complaints about noisy warnings, in particular after introducing new deprecations - They potentially cause CI failures - Deprecations are not really time-sensitive, can ship without breaking your app, etc. For more context - https://github.com/facebook/react/issues/9395 **test plan:** `npm run test` and unit tests for the new modules and manual testing (WIP) **issue:** https://github.com/facebook/react/issues/9395 * Add 'lowPriorityWarning' to ReactExternals **what is the change?:** We won't bundle 'lowPriorityWarning' with the rest of React when building for Facebook. NOTE: A parallel commit will introduce an internal implementation of 'lowPriorityWarning' in Facebook's codebase, to compensate. Will post a comment with the diff number once that is up. **why make this change?:** So that the sync between github and Facebook can go more smoothly! **test plan:** We will see when I run the sync! But this is a reasonable first step imo. **issue:** https://github.com/facebook/react/issues/9398 --- .../__tests__/ReactDOMFactories-test.js | 8 ++-- src/isomorphic/React.js | 5 ++- src/isomorphic/__tests__/React-test.js | 12 +++--- .../classic/element/ReactElementValidator.js | 11 ++++- .../__tests__/ReactElementValidator-test.js | 8 ++-- src/isomorphic/modern/class/ReactComponent.js | 4 +- .../ReactCoffeeScriptClass-test.coffee | 8 ++-- .../class/__tests__/ReactES6Class-test.js | 8 ++-- .../__tests__/ReactTypeScriptClass-test.ts | 8 ++-- src/renderers/shared/ReactPerf.js | 6 +-- .../shared/__tests__/ReactPerf-test.js | 16 ++++---- src/shared/utils/deprecated.js | 4 +- src/shared/utils/lowPriorityWarning.js | 41 +++++++++++++++++++ 13 files changed, 95 insertions(+), 44 deletions(-) create mode 100644 src/shared/utils/lowPriorityWarning.js diff --git a/src/addons/__tests__/ReactDOMFactories-test.js b/src/addons/__tests__/ReactDOMFactories-test.js index b4a7a78ceda57..641faa3f003dd 100644 --- a/src/addons/__tests__/ReactDOMFactories-test.js +++ b/src/addons/__tests__/ReactDOMFactories-test.js @@ -17,13 +17,15 @@ var {div} = require('ReactDOMFactories'); describe('ReactDOMFactories', () => { it('allow factories to be called without warnings', () => { spyOn(console, 'error'); + spyOn(console, 'warn'); var element = div(); expect(element.type).toBe('div'); expect(console.error).not.toHaveBeenCalled(); + expect(console.warn).not.toHaveBeenCalled(); }); it('warns once when accessing React.DOM methods', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); var a = React.DOM.a(); var p = React.DOM.p(); @@ -31,8 +33,8 @@ describe('ReactDOMFactories', () => { expect(a.type).toBe('a'); expect(p.type).toBe('p'); - expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.first().args[0]).toContain( + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn.calls.first().args[0]).toContain( 'Warning: Accessing factories like React.DOM.a has been deprecated', ); }); diff --git a/src/isomorphic/React.js b/src/isomorphic/React.js index f9cb15cc19ff7..1e30e85cc7daf 100644 --- a/src/isomorphic/React.js +++ b/src/isomorphic/React.js @@ -28,6 +28,7 @@ var createFactory = ReactElement.createFactory; var cloneElement = ReactElement.cloneElement; if (__DEV__) { + var lowPriorityWarning = require('lowPriorityWarning'); var canDefineProperty = require('canDefineProperty'); var ReactElementValidator = require('ReactElementValidator'); var didWarnPropTypesDeprecated = false; @@ -107,7 +108,7 @@ if (__DEV__) { if (canDefineProperty) { Object.defineProperty(React, 'PropTypes', { get() { - warning( + lowPriorityWarning( didWarnPropTypesDeprecated, 'Accessing PropTypes via the main React package is deprecated. Use ' + 'the prop-types package from npm instead.', @@ -126,7 +127,7 @@ if (__DEV__) { Object.keys(ReactDOMFactories).forEach(function(factory) { React.DOM[factory] = function(...args) { if (!warnedForFactories) { - warning( + lowPriorityWarning( false, 'Accessing factories like React.DOM.%s has been deprecated ' + 'and will be removed in the future. Use the ' + diff --git a/src/isomorphic/__tests__/React-test.js b/src/isomorphic/__tests__/React-test.js index c2098227e5e83..7639bb107529b 100644 --- a/src/isomorphic/__tests__/React-test.js +++ b/src/isomorphic/__tests__/React-test.js @@ -19,21 +19,21 @@ describe('React', () => { }); it('should log a deprecation warning once when using React.__spread', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); React.__spread({}); React.__spread({}); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe(1); + expect(console.warn.calls.argsFor(0)[0]).toContain( 'React.__spread is deprecated and should not be used', ); }); it('should log a deprecation warning once when using React.createMixin', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); React.createMixin(); React.createMixin(); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe(1); + expect(console.warn.calls.argsFor(0)[0]).toContain( 'React.createMixin is deprecated and should not be used', ); }); diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index f7ee32194eab9..30d40cd1fab89 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -26,7 +26,14 @@ var checkReactTypeSpec = require('checkReactTypeSpec'); var canDefineProperty = require('canDefineProperty'); var getIteratorFn = require('getIteratorFn'); -var warning = require('warning'); +var warning = require('fbjs/lib/warning'); + +if (__DEV__) { + var checkPropTypes = require('prop-types/checkPropTypes'); + var lowPriorityWarning = require('lowPriorityWarning'); + var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame'); + var {getCurrentStackAddendum} = require('ReactComponentTreeHook'); +} function getDeclarationErrorAddendum() { if (ReactCurrentOwner.current) { @@ -275,7 +282,7 @@ var ReactElementValidator = { Object.defineProperty(validatedFactory, 'type', { enumerable: false, get: function() { - warning( + lowPriorityWarning( false, 'Factory.type is deprecated. Access the class directly ' + 'before passing it to createFactory.', diff --git a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js index 26e1c7217905d..a772395aa6a2f 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js @@ -444,20 +444,20 @@ describe('ReactElementValidator', () => { }); it('should warn when accessing .type on an element factory', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); function TestComponent() { return
; } var TestFactory = React.createFactory(TestComponent); expect(TestFactory.type).toBe(TestComponent); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toBe( + expect(console.warn.calls.count()).toBe(1); + expect(console.warn.calls.argsFor(0)[0]).toBe( 'Warning: Factory.type is deprecated. Access the class directly before ' + 'passing it to createFactory.', ); // Warn once, not again expect(TestFactory.type).toBe(TestComponent); - expect(console.error.calls.count()).toBe(1); + expect(console.warn.calls.count()).toBe(1); }); it('does not warn when using DOM node as children', () => { diff --git a/src/isomorphic/modern/class/ReactComponent.js b/src/isomorphic/modern/class/ReactComponent.js index f0ab6fd090cac..aa03ca82045ce 100644 --- a/src/isomorphic/modern/class/ReactComponent.js +++ b/src/isomorphic/modern/class/ReactComponent.js @@ -16,7 +16,7 @@ var ReactNoopUpdateQueue = require('ReactNoopUpdateQueue'); var canDefineProperty = require('canDefineProperty'); var emptyObject = require('emptyObject'); var invariant = require('invariant'); -var warning = require('warning'); +var lowPriorityWarning = require('lowPriorityWarning'); /** * Base class helpers for the updating state of a component. @@ -114,7 +114,7 @@ if (__DEV__) { if (canDefineProperty) { Object.defineProperty(ReactComponent.prototype, methodName, { get: function() { - warning( + lowPriorityWarning( false, '%s(...) is deprecated in plain JavaScript React classes. %s', info[0], diff --git a/src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee b/src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee index 2e4c19c3c2ac4..23661be18e54a 100644 --- a/src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee @@ -378,16 +378,16 @@ describe 'ReactCoffeeScriptClass', -> undefined it 'should throw AND warn when trying to access classic APIs', -> - spyOn console, 'error' + spyOn console, 'warn' instance = test Inner(name: 'foo'), 'DIV', 'foo' expect(-> instance.replaceState {}).toThrow() expect(-> instance.isMounted()).toThrow() - expect(console.error.calls.count()).toBe 2 - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe 2 + expect(console.warn.calls.argsFor(0)[0]).toContain( 'replaceState(...) is deprecated in plain JavaScript React classes' ) - expect(console.error.calls.argsFor(1)[0]).toContain( + expect(console.warn.calls.argsFor(1)[0]).toContain( 'isMounted(...) is deprecated in plain JavaScript React classes' ) undefined diff --git a/src/isomorphic/modern/class/__tests__/ReactES6Class-test.js b/src/isomorphic/modern/class/__tests__/ReactES6Class-test.js index 2cad6c5dc6e13..ee63a7af70ab8 100644 --- a/src/isomorphic/modern/class/__tests__/ReactES6Class-test.js +++ b/src/isomorphic/modern/class/__tests__/ReactES6Class-test.js @@ -408,15 +408,15 @@ describe('ReactES6Class', () => { }); it('should throw AND warn when trying to access classic APIs', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); var instance = test(, 'DIV', 'foo'); expect(() => instance.replaceState({})).toThrow(); expect(() => instance.isMounted()).toThrow(); - expect(console.error.calls.count()).toBe(2); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe(2); + expect(console.warn.calls.argsFor(0)[0]).toContain( 'replaceState(...) is deprecated in plain JavaScript React classes', ); - expect(console.error.calls.argsFor(1)[0]).toContain( + expect(console.warn.calls.argsFor(1)[0]).toContain( 'isMounted(...) is deprecated in plain JavaScript React classes', ); }); diff --git a/src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts b/src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts index 6f4a2d0faf7b0..d7de7469d3433 100644 --- a/src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts +++ b/src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts @@ -502,18 +502,18 @@ describe('ReactTypeScriptClass', function() { }); it('should throw AND warn when trying to access classic APIs', function() { - spyOn(console, 'error'); + spyOn(console, 'warn'); var instance = test( React.createElement(Inner, {name: 'foo'}), 'DIV','foo' ); expect(() => instance.replaceState({})).toThrow(); expect(() => instance.isMounted()).toThrow(); - expect((console.error).calls.count()).toBe(2); - expect((console.error).calls.argsFor(0)[0]).toContain( + expect((console.warn).calls.count()).toBe(2); + expect((console.warn).calls.argsFor(0)[0]).toContain( 'replaceState(...) is deprecated in plain JavaScript React classes' ); - expect((console.error).calls.argsFor(1)[0]).toContain( + expect((console.warn).calls.argsFor(1)[0]).toContain( 'isMounted(...) is deprecated in plain JavaScript React classes' ); }); diff --git a/src/renderers/shared/ReactPerf.js b/src/renderers/shared/ReactPerf.js index 7f65b170b99ee..17abb58095e75 100644 --- a/src/renderers/shared/ReactPerf.js +++ b/src/renderers/shared/ReactPerf.js @@ -13,7 +13,7 @@ 'use strict'; var ReactDebugTool = require('ReactDebugTool'); -var warning = require('warning'); +var lowPriorityWarning = require('lowPriorityWarning'); var alreadyWarned = false; import type {FlushHistory} from 'ReactDebugTool'; @@ -390,7 +390,7 @@ function printOperations(flushHistory?: FlushHistory) { var warnedAboutPrintDOM = false; function printDOM(measurements: FlushHistory) { - warning( + lowPriorityWarning( warnedAboutPrintDOM, '`ReactPerf.printDOM(...)` is deprecated. Use ' + '`ReactPerf.printOperations(...)` instead.', @@ -401,7 +401,7 @@ function printDOM(measurements: FlushHistory) { var warnedAboutGetMeasurementsSummaryMap = false; function getMeasurementsSummaryMap(measurements: FlushHistory) { - warning( + lowPriorityWarning( warnedAboutGetMeasurementsSummaryMap, '`ReactPerf.getMeasurementsSummaryMap(...)` is deprecated. Use ' + '`ReactPerf.getWasted(...)` instead.', diff --git a/src/renderers/shared/__tests__/ReactPerf-test.js b/src/renderers/shared/__tests__/ReactPerf-test.js index fb64a841a7dd2..3c5f3d58e63df 100644 --- a/src/renderers/shared/__tests__/ReactPerf-test.js +++ b/src/renderers/shared/__tests__/ReactPerf-test.js @@ -401,30 +401,30 @@ describe('ReactPerf', () => { it('warns once when using getMeasurementsSummaryMap', () => { var measurements = measure(() => {}); - spyOn(console, 'error'); + spyOn(console, 'warn'); ReactPerf.getMeasurementsSummaryMap(measurements); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe(1); + expect(console.warn.calls.argsFor(0)[0]).toContain( '`ReactPerf.getMeasurementsSummaryMap(...)` is deprecated. Use ' + '`ReactPerf.getWasted(...)` instead.', ); ReactPerf.getMeasurementsSummaryMap(measurements); - expect(console.error.calls.count()).toBe(1); + expect(console.warn.calls.count()).toBe(1); }); it('warns once when using printDOM', () => { var measurements = measure(() => {}); - spyOn(console, 'error'); + spyOn(console, 'warn'); ReactPerf.printDOM(measurements); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(console.warn.calls.count()).toBe(1); + expect(console.warn.calls.argsFor(0)[0]).toContain( '`ReactPerf.printDOM(...)` is deprecated. Use ' + '`ReactPerf.printOperations(...)` instead.', ); ReactPerf.printDOM(measurements); - expect(console.error.calls.count()).toBe(1); + expect(console.warn.calls.count()).toBe(1); }); it('returns isRunning state', () => { diff --git a/src/shared/utils/deprecated.js b/src/shared/utils/deprecated.js index e9158dbc86de6..81e24cfd98d5d 100644 --- a/src/shared/utils/deprecated.js +++ b/src/shared/utils/deprecated.js @@ -12,7 +12,7 @@ 'use strict'; -var warning = require('warning'); +var lowPriorityWarning = require('lowPriorityWarning'); /** * This will log a single deprecation notice per function and forward the call @@ -35,7 +35,7 @@ function deprecated( var warned = false; if (__DEV__) { var newFn = function() { - warning( + lowPriorityWarning( warned, /* eslint-disable no-useless-concat */ // Require examples in this string must be split to prevent React's diff --git a/src/shared/utils/lowPriorityWarning.js b/src/shared/utils/lowPriorityWarning.js new file mode 100644 index 0000000000000..d4deb584dccc8 --- /dev/null +++ b/src/shared/utils/lowPriorityWarning.js @@ -0,0 +1,41 @@ +/** + * Copyright 2014-2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule lowPriorityWarning + */ + +'use strict'; + +/** + * 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. + */ + +var lowPriorityWarning = function() {}; + +if (__DEV__) { + lowPriorityWarning = function(condition, format, ...args) { + var argIndex = 0; + var message = 'Warning: ' + format.replace(/%s/g, () => args[argIndex++]); + if (!condition && typeof console !== 'undefined') { + console.warn(message); + } + }; +} + +module.exports = lowPriorityWarning; From 9a59cce3e709c63019f9f9480c2f0fa0eddfe0e7 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 23 May 2017 10:28:29 -0700 Subject: [PATCH 2/3] Tweaks to get tests passing after cherry-picking PR#9650 **what is the change?:** - adds 'lowPriorityWarning' for deprecation of '__spread' and 'createMixin' - tweaks test to check for 'warn' and not 'error' **why make this change?:** Both these issues were introduced by merge conflict resolution when cherry-picking this change from master onto 15.6. **test plan:** `yarn test` **issue:** --- src/isomorphic/React.js | 5 ++--- src/isomorphic/classic/element/ReactElementValidator.js | 8 +------- src/renderers/dom/client/__tests__/ReactDOM-test.js | 4 ++-- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/isomorphic/React.js b/src/isomorphic/React.js index 1e30e85cc7daf..e816d569ba63d 100644 --- a/src/isomorphic/React.js +++ b/src/isomorphic/React.js @@ -21,7 +21,6 @@ var ReactPropTypes = require('ReactPropTypes'); var ReactVersion = require('ReactVersion'); var onlyChild = require('onlyChild'); -var warning = require('warning'); var createElement = ReactElement.createElement; var createFactory = ReactElement.createFactory; @@ -46,7 +45,7 @@ if (__DEV__) { var warnedForSpread = false; var warnedForCreateMixin = false; __spread = function() { - warning( + lowPriorityWarning( warnedForSpread, 'React.__spread is deprecated and should not be used. Use ' + 'Object.assign directly or another helper function with similar ' + @@ -58,7 +57,7 @@ if (__DEV__) { }; createMixin = function(mixin) { - warning( + lowPriorityWarning( warnedForCreateMixin, 'React.createMixin is deprecated and should not be used. You ' + 'can use this mixin directly instead.', diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 30d40cd1fab89..9e087f8162070 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -27,13 +27,7 @@ var checkReactTypeSpec = require('checkReactTypeSpec'); var canDefineProperty = require('canDefineProperty'); var getIteratorFn = require('getIteratorFn'); var warning = require('fbjs/lib/warning'); - -if (__DEV__) { - var checkPropTypes = require('prop-types/checkPropTypes'); - var lowPriorityWarning = require('lowPriorityWarning'); - var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame'); - var {getCurrentStackAddendum} = require('ReactComponentTreeHook'); -} +var lowPriorityWarning = require('lowPriorityWarning'); function getDeclarationErrorAddendum() { if (ReactCurrentOwner.current) { diff --git a/src/renderers/dom/client/__tests__/ReactDOM-test.js b/src/renderers/dom/client/__tests__/ReactDOM-test.js index e5b143e906975..b55e913ffba97 100644 --- a/src/renderers/dom/client/__tests__/ReactDOM-test.js +++ b/src/renderers/dom/client/__tests__/ReactDOM-test.js @@ -110,10 +110,10 @@ describe('ReactDOM', () => { }); it('throws warning when React.DOM factories are called', () => { - spyOn(console, 'error'); + spyOn(console, 'warn'); var element = React.DOM.div(); expect(element.type).toBe('div'); - expect(console.error.calls.count()).toBe(1); + expect(console.warn.calls.count()).toBe(1); }); it('throws in render() if the mount callback is not a function', () => { From 22c4c5589c80401a70594cfdb02954aac732b3d5 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 23 May 2017 11:50:39 -0700 Subject: [PATCH 3/3] Fix mis-written 'require' for 'warning' module **what is the change?:** Fixes 'warning' to be required from 'warning' **why make this change?:** It was causing the browserify build to crash, because we don't expect to have a path to 'warning'. **test plan:** CI --- src/isomorphic/classic/element/ReactElementValidator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 9e087f8162070..17f0332f8d2e2 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -26,7 +26,7 @@ var checkReactTypeSpec = require('checkReactTypeSpec'); var canDefineProperty = require('canDefineProperty'); var getIteratorFn = require('getIteratorFn'); -var warning = require('fbjs/lib/warning'); +var warning = require('warning'); var lowPriorityWarning = require('lowPriorityWarning'); function getDeclarationErrorAddendum() {