Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add legacy context API warning in strict mode #12849

Merged
merged 7 commits into from
May 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,11 @@ function mountClassInstance(
workInProgress,
instance,
);

ReactStrictModeWarnings.recordLegacyContextWarning(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

workInProgress,
instance,
);
}

if (warnAboutDeprecatedLifecycles) {
Expand Down
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
enableUserTimingAPI,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
warnAboutDeprecatedLifecycles,
warnAboutLegacyContextAPI,
} from 'shared/ReactFeatureFlags';
import getComponentName from 'shared/getComponentName';
import invariant from 'fbjs/lib/invariant';
Expand Down Expand Up @@ -439,6 +440,10 @@ function commitAllLifeCycles(
if (warnAboutDeprecatedLifecycles) {
ReactStrictModeWarnings.flushPendingDeprecationWarnings();
}

if (warnAboutLegacyContextAPI) {
ReactStrictModeWarnings.flushLegacyContextWarning();
}
}
while (nextEffect !== null) {
const effectTag = nextEffect.effectTag;
Expand Down
68 changes: 68 additions & 0 deletions packages/react-reconciler/src/ReactStrictModeWarnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,17 @@ type LIFECYCLE =
| 'UNSAFE_componentWillUpdate';
type LifecycleToComponentsMap = {[lifecycle: LIFECYCLE]: Array<Fiber>};
type FiberToLifecycleMap = Map<Fiber, LifecycleToComponentsMap>;
type FiberArray = Array<Fiber>;
type FiberToFiberComponentsMap = Map<Fiber, FiberArray>;

const ReactStrictModeWarnings = {
discardPendingWarnings(): void {},
flushPendingDeprecationWarnings(): void {},
flushPendingUnsafeLifecycleWarnings(): void {},
recordDeprecationWarnings(fiber: Fiber, instance: any): void {},
recordUnsafeLifecycleWarnings(fiber: Fiber, instance: any): void {},
recordLegacyContextWarning(fiber: Fiber, instance: any): void {},
flushLegacyContextWarning(): void {},
};

if (__DEV__) {
Expand All @@ -41,10 +45,12 @@ if (__DEV__) {
let pendingComponentWillReceivePropsWarnings: Array<Fiber> = [];
let pendingComponentWillUpdateWarnings: Array<Fiber> = [];
let pendingUnsafeLifecycleWarnings: FiberToLifecycleMap = new Map();
let pendingLegacyContextWarning: FiberToFiberComponentsMap = new Map();

// Tracks components we have already warned about.
const didWarnAboutDeprecatedLifecycles = new Set();
const didWarnAboutUnsafeLifecycles = new Set();
const didWarnAboutLegacyContext = new Set();

const setToSortedString = set => {
const array = [];
Expand All @@ -59,6 +65,7 @@ if (__DEV__) {
pendingComponentWillReceivePropsWarnings = [];
pendingComponentWillUpdateWarnings = [];
pendingUnsafeLifecycleWarnings = new Map();
pendingLegacyContextWarning = new Map();
};

ReactStrictModeWarnings.flushPendingUnsafeLifecycleWarnings = () => {
Expand Down Expand Up @@ -289,6 +296,67 @@ if (__DEV__) {
});
}
};

ReactStrictModeWarnings.recordLegacyContextWarning = (
fiber: Fiber,
instance: any,
) => {
const strictRoot = findStrictRoot(fiber);
if (strictRoot === null) {
warning(
false,
'Expected to find a StrictMode component in a strict mode tree. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
return;
}

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

let warningsForRoot = pendingLegacyContextWarning.get(strictRoot);

if (
typeof instance.getChildContext === 'function' ||
fiber.type.contextTypes != null ||
fiber.type.childContextTypes != null
) {
if (warningsForRoot === undefined) {
warningsForRoot = [];
pendingLegacyContextWarning.set(strictRoot, warningsForRoot);
}
warningsForRoot.push(fiber);
}
};

ReactStrictModeWarnings.flushLegacyContextWarning = () => {
((pendingLegacyContextWarning: any): FiberToFiberComponentsMap).forEach(
(fiberArray: FiberArray, strictRoot) => {
const uniqueNames = new Set();
fiberArray.forEach(fiber => {
uniqueNames.add(getComponentName(fiber) || 'Component');
didWarnAboutLegacyContext.add(fiber.type);
});

const sortedNames = setToSortedString(uniqueNames);
const strictRootComponentStack = getStackAddendumByWorkInProgressFiber(
strictRoot,
);

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

export default ReactStrictModeWarnings;
69 changes: 69 additions & 0 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
let React;
let ReactFeatureFlags;
let ReactTestRenderer;
let PropTypes;

describe('ReactStrictMode', () => {
describe('debugRenderPhaseSideEffects', () => {
Expand Down Expand Up @@ -805,4 +806,72 @@ describe('ReactStrictMode', () => {
renderer.update(<OuterComponent />);
});
});

describe('context legacy', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactTestRenderer = require('react-test-renderer');
PropTypes = require('prop-types');
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.warnAboutLegacyContextAPI = true;
});

it('should warn if the legacy context API have been used in strict mode', () => {
class LegacyContextProvider extends React.Component {
getChildContext() {
return {color: 'purple'};
}

render() {
return <LegacyContextConsumer />;
}
}

LegacyContextProvider.childContextTypes = {
color: PropTypes.string,
};

class LegacyContextConsumer extends React.Component {
render() {
return null;
}
}

const {StrictMode} = React;

class Root extends React.Component {
render() {
return (
<div>
<StrictMode>
<LegacyContextProvider />
</StrictMode>
</div>
);
}
}

LegacyContextConsumer.contextTypes = {
color: PropTypes.string,
};

let rendered;

expect(() => {
rendered = ReactTestRenderer.create(<Root />);
}).toWarnDev(
'Warning: Legacy context API has been detected within a strict-mode tree: ' +
'\n in div (at **)' +
'\n in Root (at **)' +
'\n\nPlease update the following components: LegacyContextConsumer, LegacyContextProvider' +
'\n\nLearn more about this warning here:' +
'\nhttps://fb.me/react-strict-mode-warnings',
);

// Dedupe
rendered = ReactTestRenderer.create(<Root />);
rendered.update(<Root />);
});
});
});
3 changes: 3 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
// Warn about deprecated, async-unsafe lifecycles; relates to RFC #6:
export const warnAboutDeprecatedLifecycles = false;

// Warn about legacy context API
export const warnAboutLegacyContextAPI = false;

// Gather advanced timing metrics for Profiler subtrees.
export const enableProfilerTimer = __DEV__;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const enableUserTimingAPI = __DEV__;
export const enableGetDerivedStateFromCatch = false;
export const enableSuspense = false;
export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = __DEV__;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const enableProfilerTimer = __DEV__;
export const fireGetDerivedStateFromPropsOnStateUpdates = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const enableUserTimingAPI = __DEV__;
export const enableGetDerivedStateFromCatch = false;
export const enableSuspense = false;
export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const enableProfilerTimer = false;
export const fireGetDerivedStateFromPropsOnStateUpdates = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const {

// The rest of the flags are static for better dead code elimination.
export const enableUserTimingAPI = __DEV__;
export const warnAboutLegacyContextAPI = __DEV__;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const enableSuspense = false;
export const enableUserTimingAPI = __DEV__;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = false;
export const enableProfilerTimer = __DEV__;
export const fireGetDerivedStateFromPropsOnStateUpdates = true;

Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.persistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const enableUserTimingAPI = __DEV__;
export const enableGetDerivedStateFromCatch = false;
export const enableSuspense = false;
export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const enableProfilerTimer = false;
export const fireGetDerivedStateFromPropsOnStateUpdates = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const enableUserTimingAPI = __DEV__;
export const enableGetDerivedStateFromCatch = false;
export const enableSuspense = false;
export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
export const enableProfilerTimer = false;
export const fireGetDerivedStateFromPropsOnStateUpdates = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const {
} = require('ReactFeatureFlags');

// The rest of the flags are static for better dead code elimination.
export const warnAboutLegacyContextAPI = __DEV__;

// In www, we have experimental support for gathering data
// from User Timing API calls in production. By default, we
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/shims/react-native-fb/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const ReactFeatureFlags = {
debugRenderPhaseSideEffects: false,
debugRenderPhaseSideEffectsForStrictMode: false,
warnAboutDeprecatedLifecycles: true,
warnAboutLegacyContextAPI: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@bvaughn bvaughn May 21, 2018

Choose a reason for hiding this comment

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

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__;

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

};

module.exports = ReactFeatureFlags;