Skip to content

Commit

Permalink
Downgrade deprecation warnings from errors to warnings
Browse files Browse the repository at this point in the history
**RFC:**
Pushing this for feedback while I fix all the failing tests.

**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 - facebook#9395

**test plan:**
`npm run test`
and unit tests for the new modules
and manual testing (WIP)

**issue:**
facebook#9395
  • Loading branch information
flarnie committed May 10, 2017
1 parent 7c1e971 commit 9a6616b
Show file tree
Hide file tree
Showing 17 changed files with 118 additions and 68 deletions.
8 changes: 5 additions & 3 deletions src/addons/__tests__/ReactDOMFactories-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,24 @@ 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();

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',
);
});
Expand Down
12 changes: 6 additions & 6 deletions src/isomorphic/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var createFactory = ReactElement.createFactory;
var cloneElement = ReactElement.cloneElement;

if (__DEV__) {
var warning = require('fbjs/lib/warning');
var lowPriorityWarning = require('lowPriorityWarning');
var canDefineProperty = require('canDefineProperty');
var ReactElementValidator = require('ReactElementValidator');
createElement = ReactElementValidator.createElement;
Expand Down Expand Up @@ -91,7 +91,7 @@ if (__DEV__) {
let warnedForPropTypes = false;

React.createMixin = function(mixin) {
warning(
lowPriorityWarning(
warnedForCreateMixin,
'React.createMixin is deprecated and should not be used. You ' +
'can use this mixin directly instead.',
Expand All @@ -104,7 +104,7 @@ if (__DEV__) {
if (canDefineProperty) {
Object.defineProperty(React, 'checkPropTypes', {
get() {
warning(
lowPriorityWarning(
warnedForCheckPropTypes,
'checkPropTypes has been moved to a separate package. ' +
'Accessing React.checkPropTypes is no longer supported ' +
Expand All @@ -119,7 +119,7 @@ if (__DEV__) {

Object.defineProperty(React, 'createClass', {
get: function() {
warning(
lowPriorityWarning(
warnedForCreateClass,
'React.createClass is no longer supported. Use a plain JavaScript ' +
"class instead. If you're not yet ready to migrate, " +
Expand All @@ -133,7 +133,7 @@ if (__DEV__) {

Object.defineProperty(React, 'PropTypes', {
get() {
warning(
lowPriorityWarning(
warnedForPropTypes,
'PropTypes has been moved to a separate package. ' +
'Accessing React.PropTypes is no longer supported ' +
Expand All @@ -155,7 +155,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 ' +
Expand Down
24 changes: 12 additions & 12 deletions src/isomorphic/__tests__/React-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@ describe('React', () => {
});

it('should log a deprecation warning once when using React.createMixin', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
React.createMixin();
React.createMixin();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'React.createMixin is deprecated and should not be used',
);
});

it('should warn once when attempting to access React.createClass', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
let createClass = React.createClass;
createClass = React.createClass;
expect(createClass).not.toBe(undefined);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'React.createClass is no longer supported. Use a plain JavaScript ' +
"class instead. If you're not yet ready to migrate, " +
'create-react-class is available on npm as a drop-in replacement. ' +
Expand All @@ -43,12 +43,12 @@ describe('React', () => {
});

it('should warn once when attempting to access React.PropTypes', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
let PropTypes = React.PropTypes;
PropTypes = React.PropTypes;
expect(PropTypes).not.toBe(undefined);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'PropTypes has been moved to a separate package. ' +
'Accessing React.PropTypes is no longer supported ' +
'and will be removed completely in React 16. ' +
Expand All @@ -58,12 +58,12 @@ describe('React', () => {
});

it('should warn once when attempting to access React.checkPropTypes', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
let checkPropTypes = React.checkPropTypes;
checkPropTypes = React.checkPropTypes;
expect(checkPropTypes).not.toBe(undefined);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'checkPropTypes has been moved to a separate package. ' +
'Accessing React.checkPropTypes is no longer supported ' +
'and will be removed completely in React 16. ' +
Expand Down
5 changes: 3 additions & 2 deletions src/isomorphic/classic/element/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ var getIteratorFn = require('getIteratorFn');

if (__DEV__) {
var checkPropTypes = require('prop-types/checkPropTypes');
var warning = require('fbjs/lib/warning');
var lowPriorityWarning = require('lowPriorityWarning');
var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame');
var warning = require('fbjs/lib/warning');
var {getCurrentStackAddendum} = require('ReactComponentTreeHook');
}

Expand Down Expand Up @@ -285,7 +286,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.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,20 +441,20 @@ describe('ReactElementValidator', () => {
});

it('should warn when accessing .type on an element factory', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
function TestComponent() {
return <div />;
}
var TestFactory = React.createFactory(TestComponent);
expect(TestFactory.type).toBe(TestComponent);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(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);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.warn.calls.count()).toBe(1);
});

it('does not warn when using DOM node as children', () => {
Expand Down
4 changes: 2 additions & 2 deletions src/isomorphic/modern/class/ReactBaseClasses.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var ReactNoopUpdateQueue = require('ReactNoopUpdateQueue');
var canDefineProperty = require('canDefineProperty');
var emptyObject = require('fbjs/lib/emptyObject');
var invariant = require('fbjs/lib/invariant');
var warning = require('fbjs/lib/warning');
var lowPriorityWarning = require('lowPriorityWarning');

/**
* Base class helpers for the updating state of a component.
Expand Down Expand Up @@ -108,7 +108,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],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -402,15 +402,15 @@ describe('ReactES6Class', () => {
});

it('should throw AND warn when trying to access classic APIs', () => {
spyOn(console, 'error');
spyOn(console, 'warn');
var 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',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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((<any>console.error).calls.count()).toBe(2);
expect((<any>console.error).calls.argsFor(0)[0]).toContain(
expect((<any>console.warn).calls.count()).toBe(2);
expect((<any>console.warn).calls.argsFor(0)[0]).toContain(
'replaceState(...) is deprecated in plain JavaScript React classes'
);
expect((<any>console.error).calls.argsFor(1)[0]).toContain(
expect((<any>console.warn).calls.argsFor(1)[0]).toContain(
'isMounted(...) is deprecated in plain JavaScript React classes'
);
});
Expand Down
12 changes: 6 additions & 6 deletions src/renderers/__tests__/ReactCompositeComponentState-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ describe('ReactCompositeComponent-state', () => {
});

it('should treat assigning to this.state inside cWRP as a replaceState, with a warning', () => {
spyOn(console, 'error');
spyOn(console, 'warn');

let ops = [];
class Test extends React.Component {
Expand Down Expand Up @@ -440,16 +440,16 @@ describe('ReactCompositeComponent-state', () => {
'render -- step: 3, extra: false',
'callback -- step: 3, extra: false',
]);
expect(console.error.calls.count()).toEqual(1);
expect(console.error.calls.argsFor(0)[0]).toEqual(
expect(console.warn.calls.count()).toEqual(1);
expect(console.warn.calls.argsFor(0)[0]).toEqual(
'Warning: Test.componentWillReceiveProps(): Assigning directly to ' +
"this.state is deprecated (except inside a component's constructor). " +
'Use setState instead.',
);
});

it('should treat assigning to this.state inside cWM as a replaceState, with a warning', () => {
spyOn(console, 'error');
spyOn(console, 'warn');

let ops = [];
class Test extends React.Component {
Expand Down Expand Up @@ -480,8 +480,8 @@ describe('ReactCompositeComponent-state', () => {
'render -- step: 3, extra: false',
'callback -- step: 3, extra: false',
]);
expect(console.error.calls.count()).toEqual(1);
expect(console.error.calls.argsFor(0)[0]).toEqual(
expect(console.warn.calls.count()).toEqual(1);
expect(console.warn.calls.argsFor(0)[0]).toEqual(
'Warning: Test.componentWillMount(): Assigning directly to ' +
"this.state is deprecated (except inside a component's constructor). " +
'Use setState instead.',
Expand Down
18 changes: 9 additions & 9 deletions src/renderers/__tests__/ReactPerf-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,30 +406,30 @@ describeStack('ReactPerf', () => {

it('warns once when using getMeasurementsSummaryMap', () => {
var measurements = measure(() => {});
spyOn(console, 'error');
spyOn(console, 'warn');
ReactPerf.getMeasurementsSummaryMap(measurements);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'`ReactPerf.getMeasurementsSummaryMap(...)` is deprecated. Use ' +
'`ReactPerf.getWasted(...)` instead.',
);

ReactPerf.getMeasurementsSummaryMap(measurements);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.warn.calls.count()).toBe(1);
});

it('warns once when using printDOM', () => {
fit('warns once when using printDOM', () => {
var measurements = measure(() => {});
spyOn(console, 'error');
spyOn(console, 'warn');
ReactPerf.printDOM(measurements);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
expectDev(console.warn.calls.count()).toBe(1);
expectDev(console.warn.calls.argsFor(0)[0]).toContain(
'`ReactPerf.printDOM(...)` is deprecated. Use ' +
'`ReactPerf.printOperations(...)` instead.',
);

ReactPerf.printDOM(measurements);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.warn.calls.count()).toBe(1);
});

it('returns isRunning state', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`handles events 1`] = `
"<native root> {}
View {\\"foo\\":\\"outer\\"}
View {\\"foo\\":\\"inner\\"}"
View {"foo":"outer"}
View {"foo":"inner"}"
`;
exports[`test handles events 1`] = `
"<native root> {}
View {\"foo\":\"outer\"}
View {\"foo\":\"inner\"}"
`;
6 changes: 3 additions & 3 deletions src/renderers/shared/ReactPerf.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
'use strict';

var ReactDebugTool = require('ReactDebugTool');
var warning = require('fbjs/lib/warning');
var lowPriorityWarning = require('lowPriorityWarning');
var alreadyWarned = false;

import type {FlushHistory} from 'ReactDebugTool';
Expand Down Expand Up @@ -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.',
Expand All @@ -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.',
Expand Down
Loading

0 comments on commit 9a6616b

Please sign in to comment.