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

[Experimental] Add useInsertionEffect #21913

Merged
merged 1 commit into from
Sep 14, 2021
Merged

[Experimental] Add useInsertionEffect #21913

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
14 changes: 14 additions & 0 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 @@ -78,6 +78,7 @@ function getPrimitiveStackCache(): Map<string, Array<any>> {
Dispatcher.useCacheRefresh();
}
Dispatcher.useLayoutEffect(() => {});
Dispatcher.useInsertionEffect(() => {});
Dispatcher.useEffect(() => {});
Dispatcher.useImperativeHandle(undefined, () => null);
Dispatcher.useDebugValue(null);
Expand Down Expand Up @@ -191,6 +192,18 @@ function useLayoutEffect(
});
}

function useInsertionEffect(
Copy link
Collaborator

@gaearon gaearon Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Didn't we decide to drop the ion and just do useInsertEffect? timberlake.gif

There is a legit noun.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and then no. useInsertEffect sounds too much like the thing you're inserting is an effect, which I think we noticed when we started talking about it again and thought of it as a verb first (which is probably the natural reading of it).

create: () => mixed,
inputs: Array<mixed> | void | null,
): void {
nextHook();
hookLog.push({
primitive: 'InsertionEffect',
stackError: new Error(),
value: create,
});
}

function useEffect(
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
Expand Down Expand Up @@ -339,6 +352,7 @@ const Dispatcher: DispatcherType = {
useImperativeHandle,
useDebugValue,
useLayoutEffect,
useInsertionEffect,
useMemo,
useReducer,
useRef,
Expand Down
177 changes: 177 additions & 0 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 @@ -268,6 +268,183 @@ describe('ReactHooksInspectionIntegration', () => {
]);
});

// @gate experimental || www
it('should inspect the current state of all stateful hooks, including useInsertionEffect', () => {
const useInsertionEffect = React.unstable_useInsertionEffect;
const outsideRef = React.createRef();
function effect() {}
function Foo(props) {
const [state1, setState] = React.useState('a');
const [state2, dispatch] = React.useReducer((s, a) => a.value, 'b');
const ref = React.useRef('c');

useInsertionEffect(effect);
React.useLayoutEffect(effect);
React.useEffect(effect);

React.useImperativeHandle(
outsideRef,
() => {
// Return a function so that jest treats them as non-equal.
return function Instance() {};
},
[],
);

React.useMemo(() => state1 + state2, [state1]);

function update() {
act(() => {
setState('A');
});
act(() => {
dispatch({value: 'B'});
});
ref.current = 'C';
}
const memoizedUpdate = React.useCallback(update, []);
return (
<div onClick={memoizedUpdate}>
{state1} {state2}
</div>
);
}
let renderer;
act(() => {
renderer = ReactTestRenderer.create(<Foo prop="prop" />);
});

let childFiber = renderer.root.findByType(Foo)._currentFiber();

const {onClick: updateStates} = renderer.root.findByType('div').props;

let tree = ReactDebugTools.inspectHooksOfFiber(childFiber);
expect(tree).toEqual([
{
isStateEditable: true,
id: 0,
name: 'State',
value: 'a',
subHooks: [],
},
{
isStateEditable: true,
id: 1,
name: 'Reducer',
value: 'b',
subHooks: [],
},
{isStateEditable: false, id: 2, name: 'Ref', value: 'c', subHooks: []},
{
isStateEditable: false,
id: 3,
name: 'InsertionEffect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 4,
name: 'LayoutEffect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 5,
name: 'Effect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 6,
name: 'ImperativeHandle',
value: outsideRef.current,
subHooks: [],
},
{
isStateEditable: false,
id: 7,
name: 'Memo',
value: 'ab',
subHooks: [],
},
{
isStateEditable: false,
id: 8,
name: 'Callback',
value: updateStates,
subHooks: [],
},
]);

updateStates();

childFiber = renderer.root.findByType(Foo)._currentFiber();
tree = ReactDebugTools.inspectHooksOfFiber(childFiber);

expect(tree).toEqual([
{
isStateEditable: true,
id: 0,
name: 'State',
value: 'A',
subHooks: [],
},
{
isStateEditable: true,
id: 1,
name: 'Reducer',
value: 'B',
subHooks: [],
},
{isStateEditable: false, id: 2, name: 'Ref', value: 'C', subHooks: []},
{
isStateEditable: false,
id: 3,
name: 'InsertionEffect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 4,
name: 'LayoutEffect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 5,
name: 'Effect',
value: effect,
subHooks: [],
},
{
isStateEditable: false,
id: 6,
name: 'ImperativeHandle',
value: outsideRef.current,
subHooks: [],
},
{
isStateEditable: false,
id: 7,
name: 'Memo',
value: 'Ab',
subHooks: [],
},
{
isStateEditable: false,
id: 8,
name: 'Callback',
value: updateStates,
subHooks: [],
},
]);
});

it('should inspect the value of the current provider in useContext', () => {
const MyContext = React.createContext('default');
function Foo(props) {
Expand Down
18 changes: 18 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ let useCallback;
let useMemo;
let useRef;
let useImperativeHandle;
let useInsertionEffect;
let useLayoutEffect;
let useDebugValue;
let useOpaqueIdentifier;
Expand Down Expand Up @@ -54,6 +55,7 @@ function initModules() {
useRef = React.useRef;
useDebugValue = React.useDebugValue;
useImperativeHandle = React.useImperativeHandle;
useInsertionEffect = React.unstable_useInsertionEffect;
useLayoutEffect = React.useLayoutEffect;
useOpaqueIdentifier = React.unstable_useOpaqueIdentifier;
forwardRef = React.forwardRef;
Expand Down Expand Up @@ -638,6 +640,22 @@ describe('ReactDOMServerHooks', () => {
expect(domNode.textContent).toEqual('Count: 0');
});
});
describe('useInsertionEffect', () => {
// @gate experimental || www
it('should warn when invoked during render', async () => {
function Counter() {
useInsertionEffect(() => {
throw new Error('should not be invoked');
});

return <Text text="Count: 0" />;
}
const domNode = await serverRender(<Counter />, 1);
expect(clearYields()).toEqual(['Count: 0']);
expect(domNode.tagName).toEqual('SPAN');
expect(domNode.textContent).toEqual('Count: 0');
});
});

describe('useLayoutEffect', () => {
it('should warn when invoked during render', async () => {
Expand Down
17 changes: 17 additions & 0 deletions packages/react-dom/src/server/ReactPartialRendererHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,22 @@ function useRef<T>(initialValue: T): {|current: T|} {
}
}

function useInsertionEffect(
create: () => mixed,
inputs: Array<mixed> | void | null,
) {
if (__DEV__) {
currentHookNameInDev = 'useInsertionEffect';
console.error(
'useInsertionEffect does nothing on the server, because its effect cannot ' +
"be encoded into the server renderer's output format. This will lead " +
'to a mismatch between the initial, non-hydrated UI and the intended ' +
'UI. To avoid this, useInsertionEffect should only be used in ' +
'components that render exclusively on the client.',
);
}
}
Comment on lines +388 to +402
Copy link
Contributor

Choose a reason for hiding this comment

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

How are CSS-in-JS libraries supposed to handle SSR then? If the main use for this hook is stylesheet libraries then I think we need to document a compelling strategy for how those should be implemented "end to end".

In Emotion we currently conditionally render <style/> tags "inline" in the SSRed output and expect our hydration function to move them to <head/> (most commonly) before React "sees" them - it's tricky, but it sort of works (at least right now). Is this an acceptable approach?

Especially that last part of this error message seems to be off - given the primary use case for this hook. Style-based components cannot be exclusively rendered on the client because that would almost completely obliviate the sense of SSRing applications using those kinds of libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Andarist short term they'll insert them into the stream separately, chunk points. @sebmarkbage is writing up a tutorial for how to do that as a working group post. Long term, we have an RFC we're going to publish for how we intend to handle this in an integrated way that works isomorphically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, thanks for the comment and I can't wait to read how this will work 👍

However, that probably means that the last paragraph of my comment is still valid - the error message here seems to be somewhat inaccurate and could throw somebody off. Unless you expect library maintainers to ignore that and "know better". 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably wrong in the same sense as the useLayoutEffect warning being wrong since it can also be done differently on the server to inject through different means and needs to be done conditionally to workaround these issues.


export function useLayoutEffect(
create: () => (() => void) | void,
inputs: Array<mixed> | void | null,
Expand Down Expand Up @@ -508,6 +524,7 @@ export const Dispatcher: DispatcherType = {
useReducer,
useRef,
useState,
useInsertionEffect,
useLayoutEffect,
useCallback,
// useImperativeHandle is not run in the server environment
Expand Down
21 changes: 20 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ import {
NoFlags as NoHookEffect,
HasEffect as HookHasEffect,
Layout as HookLayout,
Insertion as HookInsertion,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new';
Expand Down Expand Up @@ -525,6 +526,8 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) {
let hookName;
if ((effect.tag & HookLayout) !== NoFlags) {
hookName = 'useLayoutEffect';
} else if ((effect.tag & HookInsertion) !== NoFlags) {
salazarm marked this conversation as resolved.
Show resolved Hide resolved
hookName = 'useInsertionEffect';
} else {
hookName = 'useEffect';
}
Expand Down Expand Up @@ -1153,7 +1156,10 @@ function commitUnmount(
do {
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookLayout) !== NoHookEffect) {
if (
(tag & HookInsertion) !== NoHookEffect ||
(tag & HookLayout) !== NoHookEffect
) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
Expand Down Expand Up @@ -1738,6 +1744,13 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
commitHookEffectListUnmount(
HookInsertion | HookHasEffect,
finishedWork,
finishedWork.return,
);
commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork);

// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
Expand Down Expand Up @@ -1810,6 +1823,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
commitHookEffectListUnmount(
HookInsertion | HookHasEffect,
finishedWork,
finishedWork.return,
);
commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork);
// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
Expand Down
Loading