Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should Undux forward refs to wrapped components? #75

Open
wincent opened this issue Dec 19, 2018 · 4 comments
Open

Should Undux forward refs to wrapped components? #75

wincent opened this issue Dec 19, 2018 · 4 comments

Comments

@wincent
Copy link

wincent commented Dec 19, 2018

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?

@bcherny
Copy link
Owner

bcherny commented Dec 19, 2018

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

@wincent
Copy link
Author

wincent commented Dec 20, 2018

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
Copy link
Author

wincent commented Dec 20, 2018

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

@English3000
Copy link

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: facebook/react#15291 (comment)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants