Skip to content

Commit

Permalink
Refactor stack handling (no functional changes) (#13165)
Browse files Browse the repository at this point in the history
* Refactor ReactDebugCurrentFiber to use named exports

This makes the difference between it and ReactFiberCurrentFrame a bit clearer.

ReactDebugCurrentFiber is Fiber's own implementation.
ReactFiberCurrentFrame is the thing that holds a reference to the current implementation and delegates to it.

* Unify ReactFiberComponentTreeHook and ReactDebugCurrentFiber

Conceptually they're very related.

ReactFiberComponentTreeHook contains implementation details of reading Fiber's stack (both in DEV and PROD).
ReactDebugCurrentFiber contained a reference to the current fiber, and used the above utility.

It was confusing when to use which one. Colocating them makes it clearer what you could do with each method.

In the future, the plan is to stop using these methods explicitly in most places, and instead delegate to a warning system that includes stacks automatically. This change makes future refactorings simpler by colocating related logic.

* Rename methods to better reflect their meanings

Clarify which are DEV or PROD-only.
Clarify which can return null.

I believe the "work in progress only" was a mistake. I introduced it because I wasn't sure what guarantees we have around .return. But we know for sure that following a .return chain gives us an accurate stack even if we get into WIP trees because we don't have reparenting. So it's fine to relax that naming.

* Rename ReactDebugCurrentFiber -> ReactCurrentFiber

It's not completely DEV-only anymore.
Individual methods already specify whether they work in DEV or PROD in their names.
  • Loading branch information
gaearon authored Jul 7, 2018
1 parent ebbd221 commit 5662595
Show file tree
Hide file tree
Showing 17 changed files with 201 additions and 225 deletions.
39 changes: 23 additions & 16 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
*/

// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {
getCurrentFiberOwnerNameInDevOrNull,
getCurrentFiberStackInDevOrNull,
} from 'react-reconciler/src/ReactCurrentFiber';
import {registrationNameModules} from 'events/EventPluginRegistry';
import warning from 'shared/warning';

Expand Down Expand Up @@ -45,10 +48,6 @@ import {validateProperties as validateARIAProperties} from '../shared/ReactDOMIn
import {validateProperties as validateInputProperties} from '../shared/ReactDOMNullInputValuePropHook';
import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook';

const {
getCurrentFiberOwnerName,
getCurrentFiberStackAddendum,
} = ReactDebugCurrentFiber;
let didWarnInvalidHydration = false;
let didWarnShadyDOM = false;

Expand All @@ -62,7 +61,7 @@ const HTML = '__html';

const {html: HTML_NAMESPACE} = Namespaces;

let getStack = () => '';
let getStackInDevOrNull = () => '';

let warnedUnknownTags;
let suppressHydrationWarning;
Expand All @@ -77,7 +76,7 @@ let normalizeMarkupForTextOrAttribute;
let normalizeHTML;

if (__DEV__) {
getStack = getCurrentFiberStackAddendum;
getStackInDevOrNull = getCurrentFiberStackInDevOrNull;

warnedUnknownTags = {
// Chrome is the only major browser not shipping <time>. But as of July
Expand Down Expand Up @@ -181,15 +180,15 @@ if (__DEV__) {
registrationName,
registrationName,
registrationName,
getCurrentFiberStackAddendum(),
getCurrentFiberStackInDevOrNull(),
);
} else {
warning(
false,
'Expected `%s` listener to be a function, instead got a value of `%s` type.%s',
registrationName,
typeof listener,
getCurrentFiberStackAddendum(),
getCurrentFiberStackInDevOrNull(),
);
}
};
Expand Down Expand Up @@ -267,7 +266,11 @@ function setInitialDOMProperties(
}
}
// Relies on `updateStylesByID` not mutating `styleUpdates`.
CSSPropertyOperations.setValueForStyles(domElement, nextProp, getStack);
CSSPropertyOperations.setValueForStyles(
domElement,
nextProp,
getStackInDevOrNull,
);
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
const nextHtml = nextProp ? nextProp[HTML] : undefined;
if (nextHtml != null) {
Expand Down Expand Up @@ -323,7 +326,11 @@ function updateDOMProperties(
const propKey = updatePayload[i];
const propValue = updatePayload[i + 1];
if (propKey === STYLE) {
CSSPropertyOperations.setValueForStyles(domElement, propValue, getStack);
CSSPropertyOperations.setValueForStyles(
domElement,
propValue,
getStackInDevOrNull,
);
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
setInnerHTML(domElement, propValue);
} else if (propKey === CHILDREN) {
Expand Down Expand Up @@ -442,7 +449,7 @@ export function setInitialProperties(
false,
'%s is using shady DOM. Using shady DOM with React can ' +
'cause things to break subtly.',
getCurrentFiberOwnerName() || 'A component',
getCurrentFiberOwnerNameInDevOrNull() || 'A component',
);
didWarnShadyDOM = true;
}
Expand Down Expand Up @@ -516,7 +523,7 @@ export function setInitialProperties(
props = rawProps;
}

assertValidProps(tag, props, getStack);
assertValidProps(tag, props, getStackInDevOrNull);

setInitialDOMProperties(
tag,
Expand Down Expand Up @@ -604,7 +611,7 @@ export function diffProperties(
break;
}

assertValidProps(tag, nextProps, getStack);
assertValidProps(tag, nextProps, getStackInDevOrNull);

let propKey;
let styleName;
Expand Down Expand Up @@ -834,7 +841,7 @@ export function diffHydratedProperties(
false,
'%s is using shady DOM. Using shady DOM with React can ' +
'cause things to break subtly.',
getCurrentFiberOwnerName() || 'A component',
getCurrentFiberOwnerNameInDevOrNull() || 'A component',
);
didWarnShadyDOM = true;
}
Expand Down Expand Up @@ -895,7 +902,7 @@ export function diffHydratedProperties(
break;
}

assertValidProps(tag, rawProps, getStack);
assertValidProps(tag, rawProps, getStackInDevOrNull);

if (__DEV__) {
extraAttributeNames = new Set();
Expand Down
19 changes: 9 additions & 10 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
*/

// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {
getCurrentFiberOwnerNameInDevOrNull,
getCurrentFiberStackInDevOrNull,
} from 'react-reconciler/src/ReactCurrentFiber';
import invariant from 'shared/invariant';
import warning from 'shared/warning';

Expand All @@ -25,10 +28,6 @@ type InputWithWrapperState = HTMLInputElement & {
},
};

const {
getCurrentFiberOwnerName,
getCurrentFiberStackAddendum,
} = ReactDebugCurrentFiber;
let didWarnValueDefaultValue = false;
let didWarnCheckedDefaultChecked = false;
let didWarnControlledToUncontrolled = false;
Expand Down Expand Up @@ -75,7 +74,7 @@ export function initWrapperState(element: Element, props: Object) {
ReactControlledValuePropTypes.checkPropTypes(
'input',
props,
getCurrentFiberStackAddendum,
getCurrentFiberStackInDevOrNull,
);

if (
Expand All @@ -91,7 +90,7 @@ export function initWrapperState(element: Element, props: Object) {
'both). Decide between using a controlled or uncontrolled input ' +
'element and remove one of these props. More info: ' +
'https://fb.me/react-controlled-components',
getCurrentFiberOwnerName() || 'A component',
getCurrentFiberOwnerNameInDevOrNull() || 'A component',
props.type,
);
didWarnCheckedDefaultChecked = true;
Expand All @@ -109,7 +108,7 @@ export function initWrapperState(element: Element, props: Object) {
'both). Decide between using a controlled or uncontrolled input ' +
'element and remove one of these props. More info: ' +
'https://fb.me/react-controlled-components',
getCurrentFiberOwnerName() || 'A component',
getCurrentFiberOwnerNameInDevOrNull() || 'A component',
props.type,
);
didWarnValueDefaultValue = true;
Expand Down Expand Up @@ -154,7 +153,7 @@ export function updateWrapper(element: Element, props: Object) {
'Decide between using a controlled or uncontrolled input ' +
'element for the lifetime of the component. More info: https://fb.me/react-controlled-components%s',
props.type,
getCurrentFiberStackAddendum(),
getCurrentFiberStackInDevOrNull(),
);
didWarnUncontrolledToControlled = true;
}
Expand All @@ -170,7 +169,7 @@ export function updateWrapper(element: Element, props: Object) {
'Decide between using a controlled or uncontrolled input ' +
'element for the lifetime of the component. More info: https://fb.me/react-controlled-components%s',
props.type,
getCurrentFiberStackAddendum(),
getCurrentFiberStackInDevOrNull(),
);
didWarnControlledToUncontrolled = true;
}
Expand Down
14 changes: 6 additions & 8 deletions packages/react-dom/src/client/ReactDOMFiberSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@
*/

// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {
getCurrentFiberOwnerNameInDevOrNull,
getCurrentFiberStackInDevOrNull,
} from 'react-reconciler/src/ReactCurrentFiber';
import warning from 'shared/warning';

