-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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.
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
Add named hooks support to react-devtools-inline #22263
Changes from all commits
08c3fcb
06fab2d
b941b03
398dacd
f4c7df5
22d2eb2
7c8609d
e89d47d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./dist/hookNames'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/** @flow */ | ||
|
||
import { | ||
parseHookNames, | ||
parseSourceAndMetadata, | ||
purgeCachedMetadata, | ||
} from 'react-devtools-shared/src/hooks/parseHookNames'; | ||
|
||
export {parseHookNames, parseSourceAndMetadata, purgeCachedMetadata}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow | ||
*/ | ||
|
||
import {createContext} from 'react'; | ||
|
||
export type FetchFileWithCaching = (url: string) => Promise<string>; | ||
export type Context = FetchFileWithCaching | null; | ||
|
||
const FetchFileWithCachingContext = createContext<Context>(null); | ||
FetchFileWithCachingContext.displayName = 'FetchFileWithCachingContext'; | ||
|
||
export default FetchFileWithCachingContext; |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow | ||
*/ | ||
|
||
import type {Thenable} from 'shared/ReactTypes'; | ||
|
||
import {createContext} from 'react'; | ||
import typeof * as ParseHookNamesModule from 'react-devtools-shared/src/hooks/parseHookNames'; | ||
|
||
export type HookNamesModuleLoaderFunction = () => Thenable<ParseHookNamesModule>; | ||
export type Context = HookNamesModuleLoaderFunction | null; | ||
|
||
// TODO (Webpack 5) Hopefully we can remove this context entirely once the Webpack 5 upgrade is completed. | ||
const HookNamesModuleLoaderContext = createContext<Context>(null); | ||
HookNamesModuleLoaderContext.displayName = 'HookNamesModuleLoaderContext'; | ||
|
||
export default HookNamesModuleLoaderContext; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,11 @@ import { | |
hasAlreadyLoadedHookNames, | ||
loadHookNames, | ||
} from 'react-devtools-shared/src/hookNamesCache'; | ||
import HookNamesContext from 'react-devtools-shared/src/devtools/views/Components/HookNamesContext'; | ||
import {loadModule} from 'react-devtools-shared/src/dynamicImportCache'; | ||
import FetchFileWithCachingContext from 'react-devtools-shared/src/devtools/views/Components/FetchFileWithCachingContext'; | ||
import HookNamesModuleLoaderContext from 'react-devtools-shared/src/devtools/views/Components/HookNamesModuleLoaderContext'; | ||
import {SettingsContext} from '../Settings/SettingsContext'; | ||
import {enableNamedHooksFeature} from 'react-devtools-feature-flags'; | ||
|
||
import type {HookNames} from 'react-devtools-shared/src/types'; | ||
import type {ReactNodeList} from 'shared/ReactTypes'; | ||
|
@@ -64,16 +67,17 @@ export type Props = {| | |
|
||
export function InspectedElementContextController({children}: Props) { | ||
const {selectedElementID} = useContext(TreeStateContext); | ||
const { | ||
fetchFileWithCaching, | ||
loadHookNames: loadHookNamesFunction, | ||
prefetchSourceFiles, | ||
purgeCachedMetadata, | ||
} = useContext(HookNamesContext); | ||
const fetchFileWithCaching = useContext(FetchFileWithCachingContext); | ||
const bridge = useContext(BridgeContext); | ||
const store = useContext(StoreContext); | ||
const {parseHookNames: parseHookNamesByDefault} = useContext(SettingsContext); | ||
|
||
// parseHookNames has a lot of code. | ||
// Embedding it into a build makes the build large. | ||
// This function enables DevTools to make use of Suspense to lazily import() it only if the feature will be used. | ||
// TODO (Webpack 5) Hopefully we can remove this indirection once the Webpack 5 upgrade is completed. | ||
const hookNamesModuleLoader = useContext(HookNamesModuleLoaderContext); | ||
bvaughn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const refresh = useCacheRefresh(); | ||
|
||
// Temporarily stores most recently-inspected (hydrated) path. | ||
|
@@ -113,24 +117,40 @@ export function InspectedElementContextController({children}: Props) { | |
setParseHookNames(parseHookNamesByDefault || alreadyLoadedHookNames); | ||
} | ||
|
||
const purgeCachedMetadataRef = useRef(null); | ||
|
||
// Don't load a stale element from the backend; it wastes bridge bandwidth. | ||
let hookNames: HookNames | null = null; | ||
let inspectedElement = null; | ||
if (!elementHasChanged && element !== null) { | ||
inspectedElement = inspectElement(element, state.path, store, bridge); | ||
|
||
if (parseHookNames || alreadyLoadedHookNames) { | ||
if ( | ||
inspectedElement !== null && | ||
inspectedElement.hooks !== null && | ||
loadHookNamesFunction !== null | ||
) { | ||
hookNames = loadHookNames( | ||
element, | ||
inspectedElement.hooks, | ||
loadHookNamesFunction, | ||
fetchFileWithCaching, | ||
); | ||
if (enableNamedHooksFeature) { | ||
if (typeof hookNamesModuleLoader === 'function') { | ||
if (parseHookNames || alreadyLoadedHookNames) { | ||
const hookNamesModule = loadModule(hookNamesModuleLoader); | ||
if (hookNamesModule !== null) { | ||
const { | ||
parseHookNames: loadHookNamesFunction, | ||
purgeCachedMetadata, | ||
} = hookNamesModule; | ||
|
||
purgeCachedMetadataRef.current = purgeCachedMetadata; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a little clunky, but since we conditionally load this via Suspense, and refer to it in an effect, we have to stash it in a ref. |
||
|
||
if ( | ||
inspectedElement !== null && | ||
inspectedElement.hooks !== null && | ||
loadHookNamesFunction !== null | ||
) { | ||
hookNames = loadHookNames( | ||
element, | ||
inspectedElement.hooks, | ||
loadHookNamesFunction, | ||
fetchFileWithCaching, | ||
); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -163,14 +183,11 @@ export function InspectedElementContextController({children}: Props) { | |
inspectedElementRef.current !== inspectedElement | ||
) { | ||
inspectedElementRef.current = inspectedElement; | ||
|
||
if (typeof prefetchSourceFiles === 'function') { | ||
prefetchSourceFiles(inspectedElement.hooks, fetchFileWithCaching); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This optimization wasn't really important given the in-memory caching we now do, so I cleaned it up while I was in here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't this help speed things up in the case where the source files are loaded before devtools is open? |
||
} | ||
}, [inspectedElement, prefetchSourceFiles]); | ||
}, [inspectedElement]); | ||
|
||
useEffect(() => { | ||
const purgeCachedMetadata = purgeCachedMetadataRef.current; | ||
if (typeof purgeCachedMetadata === 'function') { | ||
// When Fast Refresh updates a component, any cached AST metadata may be invalid. | ||
const fastRefreshScheduled = () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the dynamic webpack public path override also necessary to get this working in OSS or is that something that's only necessary in
react-devtools-shell
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm not sure. Maybe it depends on which version of Webpack other projects are using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If #22267 doesn't land, should we add that as a note in the README as well? It feels a bit unintuitive for developers otherwise (it definitely was for us 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...let's see what happens 😅 I'm not a Webpack expert so maybe there's a better way to do this than what I did.
TBH I don't think anyone (other than us, Code Sandbox, and Replay.io) uses the inline package.