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

@react-aria: forwardRef support #834

Closed
jlarmstrongiv opened this issue Jul 24, 2020 · 29 comments Β· Fixed by #2293 or #2458
Closed

@react-aria: forwardRef support #834

jlarmstrongiv opened this issue Jul 24, 2020 · 29 comments Β· Fixed by #2293 or #2458
Assignees
Labels
enhancement New feature or request

Comments

@jlarmstrongiv
Copy link

jlarmstrongiv commented Jul 24, 2020

πŸ™‹β€β™‚οΈ Feature Request

Support for React.forwardRef on custom form elements to accommodate:

πŸ€” Expected Behavior

  • Having an optional ref on my custom form component.

😯 Current Behavior

  • I can use React.forwardRef, but then I would always need to pass in a ref
  • I can go without refs, forgoing their benefits

πŸ’β€β™‚οΈ Possible Solution / πŸ’» Examples

Implement a utility to share / combine refs. Several similar implementations already exist:

πŸ”¦ Context

I would like to build robust and accessible custom form components that support both controlled and uncontrolled component patterns.

@devongovett
Copy link
Member

One pattern we've used is something like this:

const Button = React.forwardRef((props, ref) => {
  let fallbackRef = React.useRef();
  let domRef = ref || fallbackRef;

  return (
    <button ref={domRef}>button</button>
  );
});

This uses the passed ref if available, but falls back to a ref created by useRef. Is that what you were thinking? Seems like a nice util to have potentially.

@devongovett devongovett added the enhancement New feature or request label Jul 24, 2020
@jlarmstrongiv
Copy link
Author

Yes, that’s exactly what I was thinking! Thank you for the example πŸ‘ either a utility hook or a mention in the docs would be a nice addition

@devongovett
Copy link
Member

Sounds good! We could add a useOptionalRef hook to the @react-aria/utils package.

@devongovett devongovett added the good first issue Good for newcomers label Jul 25, 2020
@Andarist
Copy link
Contributor

Just keep in mind that you might often need access to the ref.current, but the provided ref can be a function - so instead of a simple ref || fallbackRef; you might need to implement a more complex solution of composing refs. I see that @jlarmstrongiv has already linked to such solutions - so you can take inspiration there. None of them behaves like regular refs though (the update timing is different, refs are not cleared with null etc) - I believe that the hook I've created in the past for my own usage gets pretty close to the regular semantics of refs: https://github.com/Andarist/use-composed-ref/blob/1cafdd6123d7e7aa4f3c8e6a74950c761524329a/src/index.ts#L20 .

@jherr
Copy link

jherr commented Sep 2, 2020

@devongovett I'm not sure how to use your advice here on making a new version of, something like TextField with React.forwardRef. What React-Hook-Form is looking for is a ref for the underlying input. Other component toolkits have something like inputRef or innerRef as a property to handle this case. Is there something like that in Spectrum that I'm just not finding? I've scoured all the type definitions and the code.

@Andarist
Copy link
Contributor

Andarist commented Sep 3, 2020

Other component toolkits have something like inputRef or innerRef as a property to handle this case

Those props only exist because in the past there was no React.forwardRef so a different prop name than ref had to be used to account for this issue.

The thing here gets complicated because TextFieldBase is using useImperativeHandle. Ideally TextFieldBase would use React.forwardRef as well and reuse the received ref when possible, but not sure how to best handle this here - given the API choice to expose extra imperative methods on the external ref rather than exposing the raw DOM node.

@jherr
Copy link

jherr commented Sep 3, 2020

@Andarist Thanks for the reply. I'm down with using ref but that doesn't work in this case either. If it should work then I'd be happy to try to put together a PR that would make it work. I'm just not sure what the intention is.

@devongovett
Copy link
Member

It was an intentional choice to not expose the raw DOM node from our refs, but wrap in an API we control. This allows us to swap out the underlying implementation when needed, e.g. switch element types. Also allows us to reuse the same API across multiple platforms, e.g. native, where the internal elements are different. Sometimes it's also unclear where the ref prop that's forwarded should go – does it go on the outer most element or on something inside?

