Possible optimization to useSyncExternalStore's withSelector

[dutziworks]

react/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js

Lines 70 to 91 in dd2d652

	const prevSnapshot: Snapshot = (memoizedSnapshot: any);
	const prevSelection: Selection = (memoizedSelection: any);
	if (is(prevSnapshot, nextSnapshot)) {
	// The snapshot is the same as last time. Reuse the previous selection.
	return prevSelection;
	}
	// The snapshot has changed, so we need to compute a new selection.
	const nextSelection = selector(nextSnapshot);
	// If a custom isEqual function is provided, use that to check if the data
	// has changed. If it hasn't, return the previous selection. That signals
	// to React that the selections are conceptually equal, and we can bail
	// out of rendering.
	if (isEqual !== undefined && isEqual(prevSelection, nextSelection)) {
	return prevSelection;
	}
	memoizedSnapshot = nextSnapshot;
	memoizedSelection = nextSelection;
	return nextSelection;

Hey, isn't line 89 in block above supposed to be right before line 78?

I mean, if prevSnapshot and nextSnapshot are not equal, but prevSelection and nextSelection are equal, shouldn't we at least update memoizedSnapshot to be equal to nextSnapshot?

That way, in the next run of this callback, we at least don't run selector and isEqual again.

I may be missing something here, but I did make that change in our, pretty hefty, code base and all the tests passed and I got a nice performance boost.

const prevSnapshot: Snapshot = (memoizedSnapshot: any);
const prevSelection: Selection = (memoizedSelection: any);

if (is(prevSnapshot, nextSnapshot)) {
  // The snapshot is the same as last time. Reuse the previous selection.
  return prevSelection;
}

memoizedSnapshot = nextSnapshot;

// The snapshot has changed, so we need to compute a new selection.
const nextSelection = selector(nextSnapshot);

// If a custom isEqual function is provided, use that to check if the data
// has changed. If it hasn't, return the previous selection. That signals
// to React that the selections are conceptually equal, and we can bail
// out of rendering.
if (isEqual !== undefined && isEqual(prevSelection, nextSelection)) {
  return prevSelection;
}

memoizedSelection = nextSelection;
return nextSelection;

[karankulshrestha]

I will fix it please give me

[karankulshrestha]

please assign me

[jellevoost]

Next to the optimization mentioned, there is another issue this resolves: reduxjs/react-redux#1981
If you use a shared store that changes often, with selectors that always return an equal result, it will keep copies of that old store alive as they are never updated.

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[markerikson]

Still worth looking at.