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

Fix the useRef typing to include undefined when called without initial value #3056

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

cmlenz
Copy link
Contributor

@cmlenz cmlenz commented Mar 10, 2021

Fix the PropRef typing in hooks to include the undefined type for the current property, as it will be undefined (at least) on initial render.

This was changed in #2803 when the current property was made non-optional, but it still needs to include the undefined type. The current property will always be undefined on initial render, for example:

import {useRef} from 'preact/hooks';function Test() {
  const ref = useRef<HTMLButtonElement>()
  console.log(ref.current.textContent) // oops
  return <button ref={ref}>Click</button>
}

This change made in this PR mirrors the corresponding override in the React typings:

function useRef<T = undefined>(): MutableRefObject<T | undefined>;

See https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e47aff8a3a1331d0c540550db79dd5065ab735e5/types/react/index.d.ts#L1045

@coveralls
Copy link

coveralls commented Mar 10, 2021

Coverage Status

Coverage remained the same at 99.445% when pulling 1a337a8 on cmlenz:master into bd0942f on preactjs:master.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Just checked it out but I didn't get a compile error. Presumably, we need to remove the optional sign (=?) at this line:

export function useRef<T>(initialValue?: T | null): Ref<T>;

When that's removed accessing myRef.current.foo errors as expected.

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 14, 2021

You are right, this change does not do what it was intended to do. Sorry for not checking this thoroughly myself.

In my example snippet from above, the current property will always be undefined on first render, but the typings in both React and Preact choose the pragmatic approach of not including null or undefined in its type. While this may mask bugs that TypeScript should normally warn against, the alternative would add a lot of null-checking that is not what library users would expect, apparently. Even the version of useRef that expects an argument excludes null and undefined from the return type, even though they are clearly possible.

I do wonder why this is different between the Ref type in hooks and the RefObject type in core, which does include null. Seems to me that the simplest thing would be to use that type for both createRef and useRef.

Anyway, after playing around with this some more, I have come away more confused than before, but have learned this:

  1. The useRef typings in Preact no longer make sense since Make ref types' current property non-optional #2803, as making the parameter optional means the compiler will not choose the second argument-less override. Makes me wonder why Make ref types' current property non-optional #2803 was submitted in the first place.
  2. Also, that override has its own return type PropRef<T>, which is (outside of this PR) exactly the same as the Ref<T> type in hooks.

My proposal would be to simplify the typings by keeping the argument optional, removing the override, and removing (or deprecating?) the PropRef<T> type alias. I can update the PR with this change if that sounds like a good idea to you :)

@cmlenz cmlenz force-pushed the master branch 2 times, most recently from 88878fd to d0f5758 Compare March 14, 2021 22:35
@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 14, 2021

I went ahead and updated the PR.

- when `null` is passed as the initial value, the hook returns a `RefObject` will a nullable `current` property.
- when an instance of the type is passed, the return type of the hook will not include `null` or `undefined`
- when no argument is passed to the hook, the `current` property of the returned object will include `undefined`.

This change also deprecated the `PropRef<T>` type, as it is exactly the same as `Ref<T>`.
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, this works and is much better 🙌
+1 on deprecating the PropRef type

@marvinhagemeister marvinhagemeister merged commit 4e29ec7 into preactjs:master Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants