fix(react): support React refs (object and callback) #23152

TuckerWhitehouse · 2021-04-06T01:14:30Z

React refs can be an object, or a callback - callbacks are
commonly used by 3rd party libraries for combining multiple refs.
https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: #23153

Providing a callback ref breaks internal assumptions that forwardedRefs will always be in the object format.

What is the new behavior?

Internal ref usage has been decoupled from forwardedRefs.
Refs may be provided in object or callback format.

Does this introduce a breaking change?

Yes
No

Other information

liamdebeasi

Great job! PR will be approved once the changes noted below are made.

Here is a dev build if you would like to test this in your app:

npm install @ionic/react@5.7.0-dev.202104151952.ec15e01 @ionic/react-router@5.7.0-dev.202104151952.ec15e01

liamdebeasi · 2021-04-15T19:47:23Z

packages/react/src/components/createControllerComponent.tsx

@@ -107,7 +111,11 @@ export const createControllerComponent = <
      // It's also possible for the component to have become unmounted.
      if (this.props.isOpen === true && this.isUnmounted === false) {
        if (this.props.forwardedRef) {
-          (this.props.forwardedRef as any).current = this.overlay;
+          if (typeof this.props.forwardedRef === 'function') {


Can we extract this to a shared function in the utils file? Something like:

const setRef = (ref, value = null) => { if (typeof ref === 'function') { ref(value); } else { ref.current = value } };

That way we can use this in both createControllerComponent.tsx and createOverlayComponent.tsx.

liamdebeasi · 2021-04-15T19:48:12Z

packages/react/src/components/utils/index.tsx

+        ref(value)
+      } else if (ref != null) {
+        // Cast as a MutableRef so we can assign current
+        (ref as React.MutableRefObject<any>).current = value


We can probably use the helper function here as well (we may need to cast as MutableRef in the helper function)

liamdebeasi · 2021-04-15T19:49:46Z

packages/react/src/components/utils/index.tsx

+  ...refs: (React.ForwardedRef<any> | React.Ref<any> | undefined)[]
+) => {
+  return (value: any) => {
+    refs.forEach(ref => {


Do we need to return anything here?

I don't believe react has any requirements or expectations for the return value of a callback ref, but I will double-check and confirm.

Confirmed the docs referenced above show an example with no return value, and the typescript types show the ForwardedRef function format and RefCallback should return void.

TuckerWhitehouse · 2021-04-15T20:10:58Z

Hey @liamdebeasi - I believe all feedback has been addressed in the updated commit, please let me know if there are any other required changes -- thanks for reviewing this!

liamdebeasi

I tested an updated dev build (5.7.0-dev.202104161346.589a1b8) and it looks like the onIonChange handler is being fired twice now. I am testing this with the CodeSandbox found in #23153.

If I remove the ref or use a non-callback ref then the onIonChange handler only fires once. My guess is maybe it has something to do with the mergeRefs function.

Steps to reproduce:

Open CodeSandbox on issue I linked to with the dev build installed.
Type a. Observe that the console logs onChange a.
Type b. Observe that the console logs onChange ab twice.

liamdebeasi · 2021-04-16T13:54:37Z

packages/react/src/components/__tests__/createComponent.spec.tsx

    const ionButtonRef: React.RefObject<any> = React.createRef();
    const IonButton = createReactComponent<JSX.IonButton, HTMLIonButtonElement>('ion-button');

    const { getByText } = render(<IonButton ref={ionButtonRef}>ButtonNameA</IonButton>);
    const ionButtonItem = getByText('ButtonNameA');
    expect(ionButtonRef.current).toEqual(ionButtonItem);
  });
+
+  test('should pass ref on to web component instance (RefCallback)', () => {


We should add a test here to test that onIonChange is called at most once at a time.

Hey @liamdebeasi - I'm unsure how to go about introducing this test. I looked throughout the project and couldn't find any examples where the web components were being rendered and their behaviors verified. If you have any pointers on how to get everything set up, I'd be more than happy to add this!

I haven't actually tested this, but I was thinking something like the following:

let current; const ionInputRef: React.RefCallback<any> = value => current = value; const changeCallback = jest.fn(); const IonInput = createReactComponent<JSX.IonInput, HTMLIonInputElement>('ion-input'); const { getBy } = render(<IonInput id="my-input" onIonChange={changeCallback} ref={ionInputRef}></IonInput>); const ionInputItem = getBy('#my-input'); ionInputItem.value = 'abc'; expect(changeCallback).toBeCalledTimes(1); ionInputItem.value = 'abcxyz'; expect(changeCallback).toBeCalledTimes(2);

Directly assigning to .value doesn't seem to trigger the onIonChange -- From what I could tell, this setup doesn't actually render the ion-input WebComponent.

I tried to use fireEvent.change(ionInput, { target: { value: '' } } but that throws "The given element does not have a value setter".

🤔 Weird. Let's hold off on this. I posted the suggestion before I saw #23152 (comment), so I'll do some more digging and see if we even need to add this.

TuckerWhitehouse · 2021-04-16T16:06:33Z

I've pushed another code change to ensure the refs remain stable between renders (I don't believe this is the cause of the issue described above, but it was another issue I found while debugging).

That being said, I'm not sure the double onIonChange is actually caused by these changes (it appears to happen with both dev builds as well). I tried to recreate an example without react-hook-form and was unable to, which makes me lean towards this being a bug in that library.

--

For reference, I tried various combinations of object refs, react refs, inline functions, inline functions that set refs, inline functions that returned refs, etc, and was unable to produce the double event behavior seen with the react-hook-form ref.

liamdebeasi · 2021-04-16T16:12:41Z

Hmm ok I will take another look. If the issue is not in Ionic React then we probably do not need to add the other test I mentioned.

liamdebeasi

Ok so looks like if you type "a", the react-hook-form package sets that as the input's default value. Then when you type "ab", it sets the value to the default value and then sets it to "ab" for some reason. The actual Ionic event still shows "ab" as the value, but I agree with your original suggestion that this looks like a bug in the react-hook-form.

Code looks good to go. Thanks for putting up this PR!

liamdebeasi

One more question -- do we need to make these React.ForwardedRef as well?

IonTabButton:

ionic-framework/packages/react/src/components/navigation/IonTabButton.tsx

Line 11 in 8e0e5da

ref?: React.RefObject<HTMLIonTabButtonElement>;

IonBackButton:

ionic-framework/packages/react/src/components/navigation/IonBackButton.tsx

Line 16 in 8e0e5da

ref?: React.RefObject<HTMLIonBackButtonElement>;

TuckerWhitehouse · 2021-04-21T14:37:00Z

Hey @liamdebeasi -- I updated the two components to use React.Ref instead of React.RefObject.
From what I can tell, neither of these components are currently setup to forward the ref to the inner component, so the ref is just going to point to the instance of the class.

I could probably update them to use the createForwardRef but that is a change in behavior for refs on those elements and I'm not sure if it makes sense to introduce what could be a breaking change with the other (non-breaking) support for callback refs.

Edit: Found one more in IonRouterOutlet.

React refs can be an object, or a callback - callbacks are commonly used by 3rd party libraries for combining multiple refs. https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

liamdebeasi · 2021-04-21T14:50:59Z

That all makes sense. I agree we should not introduce a breaking change with this. Thanks!

liamdebeasi · 2021-04-22T13:56:21Z

Merged. Thank you very much for the PR!

TuckerWhitehouse · 2021-04-22T17:50:39Z

Hey @liamdebeasi - Thanks for reviewing and helping to get this merged, looking forward to this being included in an upcoming release :D (now I just need to sort out that react-hook-form bug... haha)

liamdebeasi · 2021-04-22T17:53:26Z

We just shipped v5.6.5 a few hours ago that includes this fix 😄

liamdebeasi · 2021-05-11T14:18:53Z

@TuckerWhitehouse Hey there, this seems to have caused a regression in #23287. When an Ionic component re-renders, the ref value becomes null. I am working to determine how to fix, but wanted to check in and see if you had any ideas.

TuckerWhitehouse · 2021-05-11T14:23:23Z

@liamdebeasi I believe the code referenced in that ticket is using createRef where they should be using useRef -- The createRef is going to create a new ref object on each render and React will set the ref to null whenever the value passed in as a ref changes (referential integrity and all).
https://stackoverflow.com/questions/54620698/whats-the-difference-between-useref-and-createref

liamdebeasi · 2021-05-11T14:29:01Z

Ah good catch! In this case, should we be calling that mergeRefs function in componentDidUpdate rather than once in the constructor?

liamdebeasi · 2021-05-11T14:30:42Z

Oh hmm... I guess you would use createRef in a class component an useRef in a functional component, right? I am less familiar with createRef.

TuckerWhitehouse · 2021-05-11T14:36:20Z

Yup, createRef is the util for classes, and useRef is the util for functions -- Looks like switching to useRef resolved the issue in #23287 :)

liamdebeasi · 2021-05-11T14:48:32Z

Thanks for the help!

github-actions bot added the package: react label Apr 6, 2021

coreyjv mentioned this pull request Apr 6, 2021

bug: react, callbacks refs not supported #23153

Closed

liamdebeasi suggested changes Apr 15, 2021

View reviewed changes

liamdebeasi suggested changes Apr 16, 2021

View reviewed changes

liamdebeasi approved these changes Apr 20, 2021

View reviewed changes

liamdebeasi reviewed Apr 20, 2021

View reviewed changes

fix(react): support React refs (object and callback)

099101d

React refs can be an object, or a callback - callbacks are commonly used by 3rd party libraries for combining multiple refs. https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

liamdebeasi merged commit 0dd189e into ionic-team:master Apr 22, 2021

TuckerWhitehouse deleted the react-ref-callback branch April 22, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(react): support React refs (object and callback) #23152

fix(react): support React refs (object and callback) #23152

TuckerWhitehouse commented Apr 6, 2021 •

edited by liamdebeasi

Loading

liamdebeasi left a comment

liamdebeasi Apr 15, 2021

liamdebeasi Apr 15, 2021

liamdebeasi Apr 15, 2021

TuckerWhitehouse Apr 15, 2021

TuckerWhitehouse Apr 15, 2021

TuckerWhitehouse commented Apr 15, 2021

liamdebeasi left a comment

liamdebeasi Apr 16, 2021

TuckerWhitehouse Apr 16, 2021

liamdebeasi Apr 16, 2021

TuckerWhitehouse Apr 16, 2021

liamdebeasi Apr 16, 2021 •

edited

Loading

TuckerWhitehouse commented Apr 16, 2021

liamdebeasi commented Apr 16, 2021

liamdebeasi left a comment

liamdebeasi left a comment

TuckerWhitehouse commented Apr 21, 2021 •

edited

Loading

liamdebeasi commented Apr 21, 2021

liamdebeasi commented Apr 22, 2021

TuckerWhitehouse commented Apr 22, 2021

liamdebeasi commented Apr 22, 2021

liamdebeasi commented May 11, 2021

TuckerWhitehouse commented May 11, 2021 •

edited

Loading

liamdebeasi commented May 11, 2021

liamdebeasi commented May 11, 2021

TuckerWhitehouse commented May 11, 2021

liamdebeasi commented May 11, 2021

fix(react): support React refs (object and callback) #23152

fix(react): support React refs (object and callback) #23152

Conversation

TuckerWhitehouse commented Apr 6, 2021 • edited by liamdebeasi Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

liamdebeasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TuckerWhitehouse commented Apr 15, 2021

liamdebeasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

TuckerWhitehouse commented Apr 16, 2021

liamdebeasi commented Apr 16, 2021

liamdebeasi left a comment

Choose a reason for hiding this comment

liamdebeasi left a comment

Choose a reason for hiding this comment

TuckerWhitehouse commented Apr 21, 2021 • edited Loading

liamdebeasi commented Apr 21, 2021

liamdebeasi commented Apr 22, 2021

TuckerWhitehouse commented Apr 22, 2021

liamdebeasi commented Apr 22, 2021

liamdebeasi commented May 11, 2021

TuckerWhitehouse commented May 11, 2021 • edited Loading

liamdebeasi commented May 11, 2021

liamdebeasi commented May 11, 2021

TuckerWhitehouse commented May 11, 2021

liamdebeasi commented May 11, 2021

TuckerWhitehouse commented Apr 6, 2021 •

edited by liamdebeasi

Loading

liamdebeasi Apr 16, 2021 •

edited

Loading

TuckerWhitehouse commented Apr 21, 2021 •

edited

Loading

TuckerWhitehouse commented May 11, 2021 •

edited

Loading