Skip to content

Reorganize how shared internals are accessed #13201

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.

Sign up for GitHub

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

Merged
gaearon merged 2 commits into from
Jul 13, 2018
Merged

Reorganize how shared internals are accessed #13201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57f87d2
Reorganize how shared internals are accessed
gaearon Jul 12, 2018
3cb717e
Update forks.js
gaearon Jul 13, 2018
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
4 changes: 3 additions & 1 deletion packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import * as EventPluginRegistry from 'events/EventPluginRegistry';
import * as EventPropagators from 'events/EventPropagators';
import * as ReactInstanceMap from 'shared/ReactInstanceMap';
import ReactVersion from 'shared/ReactVersion';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
import invariant from 'shared/invariant';
import lowPriorityWarning from 'shared/lowPriorityWarning';
Expand All @@ -46,6 +46,8 @@ import {
} from '../shared/HTMLNodeType';
import {ROOT_ATTRIBUTE_NAME} from '../shared/DOMProperty';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

let topLevelUpdateWarnings;
let warnOnInvalidCallback;
let didWarnAboutUnstableCreatePortal = false;
Expand Down
5 changes: 4 additions & 1 deletion packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import lowPriorityWarning from 'shared/lowPriorityWarning';
import warning from 'shared/warning';
import checkPropTypes from 'prop-types/checkPropTypes';
import describeComponentFrame from 'shared/describeComponentFrame';
import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {warnAboutDeprecatedLifecycles} from 'shared/ReactFeatureFlags';
import {
REACT_FORWARD_REF_TYPE,
Expand Down Expand Up @@ -68,6 +68,7 @@ const toArray = ((React.Children.toArray: any): toArrayType);
// Each stack is an array of frames which may contain nested stacks of elements.
let currentDebugStacks = [];

let ReactDebugCurrentFrame;
let prevGetCurrentStackImpl = null;
let getCurrentServerStackImpl = () => '';
let describeStackFrame = element => '';
Expand All @@ -78,6 +79,8 @@ let pushElementToDebugStack = (element: ReactElement) => {};
let popCurrentDebugStack = () => {};

if (__DEV__) {
ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;

validatePropertiesInDevelopment = function(type, props) {
validateARIAProperties(type, props);
validateInputProperties(type, props);
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/shared/ReactDOMInvalidARIAHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
*/

import warning from 'shared/warning';
import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';

import {ATTRIBUTE_NAME_CHAR} from './DOMProperty';
import isCustomComponent from './isCustomComponent';
import validAriaProperties from './validAriaProperties';

const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;

const warnedProperties = {};
const rARIA = new RegExp('^(aria)-[' + ATTRIBUTE_NAME_CHAR + ']*$');
const rARIACamel = new RegExp('^(aria)[A-Z][' + ATTRIBUTE_NAME_CHAR + ']*$');
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/shared/ReactDOMNullInputValuePropHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
* LICENSE file in the root directory of this source tree.
*/

import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import warning from 'shared/warning';

let ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;

let didWarnValueNull = false;

export function validateProperties(type, props) {
Expand Down
6 changes: 5 additions & 1 deletion packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
registrationNameModules,
possibleRegistrationNames,
} from 'events/EventPluginRegistry';
import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import warning from 'shared/warning';

import {
Expand All @@ -21,9 +21,13 @@ import {
import isCustomComponent from './isCustomComponent';
import possibleStandardNames from './possibleStandardNames';

let ReactDebugCurrentFrame = null;

let validateProperty = () => {};

if (__DEV__) {
ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;
Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 12, 2018

Choose a reason for hiding this comment

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

It doesn't exist outside of DEV, which is true in other "hook" files too. Other "hook" files don't have DEV blocks (they just assume they're called only in DEV) so I didn't put the condition there. Since this file already had a condition, I put it conditionally.


const warnedProperties = {};
const hasOwnProperty = Object.prototype.hasOwnProperty;
const EVENT_NAME_REGEX = /^on./;
Expand Down
3 changes: 2 additions & 1 deletion packages/react-native-renderer/src/ReactFabric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ import ReactNativeComponent from './ReactNativeComponent';
import * as ReactFabricComponentTree from './ReactFabricComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';

import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
import warning from 'shared/warning';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
const findHostInstance = ReactFabricRenderer.findHostInstance;

function findNodeHandle(componentOrHandle: any): ?number {
Expand Down
3 changes: 2 additions & 1 deletion packages/react-native-renderer/src/ReactNativeRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ import ReactNativeComponent from './ReactNativeComponent';
import * as ReactNativeComponentTree from './ReactNativeComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';

import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
import warning from 'shared/warning';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
const findHostInstance = ReactNativeFiberRenderer.findHostInstance;

function findNodeHandle(componentOrHandle: any): ?number {
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactCurrentFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
IndeterminateComponent,
FunctionalComponent,
Expand All @@ -17,6 +17,8 @@ import {
import describeComponentFrame from 'shared/describeComponentFrame';
import getComponentName from 'shared/getComponentName';

const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;

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

type LifeCyclePhase = 'render' | 'getChildContext';
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import {
Update,
Ref,
} from 'shared/ReactTypeOfSideEffect';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
enableGetDerivedStateFromCatch,
enableSuspense,
Expand Down Expand Up @@ -98,6 +98,8 @@ import {
} from './ReactFiberClassComponent';
import MAX_SIGNED_31_BIT_INT from './maxSigned31BitInt';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

let didWarnAboutBadClass;
let didWarnAboutGetDerivedStateOnFunctionalComponent;
let didWarnAboutStatelessRefs;
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {FiberRoot, Batch} from './ReactFiberRoot';
import type {ExpirationTime} from './ReactFiberExpirationTime';

import ReactErrorUtils from 'shared/ReactErrorUtils';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import ReactStrictModeWarnings from './ReactStrictModeWarnings';
import {
NoEffect,
Expand Down Expand Up @@ -155,6 +155,7 @@ export type Thenable = {
then(resolve: () => mixed, reject?: () => mixed): mixed,
};

const {ReactCurrentOwner} = ReactSharedInternals;
const {
invokeGuardedCallback,
hasCaughtError,
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberTreeReflection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import invariant from 'shared/invariant';
import warning from 'shared/warning';

import * as ReactInstanceMap from 'shared/ReactInstanceMap';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
import {
ClassComponent,
Expand All @@ -24,6 +24,8 @@ import {
} from 'shared/ReactTypeOfWork';
import {NoEffect, Placement} from 'shared/ReactTypeOfSideEffect';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

const MOUNTING = 1;
const MOUNTED = 2;
const UNMOUNTED = 3;
Expand Down
20 changes: 2 additions & 18 deletions packages/react/src/React.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* LICENSE file in the root directory of this source tree.
*/

import assign from 'object-assign';
import ReactVersion from 'shared/ReactVersion';
import {
REACT_ASYNC_MODE_TYPE,
Expand All @@ -19,7 +18,6 @@ import {enableSuspense} from 'shared/ReactFeatureFlags';
import {Component, PureComponent} from './ReactBaseClasses';
import {createRef} from './ReactCreateRef';
import {forEach, map, count, toArray, only} from './ReactChildren';
import ReactCurrentOwner from './ReactCurrentOwner';
import {
createElement,
createFactory,
Expand All @@ -33,7 +31,7 @@ import {
createFactoryWithValidation,
cloneElementWithValidation,
} from './ReactElementValidator';
import ReactDebugCurrentFrame from './ReactDebugCurrentFrame';
import ReactSharedInternals from './ReactSharedInternals';

const React = {
Children: {
Expand Down Expand Up @@ -63,25 +61,11 @@ const React = {

version: ReactVersion,

__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
ReactCurrentOwner,
// Used by renderers to avoid bundling object-assign twice in UMD bundles:
assign,
},
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: ReactSharedInternals,
};

if (enableSuspense) {
React.Placeholder = REACT_PLACEHOLDER_TYPE;
}

if (__DEV__) {
Object.assign(React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, {
// These should not be included in production.
ReactDebugCurrentFrame,
// Shim for React DOM 16.0.0 which still destructured (but not used) this.
// TODO: remove in React 17.0.
ReactComponentTreeHook: {},
});
}

export default React;
28 changes: 28 additions & 0 deletions packages/react/src/ReactSharedInternals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import assign from 'object-assign';
import ReactCurrentOwner from './ReactCurrentOwner';
import ReactDebugCurrentFrame from './ReactDebugCurrentFrame';

const ReactSharedInternals = {
ReactCurrentOwner,
// Used by renderers to avoid bundling object-assign twice in UMD bundles:
assign,
};

if (__DEV__) {
Object.assign(ReactSharedInternals, {
Copy link
Copy Markdown
Contributor

@nhunzaker nhunzaker Jul 13, 2018

Choose a reason for hiding this comment

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

I know this code was just moved, but I'm curious why assign is imported above, but Object.assign is used here. Are they equivalent?

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 13, 2018

Choose a reason for hiding this comment

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

We could put Object.assign there directly and assume it'll be compiled to an assign import but that would obscure the reason we have this code in the first place (which is to attach this specific npm dependency)

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In other words:

  • We compile Object.assign calls to assign imports
  • But we also replace assign with React.__internals.assign in renderer bundles to avoid bundling assign twice for UMD builds
  • So we need to put assign on the React internals object
  • We could do that by using Object.assign.. which would fall back to assign..

Copy link
Copy Markdown
Contributor

@nhunzaker nhunzaker Jul 13, 2018

Choose a reason for hiding this comment

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

I didn't realize Object.assign compiled down to the assign module, 👍

Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, it's early so maybe Dan's explanation makes sense– but after reading it twice, I'm still confused about why we import assign:

import assign from 'object-assign';

But then use Object.assign here.

Edit: I just spotted the assign key in the internals object above, so it makes sense why we import assign but I'm not clear on why we don't also just use assign here instead of Object.assign 😄

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 13, 2018

Choose a reason for hiding this comment

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

why we don't also just use assign here instead of Object.assign 😄

We could, I just didn't see the point in explicitly using it in a situation where we normally wouldn't.

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 13, 2018

Choose a reason for hiding this comment

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

In other words — they're the same thing. The intention of the code is "put this Node module onto an object". So I'm using assign for the "Node module" part to express that intention, and use Object.assign for the "put onto an object" part because that's how we would express it in any other file. Otherwise you start thinking: "okay, this file uses assign() from npm instead of Object.assign, maybe I need to do this in other files too.."

Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 14, 2018

Choose a reason for hiding this comment

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

That...makes sense, but I still find the distinction (within this file) confusing. And I suspect others might too. 😄

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 14, 2018

Choose a reason for hiding this comment

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

Open to other suggestions, I don’t really care either way. Feel free to change it to what’s looks less confusing.

This isn’t new code.

Copy link
Copy Markdown
Contributor

@nhunzaker nhunzaker Jul 14, 2018

Choose a reason for hiding this comment

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

I'm not really sure what's better, and I can go either way. I don't consider it a blocker for getting this change in.

// These should not be included in production.
ReactDebugCurrentFrame,
// Shim for React DOM 16.0.0 which still destructured (but not used) this.
// TODO: remove in React 17.0.
ReactComponentTreeHook: {},
});
}

export default ReactSharedInternals;
15 changes: 0 additions & 15 deletions packages/shared/ReactGlobalSharedState.js
View file
Open in desktop

This file was deleted.

13 changes: 13 additions & 0 deletions packages/shared/ReactSharedInternals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React from 'react';

const ReactSharedInternals =
React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;

export default ReactSharedInternals;
6 changes: 6 additions & 0 deletions scripts/jest/setupHostConfigs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,9 @@ inlinedHostConfigs.forEach(rendererInfo => {
return renderer;
});
});

// Make it possible to import this module inside
// the React package itself.
jest.mock('shared/ReactSharedInternals', () =>
require.requireActual('react/src/ReactSharedInternals')
);
9 changes: 9 additions & 0 deletions scripts/rollup/forks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ const forks = Object.freeze({
return 'shared/forks/object-assign.umd.js';
},

// Without this fork, importing `shared/ReactSharedInternals` inside
// the `react` package itself would not work due to a cyclical dependency.
'shared/ReactSharedInternals': (bundleType, entry) => {
if (entry === 'react') {
return 'react/src/ReactSharedInternals';
}
return null;
},

// We have a few forks for different environments.
'shared/ReactFeatureFlags': (bundleType, entry) => {
switch (entry) {
Expand Down