From 1e8aa8105ab10ee7c4ec49a405fcc7976820da4c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 25 Jul 2019 09:28:45 -0700 Subject: [PATCH] Re-enable "view source" button for standalone shell 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. --- .../src/{launchEditor.js => editor.js} | 29 ++++++++++++----- .../react-devtools-core/src/standalone.js | 24 ++++++++++---- src/devtools/store.js | 8 ----- .../views/Components/SelectedElement.js | 32 ++++++++++--------- .../Components/ViewElementSourceContext.js | 7 ++-- src/devtools/views/DevTools.js | 15 +++++---- 6 files changed, 70 insertions(+), 45 deletions(-) rename packages/react-devtools-core/src/{launchEditor.js => editor.js} (90%) diff --git a/packages/react-devtools-core/src/launchEditor.js b/packages/react-devtools-core/src/editor.js similarity index 90% rename from packages/react-devtools-core/src/launchEditor.js rename to packages/react-devtools-core/src/editor.js index 94628be8582e4..26043f07f650d 100644 --- a/packages/react-devtools-core/src/launchEditor.js +++ b/packages/react-devtools-core/src/editor.js @@ -102,31 +102,44 @@ function guessEditor(): Array { let childProcess = null; -export default function launchEditor( +export function getValidFilePath( maybeRelativePath: string, - lineNumber: number, absoluteProjectRoots: Array -) { +): 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 +): boolean { + return getValidFilePath(maybeRelativePath, absoluteProjectRoots) !== null; +} + +export function launchEditor( + maybeRelativePath: string, + lineNumber: number, + absoluteProjectRoots: Array +) { + const filePath = getValidFilePath(maybeRelativePath, absoluteProjectRoots); + if (filePath === null) { return; } diff --git a/packages/react-devtools-core/src/standalone.js b/packages/react-devtools-core/src/standalone.js index 27774b380d96d..a88691d77ed8c 100644 --- a/packages/react-devtools-core/src/standalone.js +++ b/packages/react-devtools-core/src/standalone.js @@ -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'; @@ -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 @@ -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(); diff --git a/src/devtools/store.js b/src/devtools/store.js index b843831e1deee..adaccfe24b27b 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -49,7 +49,6 @@ type Config = {| supportsNativeInspection?: boolean, supportsReloadAndProfile?: boolean, supportsProfiling?: boolean, - supportsViewSource?: boolean, |}; export type Capabilities = {| @@ -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. @@ -157,7 +155,6 @@ export default class Store extends EventEmitter<{| supportsNativeInspection, supportsProfiling, supportsReloadAndProfile, - supportsViewSource, } = config; if (supportsCaptureScreenshots) { this._supportsCaptureScreenshots = true; @@ -165,7 +162,6 @@ export default class Store extends EventEmitter<{| localStorageGetItem(LOCAL_STORAGE_CAPTURE_SCREENSHOTS_KEY) === 'true'; } this._supportsNativeInspection = supportsNativeInspection !== false; - this._supportsViewSource = supportsViewSource !== false; if (supportsProfiling) { this._supportsProfiling = true; } @@ -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; } diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index 957d844421842..30e9bd527014d 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -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); @@ -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 && @@ -201,16 +205,14 @@ export default function SelectedElement(_: Props) { > - {store.supportsViewSource && ( - - )} + {inspectedElement === null && ( diff --git a/src/devtools/views/Components/ViewElementSourceContext.js b/src/devtools/views/Components/ViewElementSourceContext.js index 0e76f7c56eb3f..30a3f017af20f 100644 --- a/src/devtools/views/Components/ViewElementSourceContext.js +++ b/src/devtools/views/Components/ViewElementSourceContext.js @@ -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, |}; diff --git a/src/devtools/views/DevTools.js b/src/devtools/views/DevTools.js index c63113dda54b6..0dd3bd3ed22d5 100644 --- a/src/devtools/views/DevTools.js +++ b/src/devtools/views/DevTools.js @@ -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. @@ -75,6 +78,7 @@ const tabs = [componentsTab, profilerTab]; export default function DevTools({ bridge, browserTheme = 'light', + canViewElementSourceFunction = null, defaultTab = 'components', componentsPortalContainer, overrideTab, @@ -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) { @@ -93,10 +96,10 @@ export default function DevTools({ const viewElementSource = useMemo( () => ({ - isFileLocationRequired: viewElementSourceRequiresFileLocation, - viewElementSourceFunction: viewElementSourceFunction || null, + canViewElementSourceFunction, + viewElementSourceFunction, }), - [viewElementSourceFunction, viewElementSourceRequiresFileLocation] + [canViewElementSourceFunction, viewElementSourceFunction] ); return (