There is a ref.current.UNSAFE_getDOMNode() method to get the outer-most element of any component in React Spectrum. This is unsafe because we don't guarantee stability of the element type that is returned. For textfield, there's also ref.current.getInputElement(), which returns the <input> element specifically.

If you need a ref object to pass the native input to something else, you could do something like this:

let textfieldRef = useRef();
let inputRef = {
  get current() {
    return textFieldRef.current.getInputElement();
  }
};

// Pass inputRef to whatever you like...

<TextField ref={textFieldRef} />

Perhaps there could be a util function to do this...

@jherr
Copy link

jherr commented Sep 4, 2020

Gotcha. Thanks! Turns out that at least for react-hook-form you can us a Controller and manage it without a ref.

@bogdansoare
Copy link

Also having trouble with React's forwardRef and useTextField from @react-aria. My use case is the same, using react-hook-form. I think an utility should be baked in @react-aria since this is pretty common usage

@bluebill1049
Copy link

bluebill1049 commented Nov 6, 2020

One of the benefits of exposing ref to third-party lib like react-hook-form is so we can focus on that input when an error occurred to improve accessibility. looking forward to the potential update.

Screen Recording 2020-10-31 at 12 31 04 pm

@devongovett
Copy link
Member

The ref exposed by the TextField component does have a focus() method already...

@reminjp
Copy link

reminjp commented Dec 31, 2020

I'm having trouble with the type error that the ref from React.forwardRef is React.Ref but @react-aria hooks accept only React.RefObject. Is there any recommended solution? Should we import @react-spectrum to use useDOMRef? Does anyone plan to add a hook like useDOMRef or useComposedRef (mentioned in #834 (comment)) to @react-aria?

@skywinston
Copy link

Similar situation: I'm attempting to forward a ref to a component using the useButton hook, and it does not like the ref:

import React from "react";
import { useButton } from "@react-aria/button";

const Button = React.forwardRef((props, forwardedRef) => {
  let internalRef = React.useRef();
  let ref = forwardedRef || internalRef;

  let { buttonProps } = useButton(props, ref);

  return (
    <button {...buttonProps} ref={ref}>
      {props.children}
    </button>
  );
});

export default Button;

The ref in the useButton hook displays this error:

No overload matches this call.
  The last overload gave the following error.
    Argument of type '((instance: unknown) => void) | MutableRefObject<unknown>' is not assignable to parameter of type 'RefObject<HTMLElement>'.
      Property 'current' is missing in type '(instance: unknown) => void' but required in type 'RefObject<HTMLElement>'.ts(2769)

Codesandbox here

@chrishoage
Copy link
Contributor

chrishoage commented Apr 1, 2021

We are trying to build a component library using react-aria and are running into trouble due to the types from TypeScript.

Being unable to wrap this hook inside forwardRef is becoming quite a challenge.

Like skywinston the "workaround" doesn't satisfy the types for useButton.

Is the React.forwardRef pattern supported by react-aria? If so is there an accepted way to achieve this behavior?

Edit: I was able to make this "work" by asserting that the ref is a mutable object ref

function assertOnlyObjectRef<RefType>(
  ref: ForwardedRef<RefType>
): asserts ref is MutableRefObject<RefType | null> | null {
  if (typeof ref === 'function') {
    throw new Error('This component may only be used with an object ref');
  }
}

export const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
  (
    { children, size = Size.M, style = Style.Default, ...buttonHookProps },
    forwardRef
  ) => {
    assertOnlyObjectRef(forwardRef);

    const internalRef = useRef<HTMLButtonElement>(null);
    const ref = forwardRef ?? internalRef;

    const { buttonProps } = useButton(buttonHookProps, ref);

    return (
      <button
        className={clsx(styles.button, styles[size], styles[style])}
        ref={ref}
        {...buttonProps}
      >
          {children}
      </button>
    );
  }
);

Is this the expected way to use useButton inside forwardRef with TypeScript?

@snowystinger
Copy link
Member

I'm curious, we use it just fine in our own component without that assertion
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/button/src/Button.tsx

I'm no typescript expert though, really just a user, so maybe we've made some error in our own?

@allforabit
Copy link

@snowystinger I'd say it's due to strict mode being enabled in the tsconfig.json

@chrishoage
Copy link
Contributor

I think it could also be due to this hook

