add legacy context API warning in strict mode #12849

cyan33 · 2018-05-17T20:42:44Z

This PR adds warning on the legacy context APIs when it's under StrictMode. Specifically, just to make sure, the if statement looks like this:

if (
  typeof instance.getChildContext === 'function' ||
  (fiber.tag === ClassComponent && fiber.type.contextTypes != null) ||
  (fiber.tag === ClassComponent && fiber.type.childContextTypes != null)
)

Feel free to leave comments.

acdlite · 2018-05-17T20:46:07Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+
+    if (
+      typeof instance.getChildContext === 'function' ||
+      (fiber.tag === ClassComponent && fiber.type.contextTypes != null) ||


You shouldn't have to check the tag since this is already part of a class-only code path.

So should fiber.tag === ClassComponent be removed in this case?

pull-bot · 2018-05-17T20:48:10Z

Details of bundled changes.

Comparing: 397d611...1fe6316

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.3%	+0.2%	617.64 KB	619.75 KB	143.99 KB	144.24 KB	UMD_DEV
react-dom.development.js	+0.3%	+0.2%	602.01 KB	604.11 KB	139.99 KB	140.25 KB	NODE_DEV
react-dom-server.browser.development.js	0.0%	0.0%	101.51 KB	101.54 KB	26.47 KB	26.48 KB	UMD_DEV
react-dom-server.browser.development.js	0.0%	+0.1%	90.81 KB	90.84 KB	24.22 KB	24.23 KB	NODE_DEV
react-dom-server.node.development.js	0.0%	+0.1%	92.73 KB	92.77 KB	24.76 KB	24.77 KB	NODE_DEV
ReactDOM-dev.js	+0.4%	+0.3%	623.77 KB	626.36 KB	142.63 KB	143 KB	FB_WWW_DEV
ReactDOMServer-dev.js	+0.4%	+0.6%	94.27 KB	94.62 KB	23.95 KB	24.1 KB	FB_WWW_DEV

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.5%	+0.3%	400.77 KB	402.87 KB	89.75 KB	90 KB	UMD_DEV
react-art.development.js	+0.6%	+0.4%	326.62 KB	328.73 KB	70.83 KB	71.09 KB	NODE_DEV
ReactART-dev.js	+0.8%	+0.5%	331.62 KB	334.21 KB	70.12 KB	70.5 KB	FB_WWW_DEV

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.6%	+0.3%	336.61 KB	338.68 KB	72.95 KB	73.19 KB	UMD_DEV
react-test-renderer.development.js	+0.6%	+0.4%	327.45 KB	329.52 KB	70.2 KB	70.45 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.7%	+0.4%	333.1 KB	335.34 KB	69.79 KB	70.05 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.7%	+0.4%	321.1 KB	323.2 KB	68.18 KB	68.45 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.6%	+0.4%	319.69 KB	321.75 KB	67.6 KB	67.84 KB	NODE_DEV

react

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react.development.js	+0.1%	+0.1%	57.11 KB	57.14 KB	15.87 KB	15.88 KB	UMD_DEV
react.development.js	+0.1%	+0.1%	47.75 KB	47.78 KB	13.53 KB	13.54 KB	NODE_DEV
React-dev.js	+0.7%	+1.2%	47.64 KB	48 KB	13.04 KB	13.19 KB	FB_WWW_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.5%	+0.3%	454.47 KB	456.71 KB	99.59 KB	99.85 KB	RN_FB_DEV
ReactNativeRenderer-dev.js	+0.5%	+0.3%	454.13 KB	456.37 KB	99.52 KB	99.78 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.5%	+0.3%	445.28 KB	447.52 KB	97.29 KB	97.54 KB	RN_FB_DEV
ReactFabric-dev.js	+0.5%	+0.3%	445.32 KB	447.56 KB	97.31 KB	97.56 KB	RN_OSS_DEV

Generated by 🚫 dangerJS

acdlite

Looks good to me but please get approval from @bvaughn, too

acdlite · 2018-05-17T22:02:48Z

Huh, why did the bundle size go up?

bvaughn · 2018-05-17T22:25:38Z

packages/shared/ReactFeatureFlags.js

@@ -39,6 +39,9 @@ export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
 // Warn about deprecated, async-unsafe lifecycles; relates to RFC #6:
 export const warnAboutDeprecatedLifecycles = false;

+// Warn about legacy context APIs
+export const warnAboutLegacyContextAPIs = false;


This new feature flag should be added to the other *FeatureFlag files as well. Flow should warn about this.

Just have added to other files. Now running yarn flow gives out 0 errors 😄

bvaughn · 2018-05-17T22:26:29Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+          'which are subjected to be removed in the future. Please switch to the new ones: ' +
+          '\n\n%s' +
+          '\n\nLearn more about this warning here:' +
+          '', // redirection link goes here


Add a link to https://fb.me/react-strict-mode-warnings

bvaughn · 2018-05-17T22:30:17Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+      lowPriorityWarning(
+        false,
+        'Below are the components that are using legacy context API, ' +
+          'which are subjected to be removed in the future. Please switch to the new ones: ' +


Couple of thoughts:

We should probably add the strict root component stack (like we do above with the unsafe lifecycle warning).

Why low priority warning instead of a warning?

I'm not sure this warning message matches the tone of our other warnings. Maybe something like...

warning( false, 'Legacy context usage has been detected within a strict-mode tree:%s' + '\n\n%s' + '\n\nLearn more about this warning here:' + '\nhttps://fb.me/react-strict-mode-warnings', strictRootComponentStack, sortedNames, );

What are your thoughts about this proposed wording change, @cyan33 ?

Sorry I missed this comment.

Consistence wise, I agree. But do we really need to show the component stack? I think the stack information sometimes could easily be noise.

I'm not quite sure, because I have seen both lowPriorityWarning and warning in this file and I thought priority-wise this is the same with deprecated lifecycles. But we can definitely change it to warning, if it needs more attention from developers.

I agree that this wording is better. Will change a bit later.

I think the Profiler component stack could be useful. There's a precedent of including that for other warnings. But I wouldn't say it was necessary.

The suggestion of warning vs lowPriorityWarning was more of a consistency with other, pre-existing StrictMode warnings.

cyan33 · 2018-05-18T07:04:13Z

I've further refactored the wording and change it to warning according to @bvaughn 's comment. Don't know if should print out the strict component stack though. What's your opinion? @acdlite

bvaughn · 2018-05-18T15:40:42Z

Legacy context API has been detected within these components

I think the new message is a little confusing. Why is this bad? Why is it a warning?

Legacy context usage has been detected within a strict-mode tree

I think this message is also a little vague, but at least explicitly mentioning strict-mode is a tip about why it's important. Thoughts?

cyan33 · 2018-05-18T16:34:28Z

at least explicitly mentioning strict-mode is a tip about why it's important.

My initial thought is to display the name of the components themselves that have the legacy API inside, not the StrictMode wrapper. (But we can do both if necessary) So I didn't add any wording related to strict mode. (On second thought tho, explicitly mentioning strict mode does make sense to me)

Why is it a warning?

To keep the consistency with the warnings of lifecycle methods.

I'm still not one hundred percent sure about what's the best warning practice in React. So I'd be much appreciated to hear more suggestions on it. @bvaughn

bvaughn · 2018-05-18T16:40:53Z

My initial thought is to display the name of the components themselves that have the legacy API inside, not the StrictMode wrapper.

This is definitely a good idea! StrictMode might wrap a lot of components, so we definitely want to call out specific ones by name.

I think it's also helpful to mention strict mode in the warning message too though, because it will remind users why we are warning about legacy context (when it has not even been deprecated).

I think it's also helpful to show the strict mode component stack, in case a component is used in multiple places (both inside of and outside of a StrictMode). This will tip users off to where the component is within their app that's causing the warning to be displayed.

To keep the consistency with the warnings of lifecycle methods.

Yeah, sorry. This was a rhetorical question. I was saying that users might ask themselves this when they read the current message. 😄

cyan33 · 2018-05-18T16:44:56Z

@bvaughn got what you meant. I'll update the PR in a while. Thanks for the explanation! 👍

cyan33 · 2018-05-18T21:50:22Z

@bvaughn @acdlite Please have a final quick review and the screenshot has also been updated at the top description of this PR. Much appreciated! 😄

Also, the format checks seem to have failed on the warnings and the error log seems quite elusive to me. Any clues?

gaearon · 2018-05-19T12:56:27Z

You need to run yarn prettier locally and then commit and push.

cyan33 · 2018-05-19T16:48:02Z

@gaearon Thanks Dan. I think I need to re-familiarize myself with the contributing doc 👍

bvaughn · 2018-05-21T14:41:11Z

@cyan33 A helpful one-liner to run before committing:

yarn prettier && yarn linc && yarn flow dom

(You might change flow dom to something else if you're working on a different renderer.)

bvaughn

This looks good to me. I left a few suggestions, mostly minor. 👍

bvaughn · 2018-05-21T14:45:18Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+    if (didWarnAboutUnsafeLifecycles.has(fiber.type)) {
+      return;
+    }
+


Why did these lines move above the StrictRoot ancestor check? I don't think they should have.

I move it upwards intentionally. Because I was thinking that maybe we should not spare time to find its strict node if we already warned this fiber? in which case the findStrictNode becomes extraneous operations.

Tell me if I was wrong.

Generally, I think error sanity checks (like the StrictRoot check) belong first in the function. In this case, I guess I agree that it doesn't matter practically speaking.

Your call 😄

That makes sense to me now. I agree.

bvaughn · 2018-05-21T14:46:48Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+    // Dedup strategy: Warn once per component.
+    if (didWarnAboutLegacyContext.has(fiber.type)) {
+      return;
+    }


nit: Should the StrictMode ancestor check (below) come before this early return?

bvaughn · 2018-05-21T14:48:24Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+    if (warningsForRoot === undefined) {
+      warningsForRoot = [];
+      pendingLegacyContextWarning.set(strictRoot, warningsForRoot);
+    }


Can we not initialize the Set until we actually have something to warn about? (In other words, can we move this initialization into the conditional below?) Seems like that would be a little better for perf.

I think this would also let us skip the fiberArray.length > 0 check in the forEach below.

That's a good point.

bvaughn · 2018-05-21T14:49:07Z

packages/react-reconciler/src/ReactStrictModeWarnings.js

+      fiber.type.childContextTypes != null
+    ) {
+      warningsForRoot.push(fiber);
+      pendingLegacyContextWarning.set(strictRoot, warningsForRoot);


nit: We don't actually need to re-set this in the Map do we?

Is it because when we do let warningsForRoot = pendingLegacyContextWarning.get(strictRoot);

and warningsForRoot.push is mutating the content in pendingLegacyContextWarning?

I thought it was creating a new copy. I'll delete this line.

Right. You're updating the reference that's already stored in the Map.

bvaughn · 2018-05-21T14:52:43Z

scripts/rollup/shims/react-native-fb/ReactFeatureFlags.js

@@ -14,6 +14,7 @@ const ReactFeatureFlags = {
  debugRenderPhaseSideEffects: false,
  debugRenderPhaseSideEffectsForStrictMode: false,
  warnAboutDeprecatedLifecycles: true,
+  warnAboutLegacyContextAPI: true,


Curiosity question: Did we intentionally turn this on for RN renderer but not but for www? (It's probably good to turn it on for both.)

I'm not sure about this. Maybe ask @acdlite 😄

Let's turn it on for the following feature flag forks:

ReactFeatureFlags.native-fb.js

ReactFeatureFlags.native-fabric-fb.js

ReactFeatureFlags.www.js

export const warnAboutLegacyContextAPI = __DEV__;

If we realize this causes problems during the next www sync (which I'll be running) we can back it off then.

Why are they ever off? It's only read in strict mode, yeah? My understanding was that these warnAbout- feature flags only exist so we can turn them off in certain tests.

The two side effect flags are controlled by GKs internally. They're off by default, but can be turned on by injection. (xplat feature flags can't directly reference MobileConfig for GKs like www can.)

bvaughn · 2018-05-21T16:53:14Z

packages/react/src/React.js

@@ -14,7 +14,7 @@ import {
  REACT_STRICT_MODE_TYPE,
  REACT_TIMEOUT_TYPE,
 } from 'shared/ReactSymbols';
-import {enableSuspense} from 'shared/ReactFeatureFlags';
+// import {enableSuspense} from 'shared/ReactFeatureFlags';


Accidental?

bvaughn · 2018-05-21T16:53:20Z

packages/react/src/React.js

@@ -70,7 +70,7 @@ const React = {
  },
 };

-if (enableSuspense) {
+if (true) {


Revert this?

gaearon · 2018-05-23T12:26:51Z

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -668,6 +668,11 @@ function mountClassInstance(
        workInProgress,
        instance,
      );
+
+      ReactStrictModeWarnings.recordLegacyContextWarning(


Looks like we're not catching functional components here. They can also have contextTypes and should also be warned about.

NE-SmallTown · 2018-08-09T02:36:03Z

@gaearon Could we add yarn prettier to pre-commit hook to avoid we forget it?

add legacy context APIs warning in strict mode

ac72ea0

cyan33 requested review from bvaughn and acdlite May 17, 2018 20:42

facebook-github-bot added the CLA Signed label May 17, 2018

acdlite reviewed May 17, 2018

View reviewed changes

cyan33 mentioned this pull request May 17, 2018

add #Detecting legacy context apis for strict mode post reactjs/react.dev#891

Merged

acdlite approved these changes May 17, 2018

View reviewed changes

refactor if statement and the warning message

ca4b796

bvaughn reviewed May 17, 2018

View reviewed changes

add other flags for type check

dc9480a

cyan33 changed the title ~~add legacy context APIs warning in strict mode~~ add legacy context API warning in strict mode May 17, 2018

merge from master

33c9ad1

add component stack tree and refactor wording

7e3e547

pull upstream master

740f21f

bvaughn approved these changes May 21, 2018

View reviewed changes

bvaughn reviewed May 21, 2018

View reviewed changes

packages/react/src/React.js Outdated

@@ -70,7 +70,7 @@ const React = {

},

};

if (enableSuspense) {

if (true) {

Copy link

Contributor

bvaughn May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this?

fix the nits

1fe6316

bvaughn approved these changes May 21, 2018

View reviewed changes

cyan33 merged commit 7350358 into facebook:master May 22, 2018

gaearon reviewed May 23, 2018

View reviewed changes

add legacy context API warning in strict mode #12849

add legacy context API warning in strict mode #12849

Conversation

cyan33 commented May 17, 2018 • edited

acdlite May 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented May 17, 2018 • edited

react-dom

react-art

react-test-renderer

react-reconciler

react

react-native-renderer

acdlite left a comment

Choose a reason for hiding this comment

acdlite commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyan33 commented May 18, 2018 • edited

bvaughn commented May 18, 2018

cyan33 commented May 18, 2018 • edited

bvaughn commented May 18, 2018 • edited

cyan33 commented May 18, 2018

cyan33 commented May 18, 2018 • edited

gaearon commented May 19, 2018

cyan33 commented May 19, 2018

bvaughn commented May 21, 2018

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NE-SmallTown commented Aug 9, 2018 • edited

cyan33 commented May 17, 2018 •

edited

acdlite May 17, 2018 •

edited

pull-bot commented May 17, 2018 •

edited

cyan33 commented May 18, 2018 •

edited

cyan33 commented May 18, 2018 •

edited

bvaughn commented May 18, 2018 •

edited

cyan33 commented May 18, 2018 •

edited

bvaughn May 21, 2018 •

edited

NE-SmallTown commented Aug 9, 2018 •

edited