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

Using mutable ref in Button rises type error #1259

Open
dagatsoin opened this issue Feb 18, 2021 · 4 comments
Open

Using mutable ref in Button rises type error #1259

dagatsoin opened this issue Feb 18, 2021 · 4 comments

Comments

@dagatsoin
Copy link
Contributor

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

fbartho commented Feb 18, 2021

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

dagatsoin commented Feb 18, 2021

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

fbartho commented Feb 18, 2021

Nice catch! -- In this thread ( #1128 (comment) ) 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?

@fbartho fbartho added this to the 2.1.0 or 3.0.0 milestone Feb 18, 2021
@erictraut
Copy link
Contributor

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.

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