Downgrade deprecation warnings from errors to warnings #9650

flarnie · 2017-05-10T13:27:23Z

Doing this first on master, then will cherry-pick to 15.6 branch.

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
Also planning to do some manual testing and paste in screenshots in comments to show how the changed warnings look.
There are some failing snapshot tests that seem unrelated, but looking into those also.

issue:
#9395

flarnie · 2017-05-10T13:40:44Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -45,6 +45,7 @@ const isArray = Array.isArray;
 if (__DEV__) {
  var {startPhaseTimer, stopPhaseTimer} = require('ReactDebugFiberPerf');
  var warning = require('fbjs/lib/warning');
+  var lowPriorityWarning = requier('lowPriorityWarning');


Going to fix this; requier -> require

flarnie · 2017-05-10T16:45:22Z

This is pretty close - some snapshots are failing and I don't see how this change caused the snapshot to change. Looking into it now.

flarnie · 2017-05-10T16:56:47Z

For some reason those tests are failing for me on master too. Made a separate issue for that. #9653

flarnie · 2017-05-10T17:57:05Z

All tests except those snapshots pass for me locally. Not sure why src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js was failing in CI.

flarnie · 2017-05-11T09:51:11Z

Figured out that the differences are at least in part to the fact that I was using Jest 20 locally. Fixed that and fixing failing tests now.

flarnie · 2017-05-11T13:39:23Z

Rebased and tweaked a couple of thigns, 🤞 should be passing all tests now.

flarnie · 2017-05-11T13:49:57Z

After chatting with @gaearon I'm going to do some internal testing of this to estimate the impact before merging. So no hurry to review.

flarnie · 2017-05-22T16:37:07Z

I'm satisfied with the results of the sync - we can internally set 'lowPriorityWarning' as an external dependency in the rollup build script if that becomes a blocker
This is one of the last 2 things blocking 15.6 release, and I'd like to get an RC out by Weds.

sophiebits

lgtm. What's the deal with the sync though?

Do we need to add lowPriorityWarning to the externals list now? Seems like we won't be able to sync now otherwise.

sophiebits · 2017-05-22T20:59:35Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -316,7 +317,7 @@ module.exports = function(

    if (oldState !== instance.state) {
      if (__DEV__) {
-        warning(
+        lowPriorityWarning(


Can we leave these two (and one in ReactCompositeComponent) as hi-pri? They should be relatively rare anyway and are also more important than the others (IMO) since the breakage would be subtle.

Sure - agreed that this should be rare and would cause bugs.

sophiebits · 2017-05-22T21:01:06Z

src/shared/utils/lowPriorityWarning.js

+    if (!condition && typeof console !== 'undefined') {
+      console.warn(message);
+    }
+  };


Can we keep this logic?

try { // --- Welcome to debugging React --- // This error was thrown as a convenience so that you can use this stack // to find the callsite that caused this warning to fire. throw new Error(message); } catch (x) {}

It might be helpful to people trying to fix warnings. It may also be worth just keeping the other same checks that we had in the original warning.js.

Sure - I will make it a duplicate and just swap 'error' for 'warn'.

flarnie · 2017-05-23T00:58:41Z

Thanks for the review!

"Do we need to add lowPriorityWarning to the externals list now? Seems like we won't be able to sync now otherwise."

Yes and yes. Was going to do this in a separate commit but I can add it to this one. Wouldn't want to block the sync or cause issues with that.

sophiebits · 2017-05-23T01:05:59Z

Sounds great. Let's just do it in this PR if you don't feel strongly.

flarnie · 2017-05-23T02:03:08Z

Assuming no blockers come up I plan to merge this first thing in the morning. :)

flarnie · 2017-05-23T15:09:22Z

Thought I fixed the failing tests - looking into that now.

**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.

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

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

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

**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:**

* 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 - facebook#9395 **test plan:** `npm run test` and unit tests for the new modules and manual testing (WIP) **issue:** facebook#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:** facebook#9398

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

* Downgrade deprecation warnings from errors to warnings (facebook#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 - facebook#9395 **test plan:** `npm run test` and unit tests for the new modules and manual testing (WIP) **issue:** facebook#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:** facebook#9398 * 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:** * 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

flarnie added this to the 15.6 milestone May 10, 2017

flarnie requested review from sophiebits and gaearon May 10, 2017 13:27

facebook-github-bot added the CLA Signed label May 10, 2017

flarnie commented May 10, 2017

View reviewed changes

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch 2 times, most recently from d334ae8 to cdc77b8 Compare May 10, 2017 16:43

flarnie changed the title ~~[RFC] Downgrade deprecation warnings from errors to warnings~~ Downgrade deprecation warnings from errors to warnings May 10, 2017

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch from cdc77b8 to a9e8f44 Compare May 10, 2017 17:54

flarnie mentioned this pull request May 10, 2017

React 15.6 Umbrella #9398

Closed

49 tasks

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch from a9e8f44 to d01c9cd Compare May 11, 2017 13:38

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch from d01c9cd to 11b22d2 Compare May 12, 2017 14:43

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch from 11b22d2 to c201d2a Compare May 22, 2017 16:46

sophiebits approved these changes May 22, 2017

View reviewed changes

flarnie added 5 commits May 23, 2017 09:06

Update results.json

8dd8213

flarnie force-pushed the downgradeDeprecationErrorsToWarnings branch from e83b550 to 8dd8213 Compare May 23, 2017 16:13

flarnie merged commit 964c263 into facebook:master May 23, 2017

flarnie mentioned this pull request May 23, 2017

Improve low priority warning #9754

Merged

alexkrolick mentioned this pull request Jun 1, 2017

Fix: Accessing PropTypes via React package is deprecated zenoamaro/react-quill#191

Closed

flarnie deleted the downgradeDeprecationErrorsToWarnings branch May 25, 2018 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Downgrade deprecation warnings from errors to warnings #9650

Downgrade deprecation warnings from errors to warnings #9650

flarnie commented May 10, 2017 •

edited

Loading

flarnie May 10, 2017

flarnie commented May 10, 2017

flarnie commented May 10, 2017

flarnie commented May 10, 2017

flarnie commented May 11, 2017

flarnie commented May 11, 2017

flarnie commented May 11, 2017

flarnie commented May 22, 2017

sophiebits left a comment

sophiebits May 22, 2017

flarnie May 23, 2017

sophiebits May 22, 2017

flarnie May 23, 2017

flarnie commented May 23, 2017

sophiebits commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 23, 2017

Downgrade deprecation warnings from errors to warnings #9650

Downgrade deprecation warnings from errors to warnings #9650

Conversation

flarnie commented May 10, 2017 • edited Loading

flarnie May 10, 2017

Choose a reason for hiding this comment

flarnie commented May 10, 2017

flarnie commented May 10, 2017

flarnie commented May 10, 2017

flarnie commented May 11, 2017

flarnie commented May 11, 2017

flarnie commented May 11, 2017

flarnie commented May 22, 2017

sophiebits left a comment

Choose a reason for hiding this comment

sophiebits May 22, 2017

Choose a reason for hiding this comment

flarnie May 23, 2017

Choose a reason for hiding this comment

sophiebits May 22, 2017

Choose a reason for hiding this comment

flarnie May 23, 2017

Choose a reason for hiding this comment

flarnie commented May 23, 2017

sophiebits commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 10, 2017 •

edited

Loading