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.

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

Jump to bottom

Expose injectIntoDevTools() to renderers #11463

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Expose injectIntoDevTools() to renderers #11463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 1 addition & 4 deletions packages/react-cs-renderer/src/ReactNativeCS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type {ReactNativeCSType} from './ReactNativeCSTypes';
import {CSStatefulComponent} from 'CSStatefulComponent';

import ReactFiberReconciler from 'react-reconciler';
import {injectInternals} from 'react-reconciler/src/ReactFiberDevToolsHook';
import ReactVersion from 'shared/ReactVersion';

const emptyObject = {};
Expand Down Expand Up @@ -240,9 +239,7 @@ const ReactNativeCSFiberRenderer = ReactFiberReconciler({
},
});

injectInternals({
findHostInstanceByFiber: ReactNativeCSFiberRenderer.findHostInstance,
// This is an enum because we may add more (e.g. profiler build)
ReactNativeCSFiberRenderer.injectIntoDevTools({
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
rendererPackageName: 'react-cs-renderer',
Expand Down
6 changes: 1 addition & 5 deletions packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ import './ReactDOMClientInjection';
import ReactFiberReconciler from 'react-reconciler';
// TODO: direct imports like some-package/src/* are bad. Fix me.
import * as ReactPortal from 'react-reconciler/src/ReactPortal';
// TODO: direct imports like some-package/src/* are bad. Fix me.
import {injectInternals} from 'react-reconciler/src/ReactFiberDevToolsHook';
import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment';
import * as ReactGenericBatching from 'events/ReactGenericBatching';
import * as ReactControlledComponent from 'events/ReactControlledComponent';
Expand Down Expand Up @@ -955,10 +953,8 @@ if (ReactFeatureFlags.enableCreateRoot) {
};
}

const foundDevTools = injectInternals({
const foundDevTools = DOMRenderer.injectIntoDevTools({
findFiberByHostInstance: ReactDOMComponentTree.getClosestInstanceFromNode,
findHostInstanceByFiber: DOMRenderer.findHostInstance,
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
rendererPackageName: 'react-dom',
Expand Down
5 changes: 1 addition & 4 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import './ReactNativeInjection';
import * as ReactFiberErrorLogger
from 'react-reconciler/src/ReactFiberErrorLogger';
import * as ReactPortal from 'react-reconciler/src/ReactPortal';
import {injectInternals} from 'react-reconciler/src/ReactFiberDevToolsHook';
import * as ReactGenericBatching from 'events/ReactGenericBatching';
import TouchHistoryMath from 'events/TouchHistoryMath';
import * as ReactGlobalSharedState from 'shared/ReactGlobalSharedState';
Expand Down Expand Up @@ -130,11 +129,9 @@ if (__DEV__) {
);
}

injectInternals({
ReactNativeFiberRenderer.injectIntoDevTools({
findFiberByHostInstance: ReactNativeComponentTree.getClosestInstanceFromNode,
findHostInstanceByFiber: ReactNativeFiberRenderer.findHostInstance,
getInspectorDataForViewTag: getInspectorDataForViewTag,
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
rendererPackageName: 'react-native-renderer',
Expand Down
52 changes: 45 additions & 7 deletions packages/react-reconciler/src/ReactFiberReconciler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
processChildContext,
} from './ReactFiberContext';
import {createFiberRoot} from './ReactFiberRoot';
import * as ReactFiberDevToolsHook from './ReactFiberDevToolsHook';
import ReactFiberScheduler from './ReactFiberScheduler';
import {insertUpdateIntoFiber} from './ReactFiberUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -211,6 +212,23 @@ type HydrationHostConfig<T, P, I, TI, C, CX, PL> = {
): void,
};

// 0 is PROD, 1 is DEV.
// Might add PROFILE later.
type BundleType = 0 | 1;

type DevToolsConfig<I, TI> = {|
bundleType: BundleType,
version: string,
rendererPackageName: string,
// Note: this actually *does* depend on Fiber internal fields.
// Used by "inspect clicked DOM element" in React DevTools.
findFiberByHostInstance?: (instance: I | TI) => Fiber,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the devtools integration used to work without this method 🤔

With this being required I think we’d also need ReactFiberTreeReflection.findCurrentHostFiber and ReactFiberTreeReflection.findCurrentHostFiberWithNoPortals also exposed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we need them. This one doesn't have to be the current one. So as long as you save the fiber on some field on the instance (or use a weakmap), you can get one back.

Copy link
Contributor

@bvaughn bvaughn Nov 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It still does. The implementation (below) is:

        findFiberByHostInstance(instance: I | TI): Fiber | null {
          if (!findFiberByHostInstance) {
            // Might not be implemented by the renderer.
            return null;
          }
          return findFiberByHostInstance(instance);
        },

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's also true. (Though it's nice to implement if you can I guess.)

// Used by RN in-app inspector.
// This API is unfortunately RN-specific.
// TODO: Change it to accept Fiber instead and type it properly.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

getInspectorDataForViewTag?: (tag: number) => Object,
Copy link
Contributor

Choose a reason for hiding this comment

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

does the isAppActive() method no longer exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you remind me where it is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

It used to be part of the devtools config I think. https://github.com/facebook/react-devtools/blob/ef493abba0d08c0243bdb1f2183edc797d5d366c/packages/react-devtools-core/src/backend.js#L13-L18

I think the usage was so that an app could indicate whether it should take over the devtools connection or not, but not really sure. Or maybe to return false while still starting up (according to this comment)

https://github.com/facebook/react-devtools/blob/ef493abba0d08c0243bdb1f2183edc797d5d366c/packages/react-devtools-core/src/backend.js#L52-L55

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not required. I added it for RN so that multiple apps on simulator don't steal devtools from each other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, it's part of the options to connectToDevTools() call which sets up a websocket connection.

This is not related to the injectIntoDevTools call which injects renderer into a global variable.

|};

export type Reconciler<C, I, TI> = {
createContainer(containerInfo: C, hydrate: boolean): OpaqueRoot,
updateContainer(
Expand All @@ -223,6 +241,7 @@ export type Reconciler<C, I, TI> = {
unbatchedUpdates<A>(fn: () => A): A,
flushSync<A>(fn: () => A): A,
deferredUpdates<A>(fn: () => A): A,
injectIntoDevTools(devToolsConfig: DevToolsConfig<I, TI>): boolean,

// Used to extract the return value from the initial render. Legacy API.
getPublicRootInstance(
Expand Down Expand Up @@ -327,6 +346,14 @@ export default function<T, P, I, TI, PI, C, CC, CX, PL>(
scheduleWork(current, expirationTime);
}

function findHostInstance(fiber: Fiber): PI | null {
const hostFiber = findCurrentHostFiber(fiber);
if (hostFiber === null) {
return null;
}
return hostFiber.stateNode;
}

return {
createContainer(containerInfo: C, hydrate: boolean): OpaqueRoot {
return createFiberRoot(containerInfo, hydrate);
Expand Down Expand Up @@ -386,13 +413,7 @@ export default function<T, P, I, TI, PI, C, CC, CX, PL>(
}
},

findHostInstance(fiber: Fiber): PI | null {
const hostFiber = findCurrentHostFiber(fiber);
if (hostFiber === null) {
return null;
}
return hostFiber.stateNode;
},
findHostInstance,

findHostInstanceWithNoPortals(fiber: Fiber): PI | null {
const hostFiber = findCurrentHostFiberWithNoPortals(fiber);
Expand All @@ -401,5 +422,22 @@ export default function<T, P, I, TI, PI, C, CC, CX, PL>(
}
return hostFiber.stateNode;
},

injectIntoDevTools(devToolsConfig: DevToolsConfig<I, TI>): boolean {
const {findFiberByHostInstance} = devToolsConfig;
return ReactFiberDevToolsHook.injectInternals({
...devToolsConfig,
findHostInstanceByFiber(fiber: Fiber): I | TI | null {
return findHostInstance(fiber);
},
findFiberByHostInstance(instance: I | TI): Fiber | null {
if (!findFiberByHostInstance) {
// Might not be implemented by the renderer.
return null;
}
return findFiberByHostInstance(instance);
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the wrapper functions necessary? Couldn't we just do...

    injectIntoDevTools(devToolsConfig: DevToolsConfig<I, TI>): boolean {
      const {findFiberByHostInstance} = devToolsConfig;
      const findFiberByHostInstanceImpl = findFiberByHostInstance
        ? findFiberByHostInstance
        : () => null;

      return ReactFiberDevToolsHook.injectInternals({
        ...devToolsConfig,
        findHostInstanceByFiber: findHostInstance,
        findFiberByHostInstance: findFiberByHostInstanceImpl,
      });
    },

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would work too. I don't feel strongly as they're only called once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried using your approach, but it seems like Flow fails to catch some issues for some reason now. I'd prefer keeping it explicit so it's better typed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Boo, Flow.

Okay. No biggie.

},
};
}
5 changes: 1 addition & 4 deletions packages/react-rt-renderer/src/ReactNativeRT.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
showDialog,
} from 'react-native-renderer/src/ReactNativeFiberErrorDialog';
import * as ReactPortal from 'react-reconciler/src/ReactPortal';
import {injectInternals} from 'react-reconciler/src/ReactFiberDevToolsHook';
import * as ReactGenericBatching from 'events/ReactGenericBatching';
import ReactVersion from 'shared/ReactVersion';

Expand Down Expand Up @@ -83,11 +82,9 @@ const ReactNativeRTFiber: ReactNativeRTType = {
flushSync: ReactNativeRTFiberRenderer.flushSync,
};

injectInternals({
ReactNativeRTFiberRenderer.injectIntoDevTools({
findFiberByHostInstance: getFiberFromTag,
findHostInstanceByFiber: ReactNativeRTFiberRenderer.findHostInstance,
getInspectorDataForViewTag: ReactNativeRTFiberInspector.getInspectorDataForViewTag,
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
rendererPackageName: 'react-rt-renderer',
Expand Down