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

[DevTools] [Context] Legacy Context #16617

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

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

😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Freakin' WebStorm 😆 #NeverTrustYourEditor


describe('InspectedElementContext', () => {
let React;
let ReactDOM;
let PropTypes;
let TestRenderer: ReactTestRenderer;
let bridge: FrontendBridge;
let store: Store;
Expand All @@ -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();

Expand Down Expand Up @@ -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<any> {
static childContextTypes = {
bool: PropTypes.bool,
};
getChildContext() {
return contextData;
}
render() {
return this.props.children;
}
}
class LegacyContextConsumer extends React.Component<any> {
static contextTypes = {
bool: PropTypes.bool,
};
render() {
return null;
}
}

// Modern Context API
const BoolContext = createContext(contextData.bool);
BoolContext.displayName = 'BoolContext';

class ModernContextType extends Component<any> {
static contextType = BoolContext;
render() {
return null;
}
}

const ModernContext = createContext();
ModernContext.displayName = 'ModernContext';

const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<React.Fragment>
<LegacyContextProvider>
<LegacyContextConsumer />
</LegacyContextProvider>
<BoolContext.Consumer>{value => null}</BoolContext.Consumer>
<ModernContextType />
<ModernContext.Provider value={contextData}>
<ModernContext.Consumer>{value => null}</ModernContext.Consumer>
</ModernContext.Provider>
</React.Fragment>,
container,
),
);

const ids = [
{
// <LegacyContextConsumer />
id: ((store.getElementIDAtIndex(1): any): number),
shouldHaveLegacyContext: true,
},
{
// <BoolContext.Consumer>
id: ((store.getElementIDAtIndex(2): any): number),
shouldHaveLegacyContext: false,
},
{
// <ModernContextType />
id: ((store.getElementIDAtIndex(3): any): number),
shouldHaveLegacyContext: false,
},
{
// <ModernContext.Consumer>
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(
<Contexts
defaultSelectedElementID={id}
defaultSelectedElementIndex={0}>
<React.Suspense fallback={null}>
<Suspender
target={id}
shouldHaveLegacyContext={shouldHaveLegacyContext}
/>
</React.Suspense>
</Contexts>,
),
false,
);
}
done();
});

it('should poll for updates for the currently selected element', async done => {
const Example = () => null;

Expand Down
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/backend/legacy/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also hide empty one though in the legacy mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

This value isn't used to hide the context label in the front end.. It just specifies whether the context is modern vs legacy. I guess it's a little weird to have a boolean value for this. Maybe we should use an enum? context type: none, modern, legacy.

Anyway, an empty legacy context won't be shown on the frontend because the InspectedElementTree component hides itself if the value it's passed is null or an empty object.


displayName: displayName,

type: type,
Expand Down
18 changes: 16 additions & 2 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2107,6 +2107,8 @@ export function attach(
type,
} = fiber;

const elementType = getElementTypeForFiber(fiber);

const usesHooks =
(tag === FunctionComponent ||
tag === SimpleMemoComponent ||
Expand All @@ -2128,7 +2130,13 @@ export function attach(
) {
canViewSource = true;
if (stateNode && stateNode.context != null) {
context = stateNode.context;
const shouldHideContext =
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably use an explanatory inline comment. (I'll add one.)

elementType === ElementTypeClass &&
!(type.contextTypes || type.contextType);

if (!shouldHideContext) {
context = stateNode.context;
}
}
} else if (
typeSymbol === CONTEXT_CONSUMER_NUMBER ||
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -2238,8 +2249,11 @@ export function attach(
// Can view component source location.
canViewSource,

// Does the component have legacy context attached to it.
hasLegacyContext,

displayName: getDisplayNameForFiber(fiber),
type: getElementTypeForFiber(fiber),
type: elementType,

// Inspectable properties.
// TODO Review sanitization approach for the below inspectable values.
Expand Down
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ function InspectedElementContextController({children}: Props) {
canEditHooks,
canToggleSuspense,
canViewSource,
hasLegacyContext,
source,
type,
owners,
Expand All @@ -173,6 +174,7 @@ function InspectedElementContextController({children}: Props) {
canEditHooks,
canToggleSuspense,
canViewSource,
hasLegacyContext,
id,
source,
type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ function InspectedElementView({
canEditFunctionProps,
canEditHooks,
canToggleSuspense,
hasLegacyContext,
context,
hooks,
owners,
Expand Down Expand Up @@ -376,7 +377,7 @@ function InspectedElementView({
)}
<HooksTree canEditHooks={canEditHooks} hooks={hooks} id={id} />
<InspectedElementTree
label="context"
label={hasLegacyContext ? 'legacy context' : 'context'}
data={context}
inspectPath={inspectContextPath}
overrideValueFn={overrideContextFn}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -80,7 +83,7 @@ export type InspectedElement = {|
// List of owners
owners: Array<Owner> | null,

// Location of component in source coude.
// Location of component in source code.
source: Source | null,

type: ElementType,
Expand Down