Skip to content

Commit

Permalink
Refreshes should not affect "sibling" boundaries
Browse files Browse the repository at this point in the history 
I had thought we decided that refreshing a boundary would also refresh
all the content that is currently consistent (i.e. shared the same
underlying cache) with it, but I was wrong. Refreshing should only
affect the nearest tree and its descendents. "Sibling" content will
intentionally be inconsistent after the refresh.

This allows me to drop the subscription stuff, which is nice.
  • Loading branch information
@acdlite
acdlite committed Dec 14, 2020
1 parent 92685bd commit b13ea9e
Show file tree
Hide file tree
Showing 14 changed files with 178 additions and 262 deletions.
45 changes: 26 additions & 19 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {
OffscreenProps,
OffscreenState,
} from './ReactFiberOffscreenComponent';
import type {Cache} from './ReactFiberCacheComponent';
import type {CacheInstance} from './ReactFiberCacheComponent';

import checkPropTypes from 'shared/checkPropTypes';

Expand Down Expand Up @@ -667,17 +667,17 @@ function updateCacheComponent(
// Read directly from the context. We don't set up a context dependency
// because the propagation function automatically includes CacheComponents in
// its search.
const parentCache: Cache | null = isPrimaryRenderer
const parentCache: CacheInstance | null = isPrimaryRenderer
? CacheContext._currentValue
: CacheContext._currentValue2;

let ownCache: Cache | null = null;
let ownCacheInstance: CacheInstance | null = null;
// TODO: Fast path if parent has a new cache. Merely an optimization. Might
// not be worth it.
if (false) {
// The parent boundary also has a new cache. We're either inside a new tree,
// or there was a refresh. In both cases, we should use the parent cache.
ownCache = null;
ownCacheInstance = null;
} else {
if (current === null) {
// This is a newly mounted component. Request a fresh cache.
Expand All @@ -692,12 +692,15 @@ function updateCacheComponent(
// spawned from a previous render that already committed. Otherwise, this
// is the root of a cache consistency boundary.
if (freshCache !== parentCache) {
ownCache = freshCache;
pushProvider(workInProgress, CacheContext, freshCache);
ownCacheInstance = {
cache: freshCache,
provider: workInProgress,
};
pushProvider(workInProgress, CacheContext, ownCacheInstance);
// No need to propagate the refresh, because this is a new tree.
} else {
// Use the parent cache
ownCache = null;
ownCacheInstance = null;
}
} else {
// This component already mounted.
Expand All @@ -711,8 +714,11 @@ function updateCacheComponent(
);
const freshCache = requestFreshCache(root, renderLanes);
if (freshCache !== parentCache) {
ownCache = freshCache;
pushProvider(workInProgress, CacheContext, freshCache);
ownCacheInstance = {
cache: freshCache,
provider: workInProgress,
};
pushProvider(workInProgress, CacheContext, ownCacheInstance);
// Refreshes propagate through the entire subtree. The refreshed cache
// will override nested caches.
propagateCacheRefresh(workInProgress, renderLanes);
Expand All @@ -721,17 +727,17 @@ function updateCacheComponent(
// unreachable in practice, because this means the parent cache was
// refreshed in the same render. So we would have already handled this
// in the earlier branch, where we check if the parent is new.
ownCache = null;
ownCacheInstance = null;
}
} else {
// Reuse the memoized cache.
const prevCache: Cache | null = current.memoizedState;
if (prevCache !== null) {
ownCache = prevCache;
const prevCacheInstance: CacheInstance | null = current.memoizedState;
if (prevCacheInstance !== null) {
ownCacheInstance = prevCacheInstance;
// There was no refresh, so no need to propagate to nested boundaries.
pushProvider(workInProgress, CacheContext, prevCache);
pushProvider(workInProgress, CacheContext, ownCacheInstance);
} else {
ownCache = null;
ownCacheInstance = null;
}
}
}
Expand All @@ -741,7 +747,7 @@ function updateCacheComponent(
// point to a cache object. Otherwise, a null state indicates that this
// CacheComponent inherits from a parent boundary. We can use this to infer
// whether to push/pop the cache context.
workInProgress.memoizedState = ownCache;
workInProgress.memoizedState = ownCacheInstance;

const nextChildren = workInProgress.pendingProps.children;
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
Expand Down Expand Up @@ -3337,9 +3343,10 @@ function beginWork(
}
case CacheComponent: {
if (enableCache) {
const ownCache: Cache | null = workInProgress.memoizedState;
if (ownCache !== null) {
pushProvider(workInProgress, CacheContext, ownCache);
const ownCacheInstance: CacheInstance | null =
workInProgress.memoizedState;
if (ownCacheInstance !== null) {
pushProvider(workInProgress, CacheContext, ownCacheInstance);
}
}
break;
Expand Down
45 changes: 26 additions & 19 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {
OffscreenProps,
OffscreenState,
} from './ReactFiberOffscreenComponent';
import type {Cache} from './ReactFiberCacheComponent';
import type {CacheInstance} from './ReactFiberCacheComponent';

import checkPropTypes from 'shared/checkPropTypes';

Expand Down Expand Up @@ -667,17 +667,17 @@ function updateCacheComponent(
// Read directly from the context. We don't set up a context dependency
// because the propagation function automatically includes CacheComponents in
// its search.
const parentCache: Cache | null = isPrimaryRenderer
const parentCache: CacheInstance | null = isPrimaryRenderer
? CacheContext._currentValue
: CacheContext._currentValue2;

let ownCache: Cache | null = null;
let ownCacheInstance: CacheInstance | null = null;
// TODO: Fast path if parent has a new cache. Merely an optimization. Might
// not be worth it.
if (false) {
// The parent boundary also has a new cache. We're either inside a new tree,
// or there was a refresh. In both cases, we should use the parent cache.
ownCache = null;
ownCacheInstance = null;
} else {
if (current === null) {
// This is a newly mounted component. Request a fresh cache.
Expand All @@ -692,12 +692,15 @@ function updateCacheComponent(
// spawned from a previous render that already committed. Otherwise, this
// is the root of a cache consistency boundary.
if (freshCache !== parentCache) {
ownCache = freshCache;
pushProvider(workInProgress, CacheContext, freshCache);
ownCacheInstance = {
cache: freshCache,
provider: workInProgress,
};
pushProvider(workInProgress, CacheContext, ownCacheInstance);
// No need to propagate the refresh, because this is a new tree.
} else {
// Use the parent cache
ownCache = null;
ownCacheInstance = null;
}
} else {
// This component already mounted.
Expand All @@ -711,8 +714,11 @@ function updateCacheComponent(
);
const freshCache = requestFreshCache(root, renderLanes);
if (freshCache !== parentCache) {
ownCache = freshCache;
pushProvider(workInProgress, CacheContext, freshCache);
ownCacheInstance = {
cache: freshCache,
provider: workInProgress,
};
pushProvider(workInProgress, CacheContext, ownCacheInstance);
// Refreshes propagate through the entire subtree. The refreshed cache
// will override nested caches.
propagateCacheRefresh(workInProgress, renderLanes);
Expand All @@ -721,17 +727,17 @@ function updateCacheComponent(
// unreachable in practice, because this means the parent cache was
// refreshed in the same render. So we would have already handled this
// in the earlier branch, where we check if the parent is new.
ownCache = null;
ownCacheInstance = null;
}
} else {
// Reuse the memoized cache.
const prevCache: Cache | null = current.memoizedState;
if (prevCache !== null) {
ownCache = prevCache;
const prevCacheInstance: CacheInstance | null = current.memoizedState;
if (prevCacheInstance !== null) {
ownCacheInstance = prevCacheInstance;
// There was no refresh, so no need to propagate to nested boundaries.
pushProvider(workInProgress, CacheContext, prevCache);
pushProvider(workInProgress, CacheContext, ownCacheInstance);
} else {
ownCache = null;
ownCacheInstance = null;
}
}
}
Expand All @@ -741,7 +747,7 @@ function updateCacheComponent(
// point to a cache object. Otherwise, a null state indicates that this
// CacheComponent inherits from a parent boundary. We can use this to infer
// whether to push/pop the cache context.
workInProgress.memoizedState = ownCache;
workInProgress.memoizedState = ownCacheInstance;

const nextChildren = workInProgress.pendingProps.children;
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
Expand Down Expand Up @@ -3337,9 +3343,10 @@ function beginWork(
}
case CacheComponent: {
if (enableCache) {
const ownCache: Cache | null = workInProgress.memoizedState;
if (ownCache !== null) {
pushProvider(workInProgress, CacheContext, ownCache);
const ownCacheInstance: CacheInstance | null =
workInProgress.memoizedState;
if (ownCacheInstance !== null) {
pushProvider(workInProgress, CacheContext, ownCacheInstance);
}
}
break;
Expand Down
10 changes: 6 additions & 4 deletions packages/react-reconciler/src/ReactFiberCacheComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import type {ReactContext} from 'shared/ReactTypes';

import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';

export type Cache = {|
providers: Set<Fiber> | null,
data: Map<() => mixed, mixed> | null,
export type Cache = Map<() => mixed, mixed>;

export type CacheInstance = {|
cache: Cache | null,
provider: Fiber,
|};

export const CacheContext: ReactContext<Cache | null> = {
export const CacheContext: ReactContext<CacheInstance | null> = {
$$typeof: REACT_CONTEXT_TYPE,
// We don't use Consumer/Provider for Cache components. So we'll cheat.
Consumer: (null: any),
Expand Down
26 changes: 1 addition & 25 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import {
enableFundamentalAPI,
enableSuspenseCallback,
enableScopeAPI,
enableCache,
} from 'shared/ReactFeatureFlags';
import {
FunctionComponent,
Expand Down Expand Up @@ -793,31 +792,8 @@ function commitLifeCycles(
case ScopeComponent:
case OffscreenComponent:
case LegacyHiddenComponent:
case CacheComponent:
return;
case CacheComponent: {
if (enableCache) {
if (current !== null) {
const oldCache: Cache | null = current.memoizedState;
if (oldCache !== null) {
const oldCacheProviders = oldCache.providers;
if (oldCacheProviders) {
oldCacheProviders.delete(current);
oldCacheProviders.delete(finishedWork);
}
}
}
const newCache: Cache | null = finishedWork.memoizedState;
if (newCache !== null) {
const newCacheProviders = newCache.providers;
if (newCacheProviders === null) {
newCache.providers = new Set([finishedWork]);
} else {
newCacheProviders.add(finishedWork);
}
}
}
return;
}
}
invariant(
false,
Expand Down
26 changes: 1 addition & 25 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import {
enableSuspenseCallback,
enableScopeAPI,
enableDoubleInvokingEffects,
enableCache,
} from 'shared/ReactFeatureFlags';
import {
FunctionComponent,
Expand Down Expand Up @@ -794,31 +793,8 @@ function commitLifeCycles(
case ScopeComponent:
case OffscreenComponent:
case LegacyHiddenComponent:
case CacheComponent:
return;
case CacheComponent: {
if (enableCache) {
if (current !== null) {
const oldCache: Cache | null = current.memoizedState;
if (oldCache !== null) {
const oldCacheProviders = oldCache.providers;
if (oldCacheProviders) {
oldCacheProviders.delete(current);
oldCacheProviders.delete(finishedWork);
}
}
}
const newCache: Cache | null = finishedWork.memoizedState;
if (newCache !== null) {
const newCacheProviders = newCache.providers;
if (newCacheProviders === null) {
newCache.providers = new Set([finishedWork]);
} else {
newCacheProviders.add(finishedWork);
}
}
}
return;
}
}
invariant(
false,
Expand Down
27 changes: 6 additions & 21 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type {
} from './ReactFiberSuspenseComponent.new';
import type {SuspenseContext} from './ReactFiberSuspenseContext.new';
import type {OffscreenState} from './ReactFiberOffscreenComponent';
import type {CacheInstance} from './ReactFiberCacheComponent';

import {resetWorkInProgressVersions as resetMutableSourceWorkInProgressVersions} from './ReactMutableSource.new';

Expand Down Expand Up @@ -1488,27 +1489,11 @@ function completeWork(
}
case CacheComponent: {
if (enableCache) {
// If the cache provided by this boundary has changed, schedule an
// effect to add this component to the cache's providers, and to remove
// it from the old cache.
// TODO: Schedule for Passive phase
const ownCache: Cache | null = workInProgress.memoizedState;
if (current === null) {
if (ownCache !== null) {
// This is a cache provider.
popProvider(CacheContext, workInProgress);
// Set up a refresh subscription.
workInProgress.flags |= Update;
}
} else {
if (ownCache !== null) {
// This is a cache provider.
popProvider(CacheContext, workInProgress);
}
if (ownCache !== current.memoizedState) {
// Cache changed. Create or update a refresh subscription.
workInProgress.flags |= Update;
}
const ownCacheInstance: CacheInstance | null =
workInProgress.memoizedState;
if (ownCacheInstance !== null) {
// This is a cache provider.
popProvider(CacheContext, workInProgress);
}
bubbleProperties(workInProgress);
return null;
Expand Down
Loading

0 comments on commit b13ea9e

Please sign in to comment.