microsoft / reactxp

Library for cross-platform app development.
https://microsoft.github.io/reactxp/
Other
8.29k stars 492 forks source link

Using mutable ref in Button rises type error #1259

Open dagatsoin opened 3 years ago

dagatsoin commented 3 years ago

Current behavior:

Passing a MutableRef to a Button rises an error.

const MyComp = function(props: ButtonProps) {
  const ref= React.useRef<Button>()
  return <Button ref={ref}/>
}

Error received

ERROR in [at-loader] ./stories/button/story.common.tsx:8:18 
    TS2769: No overload matches this call.
  Overload 1 of 2, '(props: ButtonProps | Readonly<ButtonProps>): Button', gave the following error.
    Type 'MutableRefObject<Button | undefined>' is not assignable to type '(string & ((instance: Button | null) => void)) | (string & RefObject<Button>) | (((instance: Button | null) => void) & ((instance: Button | null) => void)) | ... 4 more ... | undefined'.
      Type 'MutableRefObject<Button | undefined>' is not assignable to type 'React.RefObject<Button> & RefObject<Button>'.
        Type 'MutableRefObject<Button | undefined>' is not assignable to type 'RefObject<Button>'.
          Types of property 'current' are incompatible.
            Type 'Button | undefined' is not assignable to type 'Button | null'.
              Type 'undefined' is not assignable to type 'Button | null'.
  Overload 2 of 2, '(props: ButtonProps, context: any): Button', gave the following error.
    Type 'MutableRefObject<Button | undefined>' is not assignable to type '(string & ((instance: Button | null) => void)) | (string & RefObject<Button>) | (((instance: Button | null) => void) & ((instance: Button | null) => void)) | ... 4 more ... | undefined'.
      Type 'MutableRefObject<Button | undefined>' is not assignable to type 'React.RefObject<Button> & RefObject<Button>'.
        Type 'MutableRefObject<Button | undefined>' is not assignable to type 'RefObject<Button>'.

ERROR in [at-loader] ./stories/button/story.common.tsx:235:15 
    TS2322: Type '{ root: Types.StyleRuleSet<Types.ViewStyle>[]; content: { borderRadius: number; }; label: { color: string; }; icon: { color: string; }; }' is not assignable to type 'StyleRuleSetRecursive<StyleRuleSet<ButtonStyle>>'.
  Object literal may only specify known properties, and 'root' does not exist in type 'ButtonStyle | StyleRuleSetRecursiveArray<StyleRuleSet<ButtonStyle>>'.

Impact

1- Can't use the React.useRef hook 2- Can't use React.forwardRef

Workaround

Wrap the component and use a callback to transform the mutable ref in a callback.

const Button = React.forwardRef(function(props: Types.ButtonProps, ref) {
  const setRef = typeof ref === "object"
    ? React.useCallback(function(nativeRef: RXButton) {
      if (ref) {
        ref.current = nativeRef
      }
    }, [])
    : ref

  return <RXButton {...props} ref={setRef} />
})
fbartho commented 3 years ago

@dagatsoin -- I may be wrong here, but I believe the last really significant release of ReactXP was before React's MutableRef/ForwardRef APIs were super common place.

I've been unexpectedly swamped with unrelated stuff since September, but we have been hoping to get a turn of the crank on things.

Upgrading ReactXP to support modern Refs seems like a necessary upgrade! -- I'm putting it on the High Priority list, but I don't have a big sense of how much effort this change will require!

Any thoughts?

dagatsoin commented 3 years ago

It could be easy if it is due to an old code base. I got a quick look and I don't see any code which handle the Ref attribute. But here, the union type misses MutableObject. I added it and typescript stops to throw. I will test with a real code and see if it breaks at runtime. Meantime it would be useful if @erictraut remembers any code related to the Ref handling. @erictraut

fbartho commented 3 years ago

Nice catch! -- In this thread ( https://github.com/microsoft/reactxp/issues/1128#issuecomment-662658713 ) we agreed that the next major release for ReactXP should upgrade to React 16.3 or later!

Based on the comment by your line, maybe we should remove the private replication of the Ref API and instead just expose the one from React?

erictraut commented 3 years ago

As you speculated, the current ReactXP implementation predates the introduction of MutableObject. I think it should be relatively straightforward to add support for it. The core ReactXP implementation uses the more traditional ref callbacks in places, but TMK that mechanism is still supported by newer versions of React, so there's no need to replace them.