Skip to content

Commit 313332d

Browse files
potetopoteto
authored
[crud] Revert CRUD overload (#32741)
Cleans up this experiment. After some internal experimentation we are deprioritizing this project for now and may revisit it at a later point.
1 parent f99c9fe commit 313332d

17 files changed

+61
-1453
lines changed

Diff for: ‎packages/react-debug-tools/src/ReactDebugHooks.js

+2-5
Original file line numberDiff line numberDiff line change
@@ -374,11 +374,8 @@ function useInsertionEffect(
374374
}
375375

376376
function useEffect(
377-
create: (() => (() => void) | void) | (() => {...} | void | null),
378-
createDeps: Array<mixed> | void | null,
379-
update?: ((resource: {...} | void | null) => void) | void,
380-
updateDeps?: Array<mixed> | void | null,
381-
destroy?: ((resource: {...} | void | null) => void) | void,
377+
create: () => (() => void) | void,
378+
deps: Array<mixed> | void | null,
382379
): void {
383380
nextHook();
384381
hookLog.push({

Diff for: ‎packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js

-46
Original file line numberDiff line numberDiff line change
@@ -653,52 +653,6 @@ describe('ReactDOMServerHooks', () => {
653653
});
654654
});
655655

656-
describe('useEffect with CRUD overload', () => {
657-
gate(flags => {
658-
if (flags.enableUseEffectCRUDOverload) {
659-
const yields = [];
660-
itRenders(
661-
'should ignore resource effects on the server',
662-
async render => {
663-
function Counter(props) {
664-
useEffect(
665-
() => {
666-
yieldValue('created on client');
667-
return {resource_counter: props.count};
668-
},
669-
[props.count],
670-
resource => {
671-
resource.resource_counter = props.count;
672-
yieldValue('updated on client');
673-
},
674-
[props.count],
675-
() => {
676-
yieldValue('cleanup on client');
677-
},
678-
);
679-
return <Text text={'Count: ' + props.count} />;
680-
}
681-
682-
const domNode = await render(<Counter count={0} />);
683-
yields.push(clearLog());
684-
expect(domNode.tagName).toEqual('SPAN');
685-
expect(domNode.textContent).toEqual('Count: 0');
686-
},
687-
);
688-
689-
it('verifies yields in order', () => {
690-
expect(yields).toEqual([
691-
['Count: 0'], // server render
692-
['Count: 0'], // server stream
693-
['Count: 0', 'created on client'], // clean render
694-
['Count: 0', 'created on client'], // hydrated render
695-
// nothing yielded for bad markup
696-
]);
697-
});
698-
}
699-
});
700-
});
701-
702656
describe('useContext', () => {
703657
itThrowsWhenRendering(
704658
'if used inside a class component',

Diff for: ‎packages/react-reconciler/src/ReactFiberCallUserSpace.js

+5-50
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,6 @@ import type {CapturedValue} from './ReactCapturedValue';
1414

1515
import {isRendering, setIsRendering} from './ReactCurrentFiber';
1616
import {captureCommitPhaseError} from './ReactFiberWorkLoop';
17-
import {
18-
ResourceEffectIdentityKind,
19-
ResourceEffectUpdateKind,
20-
} from './ReactFiberHooks';
21-
import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags';
2217

2318
// These indirections exists so we can exclude its stack frame in DEV (and anything below it).
2419
// TODO: Consider marking the whole bundle instead of these boundaries.
@@ -184,51 +179,11 @@ const callCreate = {
184179
'react-stack-bottom-frame': function (
185180
effect: Effect,
186181
): (() => void) | {...} | void | null {
187-
if (!enableUseEffectCRUDOverload) {
188-
if (effect.resourceKind != null) {
189-
if (__DEV__) {
190-
console.error(
191-
'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' +
192-
'got %s',
193-
effect.resourceKind,
194-
);
195-
}
196-
}
197-
const create = effect.create;
198-
const inst = effect.inst;
199-
// $FlowFixMe[not-a-function] (@poteto)
200-
const destroy = create();
201-
// $FlowFixMe[incompatible-type] (@poteto)
202-
inst.destroy = destroy;
203-
return destroy;
204-
} else {
205-
if (effect.resourceKind == null) {
206-
const create = effect.create;
207-
const inst = effect.inst;
208-
const destroy = create();
209-
inst.destroy = destroy;
210-
return destroy;
211-
}
212-
switch (effect.resourceKind) {
213-
case ResourceEffectIdentityKind: {
214-
return effect.create();
215-
}
216-
case ResourceEffectUpdateKind: {
217-
if (typeof effect.update === 'function') {
218-
effect.update(effect.inst.resource);
219-
}
220-
break;
221-
}
222-
default: {
223-
if (__DEV__) {
224-
console.error(
225-
'Unhandled Effect kind %s. This is a bug in React.',
226-
effect.kind,
227-
);
228-
}
229-
}
230-
}
231-
}
182+
const create = effect.create;
183+
const inst = effect.inst;
184+
const destroy = create();
185+
inst.destroy = destroy;
186+
return destroy;
232187
},
233188
};
234189

Diff for: ‎packages/react-reconciler/src/ReactFiberCommitEffects.js

+7-125
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import {
2323
enableProfilerCommitHooks,
2424
enableProfilerNestedUpdatePhase,
2525
enableSchedulingProfiler,
26-
enableUseEffectCRUDOverload,
2726
enableViewTransition,
2827
enableFragmentRefs,
2928
} from 'shared/ReactFeatureFlags';
@@ -62,7 +61,6 @@ import {
6261
Layout as HookLayout,
6362
Insertion as HookInsertion,
6463
Passive as HookPassive,
65-
HasEffect as HookHasEffect,
6664
} from './ReactHookEffectTags';
6765
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
6866
import {
@@ -84,10 +82,6 @@ import {
8482
} from './ReactFiberCallUserSpace';
8583

8684
import {runWithFiberInDEV} from './ReactCurrentFiber';
87-
import {
88-
ResourceEffectIdentityKind,
89-
ResourceEffectUpdateKind,
90-
} from './ReactFiberHooks';
9185

9286
function shouldProfile(current: Fiber): boolean {
9387
return (
@@ -164,91 +158,19 @@ export function commitHookEffectListMount(
164158

165159
// Mount
166160
let destroy;
167-
if (enableUseEffectCRUDOverload) {
168-
if (effect.resourceKind === ResourceEffectIdentityKind) {
169-
if (__DEV__) {
170-
effect.inst.resource = runWithFiberInDEV(
171-
finishedWork,
172-
callCreateInDEV,
173-
effect,
174-
);
175-
if (effect.inst.resource == null) {
176-
console.error(
177-
'useEffect must provide a callback which returns a resource. ' +
178-
'If a managed resource is not needed here, do not provide an updater or ' +
179-
'destroy callback. Received %s',
180-
effect.inst.resource,
181-
);
182-
}
183-
} else {
184-
effect.inst.resource = effect.create();
185-
}
186-
destroy = effect.inst.destroy;
187-
}
188-
if (effect.resourceKind === ResourceEffectUpdateKind) {
189-
if (
190-
// We don't want to fire updates on remount during Activity
191-
(flags & HookHasEffect) > 0 &&
192-
typeof effect.update === 'function' &&
193-
effect.inst.resource != null
194-
) {
195-
// TODO(@poteto) what about multiple updates?
196-
if (__DEV__) {
197-
runWithFiberInDEV(finishedWork, callCreateInDEV, effect);
198-
} else {
199-
effect.update(effect.inst.resource);
200-
}
201-
}
202-
}
203-
}
204161
if (__DEV__) {
205162
if ((flags & HookInsertion) !== NoHookEffect) {
206163
setIsRunningInsertionEffect(true);
207164
}
208-
if (enableUseEffectCRUDOverload) {
209-
if (effect.resourceKind == null) {
210-
destroy = runWithFiberInDEV(
211-
finishedWork,
212-
callCreateInDEV,
213-
effect,
214-
);
215-
}
216-
} else {
217-
destroy = runWithFiberInDEV(
218-
finishedWork,
219-
callCreateInDEV,
220-
effect,
221-
);
222-
}
165+
destroy = runWithFiberInDEV(finishedWork, callCreateInDEV, effect);
223166
if ((flags & HookInsertion) !== NoHookEffect) {
224167
setIsRunningInsertionEffect(false);
225168
}
226169
} else {
227-
if (enableUseEffectCRUDOverload) {
228-
if (effect.resourceKind == null) {
229-
const create = effect.create;
230-
const inst = effect.inst;
231-
destroy = create();
232-
inst.destroy = destroy;
233-
}
234-
} else {
235-
if (effect.resourceKind != null) {
236-
if (__DEV__) {
237-
console.error(
238-
'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' +
239-
'got %s',
240-
effect.resourceKind,
241-
);
242-
}
243-
}
244-
const create = effect.create;
245-
const inst = effect.inst;
246-
// $FlowFixMe[incompatible-type] (@poteto)
247-
// $FlowFixMe[not-a-function] (@poteto)
248-
destroy = create();
249-
// $FlowFixMe[incompatible-type] (@poteto)
250-
inst.destroy = destroy;
251-
}
170+
const create = effect.create;
171+
const inst = effect.inst;
172+
destroy = create();
173+
inst.destroy = destroy;
252174
}
253175

254176
if (enableSchedulingProfiler) {
@@ -338,13 +260,7 @@ export function commitHookEffectListUnmount(
338260
const inst = effect.inst;
339261
const destroy = inst.destroy;
340262
if (destroy !== undefined) {
341-
if (enableUseEffectCRUDOverload) {
342-
if (effect.resourceKind == null) {
343-
inst.destroy = undefined;
344-
}
345-
} else {
346-
inst.destroy = undefined;
347-
}
263+
inst.destroy = undefined;
348264
if (enableSchedulingProfiler) {
349265
if ((flags & HookPassive) !== NoHookEffect) {
350266
markComponentPassiveEffectUnmountStarted(finishedWork);
@@ -358,41 +274,7 @@ export function commitHookEffectListUnmount(
358274
setIsRunningInsertionEffect(true);
359275
}
360276
}
361-
if (enableUseEffectCRUDOverload) {
362-
if (
363-
effect.resourceKind === ResourceEffectIdentityKind &&
364-
effect.inst.resource != null
365-
) {
366-
safelyCallDestroy(
367-
finishedWork,
368-
nearestMountedAncestor,
369-
destroy,
370-
effect.inst.resource,
371-
);
372-
if (effect.next.resourceKind === ResourceEffectUpdateKind) {
373-
// $FlowFixMe[prop-missing] (@poteto)
374-
effect.next.update = undefined;
375-
} else {
376-
if (__DEV__) {
377-
console.error(
378-
'Expected a ResourceEffectUpdateKind to follow ResourceEffectIdentityKind, ' +
379-
'got %s. This is a bug in React.',
380-
effect.next.resourceKind,
381-
);
382-
}
383-
}
384-
effect.inst.resource = null;
385-
}
386-
if (effect.resourceKind == null) {
387-
safelyCallDestroy(
388-
finishedWork,
389-
nearestMountedAncestor,
390-
destroy,
391-
);
392-
}
393-
} else {
394-
safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
395-
}
277+
safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
396278
if (__DEV__) {
397279
if ((flags & HookInsertion) !== NoHookEffect) {
398280
setIsRunningInsertionEffect(false);

0 commit comments

Comments
 (0)