Skip to content

Commit

Permalink
Downgrade deprecation warnings from errors to warnings (#9650)
Browse files Browse the repository at this point in the history
* Initial regeneration of results.json

**what is the change?:**
We ran `yarn build` and updated the perf. stats record.

**why make this change?:**
Some commits have landed without updating this. By getting an initial update, I can run the build script again after my changes and see any size regressions.

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

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

**issue:**
#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:**
#9398

* Make state mutations an error, not low-pri warning

**what is the change?:**
Even though this is a "deprecation" warning, we still want to use 'console.error' for it.

**why make this change?:**
- It's not likely to come up now, hopefully, because this warning has been present for some time
- This will cause real issues in production if ignored

**test plan:**
`yarn test` - we did fix one test which failed bc of this change

**issue:**
#9398

* Fix test of assigning to this.state that was only passing in fiber

**what is the change?:**
updated a unit test for assigning directly to state; it once again raises an error and not a warning.

**why make this change?:**
So that tests pass

**test plan:**
 REACT_DOM_JEST_USE_FIBER=1 yarn run test

**issue:**

* Update results.json
  • Loading branch information
flarnie authored May 23, 2017
1 parent 1f80931 commit 964c263
Show file tree
Hide file tree
Showing 15 changed files with 161 additions and 108 deletions.
1 change: 1 addition & 0 deletions scripts/rollup/modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ function getExternalModules(externals, bundleType, isRenderer) {
case FB_PROD:
fbjsModules.forEach(module => externalModules.push(module));
externalModules.push('ReactCurrentOwner');
externalModules.push('lowPriorityWarning');
if (isRenderer) {
externalModules.push('React');
if (externalModules.indexOf('react-dom') > -1) {
Expand Down
116 changes: 62 additions & 54 deletions scripts/rollup/results.json
Original file line number Diff line number Diff line change
@@ -1,92 +1,92 @@
{
"bundleSizes": {
"react.development.js (UMD_DEV)": {
"size": 123496,
"gzip": 31034
"size": 125119,
"gzip": 31339
},
"react.production.min.js (UMD_PROD)": {
"size": 15753,
"gzip": 5824
},
"react-dom.development.js (UMD_DEV)": {
"size": 592317,
"gzip": 136560
"size": 593408,
"gzip": 136701
},
"react-dom.production.min.js (UMD_PROD)": {
"size": 121979,
"gzip": 38564
"size": 122214,
"gzip": 38682
},
"react-dom-server.development.js (UMD_DEV)": {
"size": 498439,
"gzip": 120624
"size": 525896,
"gzip": 126687
},
"react-dom-server.production.min.js (UMD_PROD)": {
"size": 107169,
"gzip": 33472
"size": 111581,
"gzip": 34959
},
"react-art.development.js (UMD_DEV)": {
"size": 349177,
"gzip": 77902
"size": 349191,
"gzip": 77905
},
"react-art.production.min.js (UMD_PROD)": {
"size": 96267,
"gzip": 29310
"size": 96277,
"gzip": 29313
},
"react.development.js (NODE_DEV)": {
"size": 70293,
"gzip": 17777
"size": 71914,
"gzip": 18276
},
"react.production.min.js (NODE_PROD)": {
"size": 9195,
"gzip": 3614
},
"React-dev.js (FB_DEV)": {
"size": 71576,
"gzip": 18181
"size": 73423,
"gzip": 18751
},
"React-prod.js (FB_PROD)": {
"size": 36456,
"gzip": 9216
"size": 36836,
"gzip": 9248
},
"ReactDOMStack-dev.js (FB_DEV)": {
"size": 494069,
"gzip": 117876
"size": 496986,
"gzip": 118763
},
"ReactDOMStack-prod.js (FB_PROD)": {
"size": 352489,
"gzip": 84575
"size": 353129,
"gzip": 84773
},
"react-dom.development.js (NODE_DEV)": {
"size": 549893,
"gzip": 126464
"size": 550984,
"gzip": 126642
},
"react-dom.production.min.js (NODE_PROD)": {
"size": 118192,
"gzip": 37224
"size": 118427,
"gzip": 37300
},
"ReactDOMFiber-dev.js (FB_DEV)": {
"size": 550815,
"gzip": 126958
"size": 551962,
"gzip": 127141
},
"ReactDOMFiber-prod.js (FB_PROD)": {
"size": 410570,
"gzip": 94005
"size": 411657,
"gzip": 94084
},
"react-dom-server.development.js (NODE_DEV)": {
"size": 447050,
"gzip": 108025
"size": 474459,
"gzip": 114087
},
"react-dom-server.production.min.js (NODE_PROD)": {
"size": 101542,
"gzip": 31424
"size": 105956,
"gzip": 32782
},
"ReactDOMServerStack-dev.js (FB_DEV)": {
"size": 445736,
"gzip": 107878
"size": 455985,
"gzip": 109831
},
"ReactDOMServerStack-prod.js (FB_PROD)": {
"size": 333450,
"gzip": 80260
"size": 334090,
"gzip": 80459
},
"ReactARTStack-dev.js (FB_DEV)": {
"size": 143166,
Expand All @@ -97,20 +97,20 @@
"gzip": 23039
},
"react-art.development.js (NODE_DEV)": {
"size": 270501,
"gzip": 57767
"size": 270515,
"gzip": 57772
},
"react-art.production.min.js (NODE_PROD)": {
"size": 57652,
"gzip": 17386
"size": 57662,
"gzip": 17391
},
"ReactARTFiber-dev.js (FB_DEV)": {
"size": 269759,
"gzip": 57590
"size": 269773,
"gzip": 57593
},
"ReactARTFiber-prod.js (FB_PROD)": {
"size": 208039,
"gzip": 43486
"size": 208053,
"gzip": 43488
},
"ReactNativeStack.js (RN)": {
"size": 233993,
Expand All @@ -121,20 +121,20 @@
"gzip": 84001
},
"ReactTestRendererFiber-dev.js (FB_DEV)": {
"size": 267261,
"gzip": 56462
"size": 267275,
"gzip": 56465
},
"ReactTestRendererStack-dev.js (FB_DEV)": {
"size": 151770,
"gzip": 34846
},
"react-noop-renderer.development.js (NODE_DEV)": {
"size": 259178,
"gzip": 54431
"size": 259192,
"gzip": 54435
},
"react-test-renderer.development.js (NODE_DEV)": {
"size": 268012,
"gzip": 56651
"size": 268026,
"gzip": 56654
},
"react-dom-test-utils.development.js (NODE_DEV)": {
"size": 52792,
Expand All @@ -155,6 +155,14 @@
"ReactShallowRenderer-dev.js (FB_DEV)": {
"size": 8063,
"gzip": 2229
},
"ReactDOMServerStream-dev.js (FB_DEV)": {
"size": 472917,
"gzip": 113925
},
"ReactDOMServerStream-prod.js (FB_PROD)": {
"size": 345920,
"gzip": 83497
}
}
}
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
Loading

0 comments on commit 964c263

Please sign in to comment.