Skip to content

Commit

Permalink
Re-enable "view source" button for standalone shell
Browse files Browse the repository at this point in the history
But only do this if we can verify the element file path. This hopefully avoids the case where clicking the button does nothing because of an invalid/incomplete path.
  • Loading branch information
Brian Vaughn committed Jul 25, 2019
1 parent c6fb743 commit 1e8aa81
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,31 +102,44 @@ function guessEditor(): Array<string> {

let childProcess = null;

export default function launchEditor(
export function getValidFilePath(
maybeRelativePath: string,
lineNumber: number,
absoluteProjectRoots: Array<string>
) {
): string | null {
// We use relative paths at Facebook with deterministic builds.
// This is why our internal tooling calls React DevTools with absoluteProjectRoots.
// If the filename is absolute then we don't need to care about this.
let filePath;
if (isAbsolute(maybeRelativePath)) {
if (existsSync(maybeRelativePath)) {
filePath = maybeRelativePath;
return maybeRelativePath;
}
} else {
for (let i = 0; i < absoluteProjectRoots.length; i++) {
const projectRoot = absoluteProjectRoots[i];
const joinedPath = join(projectRoot, maybeRelativePath);
if (existsSync(joinedPath)) {
filePath = joinedPath;
break;
return joinedPath;
}
}
}

if (!filePath) {
return null;
}

export function doesFilePathExist(
maybeRelativePath: string,
absoluteProjectRoots: Array<string>
): boolean {
return getValidFilePath(maybeRelativePath, absoluteProjectRoots) !== null;
}

export function launchEditor(
maybeRelativePath: string,
lineNumber: number,
absoluteProjectRoots: Array<string>
) {
const filePath = getValidFilePath(maybeRelativePath, absoluteProjectRoots);
if (filePath === null) {
return;
}

Expand Down
24 changes: 18 additions & 6 deletions packages/react-devtools-core/src/standalone.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { Server } from 'ws';
import { existsSync, readFileSync } from 'fs';
import { installHook } from 'src/hook';
import DevTools from 'src/devtools/views/DevTools';
import launchEditor from './launchEditor';
import { doesFilePathExist, launchEditor } from './editor';
import { __DEBUG__ } from 'src/constants';

import type { FrontendBridge } from 'src/bridge';
Expand Down Expand Up @@ -85,16 +85,31 @@ function reload() {
root.render(
createElement(DevTools, {
bridge: ((bridge: any): FrontendBridge),
canViewElementSourceFunction,
showTabBar: true,
store: ((store: any): Store),
warnIfLegacyBackendDetected: true,
viewElementSourceFunction,
viewElementSourceRequiresFileLocation: true,
})
);
}, 100);
}

function canViewElementSourceFunction(
inspectedElement: InspectedElement
): boolean {
if (
inspectedElement.canViewSource === false ||
inspectedElement.source === null
) {
return false;
}

const { source } = inspectedElement;

return doesFilePathExist(source.fileName, projectRoots);
}

function viewElementSourceFunction(
id: number,
inspectedElement: InspectedElement
Expand Down Expand Up @@ -171,10 +186,7 @@ function initialize(socket: WebSocket) {
socket.close();
});

store = new Store(bridge, {
supportsNativeInspection: false,
supportsViewSource: projectRoots.length > 0,
});
store = new Store(bridge, { supportsNativeInspection: false });

log('Connected');
reload();
Expand Down
8 changes: 0 additions & 8 deletions src/devtools/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ type Config = {|
supportsNativeInspection?: boolean,
supportsReloadAndProfile?: boolean,
supportsProfiling?: boolean,
supportsViewSource?: boolean,
|};

export type Capabilities = {|
Expand Down Expand Up @@ -125,7 +124,6 @@ export default class Store extends EventEmitter<{|
_supportsNativeInspection: boolean = false;
_supportsProfiling: boolean = false;
_supportsReloadAndProfile: boolean = false;
_supportsViewSource: boolean = true;

// Total number of visible elements (within all roots).
// Used for windowing purposes.
Expand Down Expand Up @@ -157,15 +155,13 @@ export default class Store extends EventEmitter<{|
supportsNativeInspection,
supportsProfiling,
supportsReloadAndProfile,
supportsViewSource,
} = config;
if (supportsCaptureScreenshots) {
this._supportsCaptureScreenshots = true;
this._captureScreenshots =
localStorageGetItem(LOCAL_STORAGE_CAPTURE_SCREENSHOTS_KEY) === 'true';
}
this._supportsNativeInspection = supportsNativeInspection !== false;
this._supportsViewSource = supportsViewSource !== false;
if (supportsProfiling) {
this._supportsProfiling = true;
}
Expand Down Expand Up @@ -365,10 +361,6 @@ export default class Store extends EventEmitter<{|
return this._supportsReloadAndProfile && this._isBackendStorageAPISupported;
}

get supportsViewSource(): boolean {
return this._supportsViewSource;
}

containsElement(id: number): boolean {
return this._idToElement.get(id) != null;
}
Expand Down
32 changes: 17 additions & 15 deletions src/devtools/views/Components/SelectedElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ export type Props = {||};
export default function SelectedElement(_: Props) {
const { inspectedElementID } = useContext(TreeStateContext);
const dispatch = useContext(TreeDispatcherContext);
const { isFileLocationRequired, viewElementSourceFunction } = useContext(
ViewElementSourceContext
);
const {
canViewElementSourceFunction,
viewElementSourceFunction,
} = useContext(ViewElementSourceContext);
const bridge = useContext(BridgeContext);
const store = useContext(StoreContext);
const { dispatch: modalDialogDispatch } = useContext(ModalDialogContext);
Expand Down Expand Up @@ -90,11 +91,14 @@ export default function SelectedElement(_: Props) {
}
}, [inspectedElement, viewElementSourceFunction]);

// In some cases (e.g. FB internal usage) the standalone shell might not be able to view the source.
// To detect this case, we defer to an injected helper function (if present).
const canViewSource =
inspectedElement &&
inspectedElement !== null &&
inspectedElement.canViewSource &&
viewElementSourceFunction !== null &&
(!isFileLocationRequired || inspectedElement.source !== null);
(canViewElementSourceFunction === null ||
canViewElementSourceFunction(inspectedElement));

const isSuspended =
element !== null &&
Expand Down Expand Up @@ -201,16 +205,14 @@ export default function SelectedElement(_: Props) {
>
<ButtonIcon type="log-data" />
</Button>
{store.supportsViewSource && (
<Button
className={styles.IconButton}
disabled={!canViewSource}
onClick={viewSource}
title="View source for this element"
>
<ButtonIcon type="view-source" />
</Button>
)}
<Button
className={styles.IconButton}
disabled={!canViewSource}
onClick={viewSource}
title="View source for this element"
>
<ButtonIcon type="view-source" />
</Button>
</div>

{inspectedElement === null && (
Expand Down
7 changes: 5 additions & 2 deletions src/devtools/views/Components/ViewElementSourceContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

import { createContext } from 'react';

import type { ViewElementSource } from 'src/devtools/views/DevTools';
import type {
CanViewElementSource,
ViewElementSource,
} from 'src/devtools/views/DevTools';

export type Context = {|
isFileLocationRequired: boolean,
canViewElementSourceFunction: CanViewElementSource | null,
viewElementSourceFunction: ViewElementSource | null,
|};

Expand Down
15 changes: 9 additions & 6 deletions src/devtools/views/DevTools.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,19 @@ export type ViewElementSource = (
id: number,
inspectedElement: InspectedElement
) => void;
export type CanViewElementSource = (
inspectedElement: InspectedElement
) => boolean;

export type Props = {|
bridge: FrontendBridge,
browserTheme?: BrowserTheme,
canViewElementSourceFunction?: ?CanViewElementSource,
defaultTab?: TabID,
showTabBar?: boolean,
store: Store,
warnIfLegacyBackendDetected?: boolean,
viewElementSourceFunction?: ?ViewElementSource,
viewElementSourceRequiresFileLocation?: boolean,

// This property is used only by the web extension target.
// The built-in tab UI is hidden in that case, in favor of the browser's own panel tabs.
Expand Down Expand Up @@ -75,6 +78,7 @@ const tabs = [componentsTab, profilerTab];
export default function DevTools({
bridge,
browserTheme = 'light',
canViewElementSourceFunction = null,
defaultTab = 'components',
componentsPortalContainer,
overrideTab,
Expand All @@ -83,8 +87,7 @@ export default function DevTools({
showTabBar = false,
store,
warnIfLegacyBackendDetected = false,
viewElementSourceFunction,
viewElementSourceRequiresFileLocation = false,
viewElementSourceFunction = null,
}: Props) {
const [tab, setTab] = useState(defaultTab);
if (overrideTab != null && overrideTab !== tab) {
Expand All @@ -93,10 +96,10 @@ export default function DevTools({

const viewElementSource = useMemo(
() => ({
isFileLocationRequired: viewElementSourceRequiresFileLocation,
viewElementSourceFunction: viewElementSourceFunction || null,
canViewElementSourceFunction,
viewElementSourceFunction,
}),
[viewElementSourceFunction, viewElementSourceRequiresFileLocation]
[canViewElementSourceFunction, viewElementSourceFunction]
);

return (
Expand Down

0 comments on commit 1e8aa81

Please sign in to comment.