import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';

const {
getCurrentFiberOwnerName,
getCurrentFiberStackAddendum,
} = ReactDebugCurrentFiber;

let didWarnValueDefaultValue;

if (__DEV__) {
Expand All @@ -32,7 +30,7 @@ type SelectWithWrapperState = HTMLSelectElement & {
};

function getDeclarationErrorAddendum() {
const ownerName = getCurrentFiberOwnerName();
const ownerName = getCurrentFiberOwnerNameInDevOrNull();
if (ownerName) {
return '\n\nCheck the render method of `' + ownerName + '`.';
}
Expand All @@ -48,7 +46,7 @@ function checkSelectPropTypes(props) {
ReactControlledValuePropTypes.checkPropTypes(
'select',
props,
getCurrentFiberStackAddendum,
getCurrentFiberStackInDevOrNull,
);

for (let i = 0; i < valuePropNames.length; i++) {
Expand Down
5 changes: 2 additions & 3 deletions packages/react-dom/src/client/ReactDOMFiberTextarea.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@
import invariant from 'shared/invariant';
import warning from 'shared/warning';
// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {getCurrentFiberStackInDevOrNull} from 'react-reconciler/src/ReactCurrentFiber';

import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';

const {getCurrentFiberStackAddendum} = ReactDebugCurrentFiber;
let didWarnValDefaultVal = false;

type TextAreaWithWrapperState = HTMLTextAreaElement & {
Expand Down Expand Up @@ -68,7 +67,7 @@ export function initWrapperState(element: Element, props: Object) {
ReactControlledValuePropTypes.checkPropTypes(
'textarea',
props,
getCurrentFiberStackAddendum,
getCurrentFiberStackInDevOrNull,
);
if (
props.value !== undefined &&
Expand Down
5 changes: 2 additions & 3 deletions packages/react-dom/src/client/validateDOMNesting.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@

import warning from 'shared/warning';
// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {getCurrentFiberStackInDevOrNull} from 'react-reconciler/src/ReactCurrentFiber';

const {getCurrentFiberStackAddendum} = ReactDebugCurrentFiber;
let validateDOMNesting = () => {};

if (__DEV__) {
Expand Down Expand Up @@ -427,7 +426,7 @@ if (__DEV__) {
}

const ancestorTag = invalidParentOrAncestor.tag;
const addendum = getCurrentFiberStackAddendum();
const addendum = getCurrentFiberStackInDevOrNull();

const warnKey =
!!invalidParent + '|' + childTag + '|' + ancestorTag + '|' + addendum;
Expand Down
6 changes: 3 additions & 3 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import type {ReactNodeList} from 'shared/ReactTypes';
import './ReactNativeInjection';

import * as ReactNativeFiberRenderer from 'react-reconciler/inline.native';
// TODO: direct imports like some-package/src/* are bad. Fix me.
import {getStackByFiberInDevAndProd} from 'react-reconciler/src/ReactCurrentFiber';
import * as ReactPortal from 'shared/ReactPortal';
import * as ReactGenericBatching from 'events/ReactGenericBatching';
import ReactVersion from 'shared/ReactVersion';
// Module provided by RN:
import UIManager from 'UIManager';

import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook';

import NativeMethodsMixin from './NativeMethodsMixin';
import ReactNativeComponent from './ReactNativeComponent';
import * as ReactNativeComponentTree from './ReactNativeComponentTree';
Expand Down Expand Up @@ -80,7 +80,7 @@ function computeComponentStackForErrorReporting(reactTag: number): string {
if (!fiber) {
return '';
}
return getStackAddendumByWorkInProgressFiber(fiber);
return getStackByFiberInDevAndProd(fiber);
}

const roots = new Map();
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactCapturedValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import type {Fiber} from './ReactFiber';

import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook';
import {getStackByFiberInDevAndProd} from './ReactCurrentFiber';

export type CapturedValue<T> = {
value: T,
Expand All @@ -36,6 +36,6 @@ export function createCapturedValue<T>(
return {
value,
source,
stack: getStackAddendumByWorkInProgressFiber(source),
stack: getStackByFiberInDevAndProd(source),
};
}
Loading

0 comments on commit 5662595

Please sign in to comment.