undux icon indicating copy to clipboard operation
undux copied to clipboard

Should Undux forward refs to wrapped components?

Open wincent opened this issue 7 years ago • 4 comments

I think it would be handy if Undux used React.forwardRef() to make sure that any refs attached to components created using withStore got attached to the wrapped component instead of the wrapper itself. Thoughts?

wincent avatar Dec 19 '18 22:12 wincent

This is a really great idea. Want to submit a PR?

bcherny avatar Dec 19 '18 23:12 bcherny

I'm not super familiar with TypeScript so it would probably be a pretty clowny PR, but I think the core of it would be something like (untested):

diff --git a/src/react/connectAs.tsx b/src/react/connectAs.tsx
index 682508d..3f152c1 100644
--- a/src/react/connectAs.tsx
+++ b/src/react/connectAs.tsx
@@ -24,7 +24,7 @@ export function connectAs<
       subscriptions: Subscription[]
     }
 
-    return class extends React.Component<Diff<Props, Stores>, State> {
+    class WithStore extends React.Component<Diff<Props, Stores>, State> {
       static displayName = `withStore(${getDisplayName(Component)})`
       state = {
         stores: mapValues(stores, _ =>
@@ -49,8 +49,15 @@ export function connectAs<
           || Object.keys(props).some(_ => (props as any)[_] !== (this.props as any)[_])
       }
       render() {
-        return <Component {...this.props} {...this.state.stores} />
+        const { _forwardedRef, ...rest } = this.props
+        return (
+          <Component ref={_forwardedRef} {...rest} {...this.state.stores} />
+        )
       }
     }
+
+    return React.forwardRef((props, ref) => {
+      return <WithStore {...props} _forwardedRef={ref} />
+    })
   }
 }
diff --git a/src/react/createConnectedStore.tsx b/src/react/createConnectedStore.tsx
index 517046b..88bccf8 100644
--- a/src/react/createConnectedStore.tsx
+++ b/src/react/createConnectedStore.tsx
@@ -85,12 +85,18 @@ export function createConnectedStore<State extends object>(
     Component: React.ComponentType<Props>
   ): React.ComponentType<PropsWithoutStore> {
     let displayName = getDisplayName(Component)
-    let f: React.StatelessComponent<PropsWithoutStore> = props =>
-      <Consumer displayName={displayName}>
-        {storeSnapshot => <Component store={storeSnapshot} {...props} />}
-      </Consumer>
+    let f: React.StatelessComponent<PropsWithoutStore> = props => {
+      const { _forwardedRef, ...rest } = props
+      return (
+        <Consumer displayName={displayName}>
+          {storeSnapshot => (
+            <Component ref={_forwardedRef} store={storeSnapshot} {...rest} />
+          )}
+        </Consumer>
+      )
+    }
     f.displayName = `withStore(${displayName})`
-    return f
+    return React.forwardRef((props, ref) => f({ ...props, _forwardedRef: ref }))
   }
 
   return {
diff --git a/src/react/createConnectedStoreAs.tsx b/src/react/createConnectedStoreAs.tsx
index ea02ea4..27315f6 100644
--- a/src/react/createConnectedStoreAs.tsx
+++ b/src/react/createConnectedStoreAs.tsx
@@ -105,10 +105,14 @@ export function createConnectedStoreAs<States extends {
     Component: React.ComponentType<Props>
   ): React.ComponentType<PropsWithoutStore> {
     let displayName = getDisplayName(Component)
-    let f: React.StatelessComponent<PropsWithoutStore> = props =>
-      <Consumer displayName={displayName}>
-        {stores => <Component {...stores} {...props} />}
-      </Consumer>
+    let f: React.StatelessComponent<PropsWithoutStore> = props => {
+      const { _forwardedRef, ...rest } = props
+      return (
+        <Consumer displayName={displayName}>
+          {stores => <Component ref={_forwardedRef} {...stores} {...rest} />}
+        </Consumer>
+      )
+    }
     f.displayName = `withStores(${displayName})`
     return f
   }

wincent avatar Dec 20 '18 09:12 wincent

One other thing: Flow recently made some enhancements that improve the typing of higher-order components. One of their examples includes React.forwardRef.

wincent avatar Dec 20 '18 10:12 wincent

Just saw this here. On the topic of refs...

I converted my frontend to React Hooks. Now, I'm debugging a bunch of referencing errors all due to the same bug: https://github.com/facebook/react/issues/15291#issuecomment-479524644

As a result, I created this function--

export const updateRef = (store : Store, key : string, ref : React.MutableRefObject<any>) =>
                           store.on(key).subscribe((newValue : any) => ref.current = newValue)

export default function Game(){
  // ...
  const payload = GameStore.get("payload'),
            payloadValue = useRef(payload)

  updateRef(GameStore, "payload", payloadValue)

  function joinGame(){
    // ...
    gameChannel.on( "islands_set", playerData => {
      payloadValue.current[playerData.key] = playerData
      GameStore.set("payload")(payloadValue.current)
    })
  }
}

--to keep refs in sync.

The use-case is for the callback of a function defined within a hook that reads from the store, i.e. using payload within the callback would use a captured value that would not update (even if using GameStore.get(payload) b/c then GameStore would be captured and have the same issue).

I still have plenty more of this same kind to fix, so no ask yet. Just wanted to share the use-case.

English3000 avatar Apr 16 '19 02:04 English3000