export function useFocusableRef<T extends HTMLElement = HTMLElement>(ref: FocusableRef<T>, focusableRef?: RefObject<HTMLElement>): RefObject<T> {

It returns a mutable object ref from useRef and the ref from forwardRef is consumed by useImperativeHandle

The other issue I see is that Button.tsx casts React.forwardRef(Button) as a function that doesn't match the signature of forwardRef

Basically that Button.tsx is instructing typescript that it has a different signature that React.forwardRef actually defined

The crux of the issue useButton has types which can't accept a ref function which the type signature of React.forwardRef allows for.

My assertion narrows the type of the forward ref to only be a RefObject which satisfies the types of useButton

I imagine if the casting was removed from the Button.tsx example you shared it would expose a similar type error as described by myself and skywinston

@chrishoage
Copy link
Contributor

@snowystinger I have modified Button.tsx to be a more typical / common usage of forwardRef and it exposes the same problem

image

I will be clear I understand that the casting is to support a generic button ElemenType which otherwise can't be supported with React.forwardRef however the type signature that it is cast to is not honest to the actual types that React.forwardRef will return in normal usage of React.

Passing a ref function is a normal and supported way of retereving a ref from a component, one which useButton (and I presume a lot of other hooks in the react-aria codebase) don't support.

I hope this diff illustates the issue clearly in the react-spectum codebase.

Please let me know if you have further questions or there is anything that I may clearify!

diff --git a/packages/@react-spectrum/button/src/Button.tsx b/packages/@react-spectrum/button/src/Button.tsx
index 6dea9cde..ed2f3ff3 100644
--- a/packages/@react-spectrum/button/src/Button.tsx
+++ b/packages/@react-spectrum/button/src/Button.tsx
@@ -10,24 +10,30 @@
  * governing permissions and limitations under the License.
  */
 
-import {classNames, SlotProvider, useFocusableRef, useSlotProps, useStyleProps} from '@react-spectrum/utils';
-import {FocusableRef} from '@react-types/shared';
-import {FocusRing} from '@react-aria/focus';
-import {mergeProps} from '@react-aria/utils';
-import React, {ElementType, ReactElement} from 'react';
-import {SpectrumButtonProps} from '@react-types/button';
+import { classNames, SlotProvider, useFocusableRef, useSlotProps, useStyleProps } from '@react-spectrum/utils';
+import { FocusableRef } from '@react-types/shared';
+import { FocusRing } from '@react-aria/focus';
+import { mergeProps } from '@react-aria/utils';
+import React, { ElementType, ReactElement } from 'react';
+import { SpectrumButtonProps } from '@react-types/button';
 import styles from '@adobe/spectrum-css-temp/components/button/vars.css';
-import {Text} from '@react-spectrum/text';
-import {useButton} from '@react-aria/button';
-import {useHover} from '@react-aria/interactions';
-import {useProviderProps} from '@react-spectrum/provider';
+import { Text } from '@react-spectrum/text';
+import { useButton } from '@react-aria/button';
+import { useHover } from '@react-aria/interactions';
+import { useProviderProps } from '@react-spectrum/provider';
 
 // todo: CSS hasn't caught up yet, map
 let VARIANT_MAPPING = {
   negative: 'warning'
 };
 
-function Button<T extends ElementType = 'button'>(props: SpectrumButtonProps<T>, ref: FocusableRef<HTMLElement>) {
+
+/**
+ * Buttons allow users to perform an action or to navigate to another page.
+ * They have multiple styles for various needs, and are ideal for calling attention to
+ * where a user needs to do something in order to move forward in a flow.
+ */
+let _Button = React.forwardRef<HTMLElement, SpectrumButtonProps<'button'>>((props, ref) => {
   props = useProviderProps(props);
   props = useSlotProps(props, 'button');
   let {
@@ -39,10 +45,10 @@ function Button<T extends ElementType = 'button'>(props: SpectrumButtonProps<T>,
     autoFocus,
     ...otherProps
   } = props;
-  let domRef = useFocusableRef(ref);
-  let {buttonProps, isPressed} = useButton(props, domRef);
-  let {hoverProps, isHovered} = useHover({isDisabled});
-  let {styleProps} = useStyleProps(otherProps);
+  // let domRef = useFocusableRef(ref);
+  let { buttonProps, isPressed } = useButton(props, ref);
+  let { hoverProps, isHovered } = useHover({ isDisabled });
+  let { styleProps } = useStyleProps(otherProps);
 
   let buttonVariant = variant;
   if (VARIANT_MAPPING[variant]) {
@@ -86,12 +92,5 @@ function Button<T extends ElementType = 'button'>(props: SpectrumButtonProps<T>,
       </ElementType>
     </FocusRing>
   );
-}
-
-/**
- * Buttons allow users to perform an action or to navigate to another page.
- * They have multiple styles for various needs, and are ideal for calling attention to
- * where a user needs to do something in order to move forward in a flow.
- */
-let _Button = React.forwardRef(Button) as <T extends ElementType = 'button'>(props: SpectrumButtonProps<T> & {ref?: FocusableRef<HTMLElement>}) => ReactElement;
-export {_Button as Button};
+});
+export { _Button as Button };

@Newbie012
Copy link

What's the status of this issue? not to be rude, but it gets impossible to integrate @react-aria with a strictly typed app 😟

@Newbie012
Copy link

I managed to find a solution via useImperativeHandle:

const Checkbox = React.forwardRef(
  (props: PropsWithChildren<CheckboxProps>, ref: React.ForwardedRef<HTMLInputElement>) => {
    const inputRef = React.useRef<HTMLInputElement>(null);

    const { inputProps } = useCheckbox(props, state, inputRef);

    React.useImperativeHandle(ref, getCurrentRef(inputRef));

    return (
      <CheckboxWrapper isValid={isValid} isDisabled={isDisabled}>
        <VisuallyHidden>
          <input {...inputProps} {...focusProps} ref={inputRef} />
        </VisuallyHidden>
        ...
      </CheckboxWrapper>
    );
  }
);

export function getCurrentRef<T>(ref: React.RefObject<T>) {
  return () => {
    if (ref.current === null) {
      throw new Error("getCurrentRef should only be used when ref is certainly defined");
    }

    return ref.current;
  };
}

@LFDanLu LFDanLu removed the good first issue Good for newcomers label Aug 25, 2021
@solimant solimant mentioned this issue Sep 7, 2021
5 tasks
@LFDanLu
Copy link
Member

LFDanLu commented Oct 27, 2021

Handled in #2293, should go out in the next release

@LFDanLu LFDanLu closed this as completed Oct 27, 2021
This was linked to pull requests Oct 27, 2021
@devongovett
Copy link
Member

Did we do this yet? #2293 (comment)

@LFDanLu
Copy link
Member

LFDanLu commented Oct 27, 2021

Ah my bad, reopening. For others in the thread, useObjectRef will be available in the next release in react-aria/utils but we are planning on calling that util hook to our aria hooks so that you won't need to do so yourselves.

@LFDanLu LFDanLu reopened this Oct 27, 2021
@solimant
Copy link
Collaborator

@devongovett, @LFDanLu - I've created #2503 to track the ask in #2293 (comment). I think we can close this since we have a way to address the original ask, albeit not ideal.

@jlarmstrongiv
Copy link
Author

  • To confirm, is useObjectRef is available in the latest release?
  • Was this issue addressed?
  • So an example of current usage would be something like this?
    const TextField = React.forwardRef((props, forwardedRef) => {
      const ref = useObjectRef(forwardedRef);

      return <input {...props} ref={ref} />;
    });

@bstro
Copy link

bstro commented Nov 6, 2021

@jlarmstrongiv I can't speak to the question in your second bullet point, but I don't think it has been released yet. I've been copying the file at this commit and pasting it into my projects until it is published properly. Not ideal, but I don't mind using it as a stop-gap until the next release of react-aria. Your usage example is how I've been using it.

@LFDanLu
Copy link
Member

LFDanLu commented Nov 8, 2021

@jlarmstrongiv It is not currently available in the current release but it will be with the next release (3.15). For now you can access it via a the @react-aria/util nightlies. The usage looks good to me, I'll have @solimant answer the second bullet point.

@devongovett
Copy link
Member

Followup task tracked in #2503. Closing this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet