Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Commit c11c951

Browse files
authored
[crud] Fix deps comparison bug (#31599)
Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
1 parent 64f8951 commit c11c951

3 files changed

Lines changed: 115 additions & 42 deletions

File tree

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

Lines changed: 39 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
253253
update: ((resource: mixed)=>void)|void,
254254
deps: Array<mixed>|void|null,
255255
next: Effect,
256+
identity: ResourceEffectIdentity,
256257
};
257258

258259
typeStoreInstance<T>={
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
25852586
returnpushEffectImpl(effect);
25862587
}
25872588

2588-
function pushResourceEffectIdentity(
2589-
tag: HookFlags,
2589+
function pushResourceEffect(
2590+
identityTag: HookFlags,
2591+
updateTag: HookFlags,
25902592
inst: EffectInstance,
25912593
create: () =>mixed,
2592-
deps: Array<mixed> | void | null,
2594+
createDeps: Array<mixed> | void | null,
2595+
update: ((resource: mixed) =>void)|void,
2596+
updateDeps: Array<mixed> | void | null,
25932597
): Effect {
2594-
consteffect: ResourceEffectIdentity={
2598+
consteffectIdentity: ResourceEffectIdentity={
25952599
resourceKind: ResourceEffectIdentityKind,
2596-
tag,
2600+
tag: identityTag,
25972601
create,
2598-
deps,
2602+
deps: createDeps,
25992603
inst,
26002604
// Circular
26012605
next: (null: any),
26022606
};
2603-
returnpushEffectImpl(effect);
2604-
}
2607+
pushEffectImpl(effectIdentity);
26052608

2606-
function pushResourceEffectUpdate(
2607-
tag: HookFlags,
2608-
inst: EffectInstance,
2609-
update: ((resource: mixed) =>void)|void,
2610-
deps: Array<mixed> | void | null,
2611-
): Effect {
2612-
consteffect: ResourceEffectUpdate={
2609+
consteffectUpdate: ResourceEffectUpdate={
26132610
resourceKind: ResourceEffectUpdateKind,
2614-
tag,
2611+
tag: updateTag,
26152612
update,
2616-
deps,
2613+
deps: updateDeps,
26172614
inst,
2615+
identity: effectIdentity,
26182616
// Circular
26192617
next: (null: any),
26202618
};
2621-
returnpushEffectImpl(effect);
2619+
returnpushEffectImpl(effectUpdate);
26222620
}
26232621

26242622
function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
27922790
currentlyRenderingFiber.flags|=fiberFlags;
27932791
constinst=createEffectInstance();
27942792
inst.destroy=destroy;
2795-
hook.memoizedState=pushResourceEffectIdentity(
2793+
hook.memoizedState=pushResourceEffect(
27962794
HookHasEffect|hookFlags,
2795+
hookFlags,
27972796
inst,
27982797
create,
27992798
createDeps,
2800-
);
2801-
hook.memoizedState=pushResourceEffectUpdate(
2802-
hookFlags,
2803-
inst,
28042799
update,
28052800
updateDeps,
28062801
);
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
28472842
constprevEffect: Effect=currentHook.memoizedState;
28482843
if(nextCreateDeps!==null){
28492844
letprevCreateDeps;
2850-
// Seems sketchy but in practice we always push an Identity and an Update together. For safety
2851-
// we error in DEV if this does not hold true.
2852-
if(prevEffect.resourceKind===ResourceEffectUpdateKind){
2845+
if(
2846+
prevEffect.resourceKind!=null&&
2847+
prevEffect.resourceKind===ResourceEffectUpdateKind
2848+
){
28532849
prevCreateDeps=
2854-
prevEffect.next.deps!=null ? prevEffect.next.deps : null;
2850+
prevEffect.identity.deps!=null ? prevEffect.identity.deps : null;
28552851
}else{
2856-
if(__DEV__){
2857-
console.error(
2858-
'Expected a ResourceEffectUpdateKind to be pushed together with '+
2859-
'ResourceEffectIdentityKind, got %s. This is a bug in React.',
2860-
prevEffect.resourceKind,
2861-
);
2862-
}
2863-
prevCreateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2852+
thrownewError(
2853+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2854+
);
28642855
}
28652856
isCreateDepsSame=areHookInputsEqual(nextCreateDeps,prevCreateDeps);
28662857
}
28672858
if (nextUpdateDeps !== null) {
2868-
constprevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2859+
letprevUpdateDeps;
2860+
if(
2861+
prevEffect.resourceKind!=null&&
2862+
prevEffect.resourceKind===ResourceEffectUpdateKind
2863+
){
2864+
prevUpdateDeps=prevEffect.deps!=null ? prevEffect.deps : null;
2865+
} else {
2866+
thrownewError(
2867+
`Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
2868+
);
2869+
}
28692870
isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
28702871
}
28712872
}
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
28742875
currentlyRenderingFiber.flags|=fiberFlags;
28752876
}
28762877

2877-
hook.memoizedState = pushResourceEffectIdentity(
2878+
hook.memoizedState = pushResourceEffect(
28782879
isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2880+
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
28792881
inst,
28802882
create,
28812883
nextCreateDeps,
2882-
);
2883-
hook.memoizedState = pushResourceEffectUpdate(
2884-
isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
2885-
inst,
28862884
update,
28872885
nextUpdateDeps,
28882886
);

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
39273927
</>,
39283928
);
39293929
});
3930+
3931+
// @gate enableUseResourceEffectHook
3932+
it('composes with other kinds of effects',async()=>{
3933+
letrerender;
3934+
functionApp({id, username}){
3935+
const[count,rerender_]=useState(0);
3936+
rerender=rerender_;
3937+
constopts=useMemo(()=>{
3938+
return{username};
3939+
},[username]);
3940+
useEffect(()=>{
3941+
Scheduler.log(`useEffect(${count})`);
3942+
},[count]);
3943+
useResourceEffect(
3944+
()=>{
3945+
constresource=newResource(id,opts);
3946+
Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
3947+
returnresource;
3948+
},
3949+
[id],
3950+
resource=>{
3951+
resource.update(opts);
3952+
Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
3953+
},
3954+
[opts],
3955+
resource=>{
3956+
resource.destroy();
3957+
Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
3958+
},
3959+
);
3960+
returnnull;
3961+
}
3962+
3963+
awaitact(()=>{
3964+
ReactNoop.render(<Appid={1}username="Jack"/>);
3965+
});
3966+
assertLog(['useEffect(0)','create(1, Jack)']);
3967+
3968+
awaitact(()=>{
3969+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3970+
});
3971+
assertLog(['update(1, Lauren)']);
3972+
3973+
awaitact(()=>{
3974+
ReactNoop.render(<Appid={1}username="Lauren"/>);
3975+
});
3976+
assertLog([]);
3977+
3978+
awaitact(()=>{
3979+
ReactNoop.render(<Appid={1}username="Jordan"/>);
3980+
});
3981+
assertLog(['update(1, Jordan)']);
3982+
3983+
awaitact(()=>{
3984+
rerender(n=>n+1);
3985+
});
3986+
assertLog(['useEffect(1)']);
3987+
3988+
awaitact(()=>{
3989+
ReactNoop.render(<Appid={1}username="Mofei"/>);
3990+
});
3991+
assertLog(['update(1, Mofei)']);
3992+
3993+
awaitact(()=>{
3994+
ReactNoop.render(<Appid={2}username="Jack"/>);
3995+
});
3996+
assertLog(['destroy(1, Mofei)','create(2, Jack)']);
3997+
3998+
awaitact(()=>{
3999+
ReactNoop.render(null);
4000+
});
4001+
assertLog(['destroy(2, Jack)']);
4002+
});
39304003
});
39314004

39324005
describe('useCallback',()=>{

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,5 +527,7 @@
527527
"539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
528528
"540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
529529
"541": "Compared context values must be arrays",
530-
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
530+
"542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
531+
"543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
531532
}
533+

0 commit comments

Comments
 (0)