Skip to content

Commit

Permalink
Comment areas of concern for connect + uSES
Browse files Browse the repository at this point in the history
  • Loading branch information
markerikson committed Dec 22, 2021
1 parent bc4569f commit 63f626b
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
14 changes: 13 additions & 1 deletion src/components/connect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions src/utils/useIsomorphicLayoutEffect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 63f626b

Please sign in to comment.