From 63f626b3b0703c4b7de658dce7c319a8ed3c7ee2 Mon Sep 17 00:00:00 2001
From: Mark Erikson <mark@isquaredsoftware.com>
Date: Mon, 1 Nov 2021 22:34:23 -0400
Subject: [PATCH] Comment areas of concern for `connect` + `uSES`

---
 src/components/connect.tsx             | 14 +++++++++++++-
 src/utils/useIsomorphicLayoutEffect.ts |  8 +++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/components/connect.tsx b/src/components/connect.tsx
index f4eb8da80..fad8e6b22 100644
--- a/src/components/connect.tsx
+++ b/src/components/connect.tsx
@@ -27,7 +27,10 @@ import defaultMapStateToPropsFactories from '../connect/mapStateToProps'
 import defaultMergePropsFactories from '../connect/mergeProps'
 
 import { createSubscription, Subscription } from '../utils/Subscription'
-import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'
+import {
+  useIsomorphicLayoutEffect,
+  canUseDOM,
+} from '../utils/useIsomorphicLayoutEffect'
 import shallowEqual from '../utils/shallowEqual'
 
 import {
@@ -124,6 +127,7 @@ function subscribeUpdates(
       return
     }
 
+    // TODO We're currently calling getState ourselves here, rather than letting `uSES` do it
     const latestStoreState = store.getState()
 
     let newChildProps, error
@@ -157,6 +161,7 @@ function subscribeUpdates(
       childPropsFromStoreUpdate.current = newChildProps
       renderIsScheduled.current = true
 
+      // TODO This is hacky and not how `uSES` is meant to be used
       // Trigger the React `useSyncExternalStore` subscriber
       additionalSubscribeListener()
     }
@@ -597,6 +602,10 @@ function connect<
         ? props.store!
         : contextValue!.store
 
+      const getServerSnapshot = didStoreComeFromContext
+        ? contextValue.getServerState
+        : store.getState
+
       const childPropsSelector = useMemo(() => {
         // The child props selector needs the store reference as an input.
         // Re-create this selector whenever the store changes.
@@ -724,7 +733,10 @@ function connect<
 
       try {
         actualChildProps = useSyncExternalStore(
+          // TODO We're passing through a big wrapper that does a bunch of extra side effects besides subscribing
           subscribeForReact,
+          // TODO This is incredibly hacky. We've already processed the store update and calculated new child props,
+          // TODO and we're just passing that through so it triggers a re-render for us rather than relying on `uSES`.
           actualChildPropsSelector,
           // TODO Need a real getServerSnapshot here
           actualChildPropsSelector
diff --git a/src/utils/useIsomorphicLayoutEffect.ts b/src/utils/useIsomorphicLayoutEffect.ts
index 0e87d6e0c..d60f51c2e 100644
--- a/src/utils/useIsomorphicLayoutEffect.ts
+++ b/src/utils/useIsomorphicLayoutEffect.ts
@@ -9,9 +9,11 @@ import { useEffect, useLayoutEffect } from 'react'
 // is created synchronously, otherwise a store update may occur before the
 // subscription is created and an inconsistent state may be observed
 
-export const useIsomorphicLayoutEffect =
+// Matches logic in React's `shared/ExecutionEnvironment` file
+export const canUseDOM = !!(
   typeof window !== 'undefined' &&
   typeof window.document !== 'undefined' &&
   typeof window.document.createElement !== 'undefined'
-    ? useLayoutEffect
-    : useEffect
+)
+
+export const useIsomorphicLayoutEffect = canUseDOM ? useLayoutEffect : useEffect