Skip to content

fix[devtools/useMemoCache]: implement a working copy of useMemoCache #27659

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
merged 1 commit into from
Nov 14, 2023
Merged

fix[devtools/useMemoCache]: implement a working copy of useMemoCache #27659

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
50 changes: 43 additions & 7 deletions packages/react-debug-tools/src/ReactDebugHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
ContextProvider,
ForwardRef,
} from 'react-reconciler/src/ReactWorkTags';
import {REACT_MEMO_CACHE_SENTINEL} from 'shared/ReactSymbols';

type CurrentDispatcherRef = typeof ReactSharedInternals.ReactCurrentDispatcher;

Expand Down Expand Up @@ -93,7 +94,9 @@ function getPrimitiveStackCache(): Map<string, Array<any>> {
return primitiveStackCache;
}

let currentFiber: null | Fiber = null;
let currentHook: null | Hook = null;

function nextHook(): null | Hook {
const hook = currentHook;
if (hook !== null) {
Expand Down Expand Up @@ -319,9 +322,31 @@ function useId(): string {

// useMemoCache is an implementation detail of Forget's memoization
// it should not be called directly in user-generated code
// we keep it as a stub for dispatcher
function useMemoCache(size: number): Array<any> {
return [];
const fiber = currentFiber;
// Don't throw, in case this is called from getPrimitiveStackCache
if (fiber == null) {
return [];
}

// $FlowFixMe[incompatible-use]: updateQueue is mixed
const memoCache = fiber.updateQueue?.memoCache;
if (memoCache == null) {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this case need to write the cache?

Where is this implementation coming from?

Copy link
Member

Choose a reason for hiding this comment

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

Compared to

react/packages/react-reconciler/src/ReactFiberHooks.js

Line 1113 in ce2bc58

function useMemoCache(size: number): Array<any> {
it generally seems to be doing a lot less. Could this just call the "real implementation", esp if it doesn't need to do any extra record keeping?

Copy link
Member

Choose a reason for hiding this comment

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

i think the main difference is that it's using syntax sugar which the real version has to avoid because we prefer to avoid the bloated expansion of optional chains and such. this looks right at least, though +1 to just reusing the same implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not do the same thing as the original implementation. Other hook implementations in this file actually don't mutate anything in Fiber or Hook object.

This can't be applied for useMemoCache and the main reason is its implementation: although it is a hook, it is not implemented in the same way as others. It doesn't update memoizedState on the Fiber, so we could see it in the chain with other hooks.

When inspectHooksOfFiber is called, it gets a Fiber as an argument, which is already "rendered", so we could traverse through Fiber.memoizedState and return values from there. For some reason, this doesn't work with useMemoCache, I've tried returning just memoCache.data[memoCache.index], but this implementation produces errors in the render function of the inspected component. I am guessing that it is somewhat related to useMemoCache's implementation.

}

let data = memoCache.data[memoCache.index];
if (data === undefined) {
data = memoCache.data[memoCache.index] = new Array(size);
for (let i = 0; i < size; i++) {
data[i] = REACT_MEMO_CACHE_SENTINEL;
}
}

// We don't write anything to hookLog on purpose, so this hook remains invisible to users.

memoCache.index++;
return data;
}

const Dispatcher: DispatcherType = {
Expand Down Expand Up @@ -699,9 +724,11 @@ export function inspectHooks<Props>(
}

const previousDispatcher = currentDispatcher.current;
let readHookLog;
currentDispatcher.current = DispatcherProxy;

let readHookLog;
let ancestorStackError;

try {
ancestorStackError = new Error();
renderFunction(props);
Expand Down Expand Up @@ -798,19 +825,25 @@ export function inspectHooksOfFiber(
'Unknown Fiber. Needs to be a function component to inspect hooks.',
);
}

// Warm up the cache so that it doesn't consume the currentHook.
getPrimitiveStackCache();

// Set up the current hook so that we can step through and read the
// current state from them.
currentHook = (fiber.memoizedState: Hook);
currentFiber = fiber;

const type = fiber.type;
let props = fiber.memoizedProps;
if (type !== fiber.elementType) {
props = resolveDefaultProps(type, props);
}
// Set up the current hook so that we can step through and read the
// current state from them.
currentHook = (fiber.memoizedState: Hook);
const contextMap = new Map<ReactContext<$FlowFixMe>, $FlowFixMe>();

const contextMap = new Map<ReactContext<any>, any>();
try {
setupContexts(contextMap, fiber);

if (fiber.tag === ForwardRef) {
return inspectHooksOfForwardRef(
type.render,
Expand All @@ -820,9 +853,12 @@ export function inspectHooksOfFiber(
includeHooksSource,
);
}

return inspectHooks(type, props, currentDispatcher, includeHooksSource);
} finally {
currentFiber = null;
currentHook = null;

restoreContexts(contextMap);
}
}
71 changes: 54 additions & 17 deletions packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,27 +633,64 @@ describe('ReactHooksInspectionIntegration', () => {
});
});

// @gate enableUseMemoCacheHook
it('useMemoCache should not be inspectable', () => {
function Foo() {
const $ = useMemoCache(1);
let t0;

if ($[0] === Symbol.for('react.memo_cache_sentinel')) {
t0 = <div>{1}</div>;
$[0] = t0;
} else {
t0 = $[0];
describe('useMemoCache', () => {
// @gate enableUseMemoCacheHook
it('should not be inspectable', () => {
function Foo() {
const $ = useMemoCache(1);
let t0;

if ($[0] === Symbol.for('react.memo_cache_sentinel')) {
t0 = <div>{1}</div>;
$[0] = t0;
} else {
t0 = $[0];
}

return t0;
}

return t0;
}
const renderer = ReactTestRenderer.create(<Foo />);
const childFiber = renderer.root.findByType(Foo)._currentFiber();
const tree = ReactDebugTools.inspectHooksOfFiber(childFiber);

const renderer = ReactTestRenderer.create(<Foo />);
const childFiber = renderer.root.findByType(Foo)._currentFiber();
const tree = ReactDebugTools.inspectHooksOfFiber(childFiber);
expect(tree.length).toEqual(0);
});

// @gate enableUseMemoCacheHook
it('should work in combination with other hooks', () => {
function useSomething() {
const [something] = React.useState(null);
const changeOtherSomething = React.useCallback(() => {}, [something]);

return [something, changeOtherSomething];
}

expect(tree.length).toEqual(0);
function Foo() {
const $ = useMemoCache(10);

useSomething();
React.useState(1);
React.useEffect(() => {});

let t0;

if ($[0] === Symbol.for('react.memo_cache_sentinel')) {
t0 = <div>{1}</div>;
$[0] = t0;
} else {
t0 = $[0];
}

return t0;
}

const renderer = ReactTestRenderer.create(<Foo />);
const childFiber = renderer.root.findByType(Foo)._currentFiber();
const tree = ReactDebugTools.inspectHooksOfFiber(childFiber);

expect(tree.length).toEqual(3);
});
});

describe('useDebugValue', () => {
Expand Down