From dc78a671b87d4a95f9dc587fd3abd6cb5ca8580e Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Fri, 30 Aug 2019 08:54:44 +0200 Subject: [PATCH 1/9] Added hasLegacyContext check. --- .../src/backend/renderer.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index e8a6d8ee8d5af..967e92b61eb59 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -2107,6 +2107,8 @@ export function attach( type, } = fiber; + const elementType = getElementTypeForFiber(fiber); + const usesHooks = (tag === FunctionComponent || tag === SimpleMemoComponent || @@ -2128,7 +2130,13 @@ export function attach( ) { canViewSource = true; if (stateNode && stateNode.context != null) { - context = stateNode.context; + const shouldHideContext = + elementType === ElementTypeClass && + !(type.contextTypes || type.contextType); + + if (!shouldHideContext) { + context = stateNode.context; + } } } else if ( typeSymbol === CONTEXT_CONSUMER_NUMBER || @@ -2166,7 +2174,10 @@ export function attach( } } + let hasLegacyContext = false; if (context !== null) { + hasLegacyContext = !!type.contextTypes; + // To simplify hydration and display logic for context, wrap in a value object. // Otherwise simple values (e.g. strings, booleans) become harder to handle. context = {value: context}; @@ -2238,8 +2249,10 @@ export function attach( // Can view component source location. canViewSource, + hasLegacyContext, + displayName: getDisplayNameForFiber(fiber), - type: getElementTypeForFiber(fiber), + type: elementType, // Inspectable properties. // TODO Review sanitization approach for the below inspectable values. From 98be5bc6e8ae54463b6b2674fd78e654d4c8fd8a Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Fri, 30 Aug 2019 08:55:19 +0200 Subject: [PATCH 2/9] Passed hasLegacyContext as prop to SelectedElement --- .../src/devtools/views/Components/InspectedElementContext.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js index f58671eb4e3f9..e2e23e75abd37 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js @@ -159,6 +159,7 @@ function InspectedElementContextController({children}: Props) { canEditHooks, canToggleSuspense, canViewSource, + hasLegacyContext, source, type, owners, @@ -173,6 +174,7 @@ function InspectedElementContextController({children}: Props) { canEditHooks, canToggleSuspense, canViewSource, + hasLegacyContext, id, source, type, From 9baac45ee48b4de9b6e1f69facb1d776d22b1964 Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Fri, 30 Aug 2019 08:57:46 +0200 Subject: [PATCH 3/9] Changing context labels based on hasLegacyContext --- .../src/devtools/views/Components/SelectedElement.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Components/SelectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/SelectedElement.js index a553f494af1a9..8ac93a1995c0f 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/SelectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/SelectedElement.js @@ -267,6 +267,7 @@ function InspectedElementView({ canEditFunctionProps, canEditHooks, canToggleSuspense, + hasLegacyContext, context, hooks, owners, @@ -376,7 +377,7 @@ function InspectedElementView({ )} Date: Fri, 30 Aug 2019 09:11:46 +0200 Subject: [PATCH 4/9] Fixed flow types. --- packages/react-devtools-shared/src/backend/renderer.js | 1 + packages/react-devtools-shared/src/backend/types.js | 3 +++ .../src/devtools/views/Components/types.js | 3 +++ 3 files changed, 7 insertions(+) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 967e92b61eb59..0aec474cd6e57 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -2249,6 +2249,7 @@ export function attach( // Can view component source location. canViewSource, + // Does the component have legacy context attached to it hasLegacyContext, displayName: getDisplayNameForFiber(fiber), diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 87f9fc14a5f6f..8a30c902d6452 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -163,6 +163,9 @@ export type InspectedElement = {| // Can view component source location. canViewSource: boolean, + // Does the component have legacy context attached to it + hasLegacyContext: boolean, + // Inspectable properties. context: Object | null, hooks: Object | null, diff --git a/packages/react-devtools-shared/src/devtools/views/Components/types.js b/packages/react-devtools-shared/src/devtools/views/Components/types.js index 547baeb9ec402..b737bb8b12eb1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/types.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/types.js @@ -71,6 +71,9 @@ export type InspectedElement = {| // Can view component source location. canViewSource: boolean, + // Does the component have legacy context attached to it + hasLegacyContext: boolean, + // Inspectable properties. context: Object | null, hooks: Object | null, From fd6914f44b349381ce3d5a3c86039a7e945592f3 Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Fri, 30 Aug 2019 09:33:11 +0200 Subject: [PATCH 5/9] Fixed typos. --- packages/react-devtools-shared/src/backend/legacy/renderer.js | 3 +++ packages/react-devtools-shared/src/backend/renderer.js | 2 +- packages/react-devtools-shared/src/backend/types.js | 2 +- .../src/devtools/views/Components/types.js | 4 ++-- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 94db3b25ada8e..bc2fb30ccf8d2 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -753,6 +753,9 @@ export function attach( // Can view component source location. canViewSource: type === ElementTypeClass || type === ElementTypeFunction, + // Only legacy context exists in legacy versions. + hasLegacyContext: true, + displayName: displayName, type: type, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 0aec474cd6e57..434d50091e57a 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -2249,7 +2249,7 @@ export function attach( // Can view component source location. canViewSource, - // Does the component have legacy context attached to it + // Does the component have legacy context attached to it. hasLegacyContext, displayName: getDisplayNameForFiber(fiber), diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 8a30c902d6452..245817b5c6600 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -163,7 +163,7 @@ export type InspectedElement = {| // Can view component source location. canViewSource: boolean, - // Does the component have legacy context attached to it + // Does the component have legacy context attached to it. hasLegacyContext: boolean, // Inspectable properties. diff --git a/packages/react-devtools-shared/src/devtools/views/Components/types.js b/packages/react-devtools-shared/src/devtools/views/Components/types.js index b737bb8b12eb1..3ce717cef6ec7 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/types.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/types.js @@ -71,7 +71,7 @@ export type InspectedElement = {| // Can view component source location. canViewSource: boolean, - // Does the component have legacy context attached to it + // Does the component have legacy context attached to it. hasLegacyContext: boolean, // Inspectable properties. @@ -83,7 +83,7 @@ export type InspectedElement = {| // List of owners owners: Array | null, - // Location of component in source coude. + // Location of component in source code. source: Source | null, type: ElementType, From b1d3a6c5d9f6a50a387cd749aa5fcae43df44094 Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Sat, 31 Aug 2019 09:52:44 +0200 Subject: [PATCH 6/9] Added tests for hasLegacyContext. --- .../__tests__/inspectedElementContext-test.js | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js index 73a3e916c2836..de2e1b03a1131 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js @@ -11,10 +11,13 @@ import typeof ReactTestRenderer from 'react-test-renderer'; import type {GetInspectedElementPath} from 'react-devtools-shared/src/devtools/views/Components/InspectedElementContext'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; +import React, {Component, createContext} from 'react'; +import {Fragment} from '../../../../../../../../../../Applications/WebStorm.app/Contents/plugins/JavaScriptLanguage/jsLanguageServicesImpl/flow/react'; describe('InspectedElementContext', () => { let React; let ReactDOM; + let PropTypes; let TestRenderer: ReactTestRenderer; let bridge: FrontendBridge; let store: Store; @@ -40,6 +43,7 @@ describe('InspectedElementContext', () => { React = require('react'); ReactDOM = require('react-dom'); + PropTypes = require('prop-types'); TestUtils = require('react-dom/test-utils'); TestRenderer = utils.requireTestRenderer(); @@ -114,6 +118,119 @@ describe('InspectedElementContext', () => { done(); }); + it('should have legacyContext flag set to true if the component is using the legacy context API.', async done => { + const contextData = { + bool: true, + }; + + // Legacy Context API. + class LegacyContextProvider extends React.Component { + static childContextTypes = { + bool: PropTypes.bool, + }; + getChildContext() { + return contextData; + } + render() { + return this.props.children; + } + } + class LegacyContextConsumer extends React.Component { + static contextTypes = { + bool: PropTypes.bool, + }; + render() { + return null; + } + } + + // Modern Context API + const BoolContext = createContext(contextData.bool); + BoolContext.displayName = 'BoolContext'; + + class ModernContextType extends Component { + static contextType = BoolContext; + render() { + return null; + } + } + + const ModernContext = createContext(); + ModernContext.displayName = 'ModernContext'; + + const container = document.createElement('div'); + await utils.actAsync(() => + ReactDOM.render( + + + + + {value => null} + + + {value => null} + + , + container, + ), + ); + + const ids = [ + { + // + id: ((store.getElementIDAtIndex(1): any): number), + shouldHaveLegacyContext: true, + }, + { + // + id: ((store.getElementIDAtIndex(2): any): number), + shouldHaveLegacyContext: false, + }, + { + // + id: ((store.getElementIDAtIndex(3): any): number), + shouldHaveLegacyContext: false, + }, + { + // + id: ((store.getElementIDAtIndex(5): any): number), + shouldHaveLegacyContext: false, + }, + ]; + + function Suspender({target, shouldHaveLegacyContext}) { + const {getInspectedElement} = React.useContext(InspectedElementContext); + const inspectedElement = getInspectedElement(target); + + expect(inspectedElement.context).not.toBe(null); + expect(inspectedElement.hasLegacyContext).toBe(shouldHaveLegacyContext); + + return null; + } + + for (let i = 0; i < ids.length; i++) { + const {id, shouldHaveLegacyContext} = ids[i]; + + await utils.actAsync( + () => + TestRenderer.create( + + + + + , + ), + false, + ); + } + done(); + }); + it('should poll for updates for the currently selected element', async done => { const Example = () => null; From e409465ff2377bd5749cd68e443e942893d687e1 Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Sat, 31 Aug 2019 09:56:58 +0200 Subject: [PATCH 7/9] Renamed test. --- .../src/__tests__/inspectedElementContext-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js index de2e1b03a1131..4772c89c62a51 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js @@ -118,7 +118,7 @@ describe('InspectedElementContext', () => { done(); }); - it('should have legacyContext flag set to true if the component is using the legacy context API.', async done => { + it('should have hasLegacyContext flag set to either "true" or "false" depending on which context API is used.', async done => { const contextData = { bool: true, }; From afa2f3dde685e1a73617fec2663edbb422276f95 Mon Sep 17 00:00:00 2001 From: Hristo Kanchev Date: Sat, 31 Aug 2019 10:03:05 +0200 Subject: [PATCH 8/9] Removed test imports. --- .../src/__tests__/inspectedElementContext-test.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js index 4772c89c62a51..ca5d074eefeaf 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js @@ -11,8 +11,6 @@ import typeof ReactTestRenderer from 'react-test-renderer'; import type {GetInspectedElementPath} from 'react-devtools-shared/src/devtools/views/Components/InspectedElementContext'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; -import React, {Component, createContext} from 'react'; -import {Fragment} from '../../../../../../../../../../Applications/WebStorm.app/Contents/plugins/JavaScriptLanguage/jsLanguageServicesImpl/flow/react'; describe('InspectedElementContext', () => { let React; @@ -145,17 +143,17 @@ describe('InspectedElementContext', () => { } // Modern Context API - const BoolContext = createContext(contextData.bool); + const BoolContext = React.createContext(contextData.bool); BoolContext.displayName = 'BoolContext'; - class ModernContextType extends Component { + class ModernContextType extends React.Component { static contextType = BoolContext; render() { return null; } } - const ModernContext = createContext(); + const ModernContext = React.createContext(); ModernContext.displayName = 'ModernContext'; const container = document.createElement('div'); From de317458da9b73cef55c0d05cdb5586f19ae1793 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 9 Sep 2019 17:01:17 -0700 Subject: [PATCH 9/9] Added inline comment --- packages/react-devtools-shared/src/backend/renderer.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 434d50091e57a..520f583013d91 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -2130,6 +2130,7 @@ export function attach( ) { canViewSource = true; if (stateNode && stateNode.context != null) { + // Don't show an empty context object for class components that don't use the context API. const shouldHideContext = elementType === ElementTypeClass && !(type.contextTypes || type.contextType);