floating-ui / react-popper

🍿⚛Official React library to use Popper, the positioning library
https://popper.js.org/react-popper/
MIT License
2.5k stars 226 forks source link

Create element ref with useState dont compile (typescript) #415

Closed alfnielsen closed 3 years ago

alfnielsen commented 3 years ago

Using the example on the popper page (https://popper.js.org/react-popper/v2/) I get this error:

TS2322: Type 'Dispatch<SetStateAction<null>>' is not assignable to type 'LegacyRef<HTMLButtonElement> | undefined'.   
Type 'Dispatch<SetStateAction<null>>' is not assignable to type '(instance: HTMLButtonElement | null) => void'.     
Types of parameters 'value' and 'instance' are incompatible.       
Type 'HTMLButtonElement | null' is not assignable to type 'SetStateAction<null>'.         
Type 'HTMLButtonElement' is not assignable to type 'SetStateAction<null>'.           
Type 'HTMLButtonElement' provides no match for the signature '(prevState: null): null'.  index.d.ts(139, 9): 
The expected type comes from property 'ref' which is declared here on type 'DetailedHTMLProps<ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>'

The example:

import React, { useState } from 'react';
import { usePopper } from 'react-popper';

const Example = () => {
  const [referenceElement, setReferenceElement] = useState(null);
  const [popperElement, setPopperElement] = useState(null);
  const [arrowElement, setArrowElement] = useState(null);
  const { styles, attributes } = usePopper(referenceElement, popperElement, {
    modifiers: [{ name: 'arrow', options: { element: arrowElement } }],
  });

  return (
    <>
      <button type="button" ref={setReferenceElement}>
        Reference element
      </button>

      <div ref={setPopperElement} style={styles.popper} {...attributes.popper}>
        Popper element
        <div ref={setArrowElement} style={styles.arrow} />
      </div>
    </>
  );
};

The usePopper method cannot take a React.MutableRefObject<null>, so this dont follow react's normal code and will not work with typescript (unless //@ts-ignore is added) React documentation (https://reactjs.org/docs/hooks-reference.html#useref)

I would think that the correct way should have been something like this:

    const referenceRef = useRef(null)
    const popperRef = useRef(null)
    const { styles, attributes } = usePopper(referenceRef, popperRef);

The usePopper could still take the "old" version by checking and allowing MutableRefObject instead of only allowing Element | PopperJS.VirtualElement | null as the current version does now)

I'm not a flow guy, so I cannot make a PR for fixes this. But this prevent me and other teams using TS from using the hook.

I hope that somebody will look into this, so we don't need different versions of usePopper hook

FezVrasta commented 3 years ago

Typescript has the wrong types, callback refs are documented in the React docs, I can't do anything if the type system you are using has a bug in their types.

alfnielsen commented 3 years ago

Okay, but useState dont provide a callback ref, but a dispatch method. An yes in a "normal" (most used) compiled JS code/react runtime, it will give you the same result, but if you do other compiling of dispatch its not the same. (Or React changes to an optimized dispatch ect..) (So no, typescript are not incorrect, but seeing your answer, I guess that is something we cannot agree on)

Still a huge thanks for the good work on Popper.js :-)

Note (workaround) for other TS people using this package: Instead of using the setReferenceElement directly in ref, just wrap it in a callback ref method like this:

import React, { useState } from 'react';
import { usePopper } from 'react-popper';

const Example = () => {
  const [referenceElement, setReferenceElement] = useState(null);
  const [popperElement, setPopperElement] = useState(null);
  const [arrowElement, setArrowElement] = useState(null);
  const { styles, attributes } = usePopper(referenceElement, popperElement, {
    modifiers: [{ name: 'arrow', options: { element: arrowElement } }],
  });

  return (
    <>
      <button type="button" ref={ref=>setReferenceElement(ref)}> // <-- HERE
        Reference element
      </button>

      <div ref={ref=>setPopperElement(ref)} style={styles.popper} {...attributes.popper}> // <-- HERE
        Popper element
        <div ref={ref=>setArrowElement(ref)} style={styles.arrow} /> // <-- HERE
      </div>
    </>
  );
};
piecyk commented 3 years ago

Note (workaround) for other TS people using this package: Instead of using the setReferenceElement directly in ref, just wrap it in a callback ref method like this:

Nope, there is no workaround, it's expected behaviour. You just need to specify type, something like

const [referenceElement, setReferenceElement] = useState<HTMLButtonElement | null>(null)
const [popperElement, setPopperElement] = useState<HTMLDivElement | null>(null)
const [arrowElement, setArrowElement] = useState<HTMLDivElement | null>(null)
thepuzzlemaster commented 2 years ago

Also, you can clean up the syntax a bit if you like as well. Instead of: <div ref={ref=>setPopperElement(ref)} style={styles.popper} {...attributes.popper}>

You can do: <div ref={setPopperElement} style={styles.popper} {...attributes.popper}>

BangKarlsen commented 1 year ago

Note (workaround) for other TS people using this package: Instead of using the setReferenceElement directly in ref, just wrap it in a callback ref method like this:

Nope, there is no workaround, it's expected behaviour. You just need to specify type, something like

const [referenceElement, setReferenceElement] = useState<HTMLButtonElement | null>(null)
const [popperElement, setPopperElement] = useState<HTMLDivElement | null>(null)
const [arrowElement, setArrowElement] = useState<HTMLDivElement | null>(null)

Oh I wish that was included as a note to the example on the popper page. Spent a day sorting this out, and I expect many people use react-popper with TypeScript.