@gaearon Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• react — on npm, on unpkg

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ❌ Continuous integration tests have failed
• 🕐 Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!

{
  "type": "info",
  "now": "-",
  "pr_number": 64772,
  "author": "gaearon",
  "headCommitOid": "459a615b873700974dc887ba6cc0db653303fe5c",
  "mergeBaseOid": "0bad336fa2bea4806c4b5d0317c52326ab50099c",
  "lastPushDate": "2023-03-21T22:43:09.000Z",
  "lastActivityDate": "2023-03-27T17:08:52.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "johnnyreilly",
        "bbenezech",
        "pzavolinsky",
        "ericanderson",
        "DovydasNavickas",
        "theruther4d",
        "guilhermehubner",
        "ferdaber",
        "jrakotoharisoa",
        "pascaloliv",
        "hotell",
        "franklixuefei",
        "Jessidhia",
        "saranshkataria",
        "lukyth",
        "eps1lon",
        "zieka",
        "dancerphil",
        "dimitropoulos",
        "disjukr",
        "vhfmag",
        "hellatan",
        "priyanshurav",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "ericanderson",
      "date": "2023-03-21T20:08:07.000Z",
      "abbrOid": "9bb15d2"
    }
  ],
  "mainBotCommentID": 1470935228,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/459a615b873700974dc887ba6cc0db653303fe5c/checks?check_suite_id=11718564804"
}

Hey @gaearon,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

🔔 @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav @Semigradsky — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Added another change to get rid of MutableRefObject.

@gaearon The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@gaearon You may want to leave MutableRefObject in place, but mark it as deprecated and stub it.

Yeah I could do that! I guess one thing I have no idea about (how to check this?) is how disruptive just removing the overload would be. I imagine it would flag a lot of existing usages and force them to add null?

Would it be better to just go for a minimal change of RefObject = MutableRefObject instead?

1. Fully agree to remove readonly.
2. I agree that the initial value conceptually required. But forcing user to add null is a quite large breaking.
3. When ref need a empty value, it does not seem to make a difference whether it is null or undefined. Should we leave it to user? Maybe undefined is also ok as long as we provides the correct type inference.

What would the next step here? Does anyone want to drive this change and figure out the exact plan?

Public facing types (e.g. MutableRefObject) should remain deprecated to reduce the blast radius of this change. Then we need to fix the tests that start fail in CI. The test changes should be used to group them into migration notes or, ideally, codemods.

But I also would appreciate a retro on what our intention was and why it failed which can be reconstructed from git-blame, closed PRs/issues and https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/hooks/#useref.

This can all be done by anybody although I understand if it seems intimidating. I'll definitely help when needed. Otherwise I'll take this on next week.

Having played with this in ts playground, while I think the types are poorly named/confusing, I do not think this change should be taken as is. That said, I do agree with the readonly fix.

1. React internal testing code uses zero-arguments, undefined, and null as arguments.

• https://github.com/facebook/react/blob/12a1d140e366aa8d95338e4412117f16da79a078/packages/react-reconciler/src/__tests__/useRef-test.internal.js#L145
• https://github.com/facebook/react/blob/12a1d140e366aa8d95338e4412117f16da79a078/packages/react-reconciler/src/__tests__/useRef-test.internal.js#L183-L184

2. Taking away zero arguments without a major version bump is too much of a burden for people.
3. Typescript types tend to lean towards requiring over-protective null/undefined checking. new Map<string, {}>().get('foo') is {} | undefined.

The constraints I think we need to hold are:

type Foo = { hello: string };
useRef<Foo>(null) => { current: Foo | null }; // technically correct and a documented case
useRef<Foo>(undefined) => { current: Foo | undefined }; // undocumented but it is how the react code works
useRef<Foo>() => { current: Foo | undefined }; // Undocumented and not desirable per @gaearon but technically consistent with react codebase.
useRef<Foo>({ hello: "world" }) => { current: Foo }; // Expected behavior
useRef<Foo|undefined>({ hello: "world}) => { current: Foo | undefined }; // Expected behavior
useRef({ hello: "world"} as Foo) => { current: Foo } // expected inference

Optionally we should change the useRef(null) usage.

useRef(null) => { current: null }; // today
useRef(null) => { current: unknown } // This may be more accurate and useful?

This proposal would change the type of .current but errors still happen, they are just variable is of type unknown instead of the current variable may be null and we could cleanup the types to remove the distinction between Mutable and not (deprecated).

IFF @gaearon and the react team want to remove/discourage the use of useRef(); then we can do that at the next major. That said, IMHO, useRef(); and useRef<Foo>() is more ergonomic than useRef(null) and useRef<Foo>(null).

Edit: playground link

Edit 2: I no longer think changing useRef(null) is a good idea as it can make assignment confusing. I left the playground with this proposed unknown behavior but I retract it from the proposal.

@gaearon One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

React internal testing code uses zero-arguments, undefined, and null as arguments.

null and undefined are valid initial values — I did not argue that they aren't.

As for zero-argument case, I agree with you that it's probably quite common in the product code. It's understandable why one might write it this way (it's less to write!) and maybe that's why it's important to support. I think I'm down with keeping it if it doesn't introduce weird semantics.

I would only ask that we don't use React's internal tests as a reason. React internal tests are written before features are tested and rolled out to production, so they don't always have all the right types (we don't typecheck tests at all), are written only for the team's own understanding, and have to test very weird cases too (so they shouldn't be used as canonical references).

Taking away zero arguments without a major version bump is too much of a burden for people.

To be clear, I didn't propose a minor bump change. I meant let's think about what we want to do for 19. And maybe there's some smaller version of that that can be done in a minor.

The constraints I think we need to hold are

These look reasonable to me.

To clarify, the zero-arguments case is not the one I care about deeply. I was just a bit surprised this is allowed.

The part that seems weird to me is useRef() (which initializes to undefined) and then passing that ref to <div ref={ref} /> (which resets it to null). So does it get null | undefined | HTMLElement? Or how should this work? Currently you can't pass useRef() to <div ref={ref} /> at all but I thought that's part of what we want to change. Or do you mean you'd prefer to keep useRef() but only for non-JSX-ref cases?

For what it's worth, if we take the example you linked to from the React codebase:

function Example({phase}) {
  const hostRef = useRef();
  const classRef = useRef();
  return (
    <>
      <div key={`host-${phase}`} ref={hostRef} />
      <Component key={`class-${phase}`} ref={classRef} />
    </>
  );
}

Then the irony is that it already does not appear to work with the current typings.

Let's compress it a bit to focus on the essence:

function Example() {
  const hostRef = useRef();
  return <div ref={hostRef} />;
}

By itself, it gives me an error.

Type 'MutableRefObject<undefined>' is not assignable to type 'LegacyRef<HTMLDivElement> | undefined'.
  Type 'MutableRefObject<undefined>' is not assignable to type 'RefObject<HTMLDivElement>'.
    Types of property 'current' are incompatible.
      Type 'undefined' is not assignable to type 'HTMLDivElement | null'.

How about this one:

function Example() {
  const hostRef = useRef<HTMLDivElement>();
  return <div ref={hostRef} />;
}

Nah, still broken:

Type 'MutableRefObject<HTMLDivElement | undefined>' is not assignable to type 'LegacyRef<HTMLDivElement> | undefined'.
  Type 'MutableRefObject<HTMLDivElement | undefined>' is not assignable to type 'RefObject<HTMLDivElement>'.
    Types of property 'current' are incompatible.
      Type 'HTMLDivElement | undefined' is not assignable to type 'HTMLDivElement | null'.
        Type 'undefined' is not assignable to type 'HTMLDivElement | null'.

Okay, maybe let me pass null:

function Example() {
  const hostRef = useRef(null);
  return <div ref={hostRef} />;
}

No error! But now let's read the actual DOM ref.

function Example() {
  const hostRef = useRef(null);
  function handleFoo() {
    console.log(hostRef.current);
  }
  return <div ref={hostRef} />;
}

No error either! But wait, my IDE tells me hostRef.current is... null.

OK let's try this:

function Example() {
  const hostRef = useRef<HTMLDivElement>(null);
  function handleFoo() {
    console.log(hostRef.current);
  }
  return <div ref={hostRef} />;
}

Cool, this works as expected.

So as you can see, it seems like you already can't really have useRef() with no arguments and pass it to the DOM. If we're saying zero argument useRef() is something we'd like to support, shouldn't we support it for DOM refs too? With the downside being { current: T | null | undefined } type or something.

Not sure I agree with this myself but I wanted to clarify what irks me about how this works.

null and undefined are valid initial values — I did not argue that they aren't.

Nor did I say you did. I am simply ensuring that when others come through here who won't go through all the comments and the git blames/prs that got us here, that we can get to a base set of understanding. A lot of people come through these types. This one in particular.

I would only ask that we don't use React's internal tests as a reason.

I think this makes perfect sense. We generally refer to documentation for these types of things. It wasn't the best argument in my comment.

To be clear, I didn't propose a minor bump change. I meant let's think about what we want to do for 19. And maybe there's some smaller version of that that can be done in a minor.

Awesome. The initial PR didn't specify and it becomes a topic on nearly every PR to @types/react. I think there is a lot of cruft in these types that we would like to fix for 19.

As for a change we could do now, from my ts playground link, I think we can safely remove the readonly and we can change the return type/signatures as I mostly proposed (excluding the { current: unknown } one) and deprecate the existing types since there is no mutable vs not distinction here.

The part that seems weird to me is useRef() (which initializes to undefined) and then passing that ref to

(which resets it to null). So does it get null | undefined | HTMLElement? Or how should this work? Currently you can't pass useRef() to

at all but I thought that's part of what we want to change. Or do you mean you'd prefer to keep useRef() but only for non-JSX-ref cases?

Modifying my proposal to add the following overload:
declare function useRefEanderson(): { current: undefined }; gets you the error Type 'undefined' is not assignable to type 'HTMLInputElement | null'.(2322) when you const foo = useRef(); <input ref={foo}/>, which is consistent with the current behavior.

Unfortunately, I can't think of a way to get the type later inferred correctly and this will just be sad.

For 19, we could say that not specifying a generic nor an initial value gets you a default type of Element | null for current. This is likely what the user wants as I can't think of a (common) reason not to specify a default besides this case?

I agree useRef() by itself right now is pointless but we still shouldn't break it. Our choices for the future would be unknown, 'any', 'never', or 'null' in this case.

• unknown would be assignable to const foo: unknown = {foo: 5} but not accessible foo.anything // error.
• any would both be assignable and accessible, with foo.anything // any
• never is neither
• null is annoying because its allowed for const foo = {current: null}; <input ref={foo}/> today but foo.anything is always deref'ing null.

Updated ts playground link

I agree useRef() by itself right now is pointless

So, in plain JS it's not quite pointless — e.g. it's useful for this pattern

const ref = useRef()
if (!ref.current) {
  ref.current = new Thing()
}

Or say

const timeoutRef = useRef()

function handleStart() {
  timeoutRef.current = setTimeout(...)
}

function handleStop() {
  if (timeoutRef.current) {
    clearTimeout(timeoutRef.current)
  }
}

So it's useful for non-DOM cases where people imply it's undefined initially, but mean some more specific type later.

If I understand correctly, to express this in TS, one needs to always add useRef<T>(). Is this assumption right?

I've updated this PR — does this match your proposal? Or is there a part of your proposal that isn't reflected in this PR? I'm very new to TS so I don't quite understand how your overrides would layer.

If we have a final proposal it would be good if I could run it by our team to verify I'm not making some silly assumptions.

@gaearon The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

Agree its not pointless in plain js. But in typescript I don't think there is a clear way (currently) for the inference to be differed.

If I understand correctly, to express this in TS, one needs to always add useRef(). Is this assumption right?

Roughly, in your example it would be like this:

const timeoutRef = useRef<ReturnType<typeof setTimeout>>();

function handleStart() {
  timeoutRef.current = setTimeout(...)
}

function handleStop() {
  if (timeoutRef.current) {
    clearTimeout(timeoutRef.current)
  }
}

which would hit this overload in my playground:

declare function useRef<T>(): { current: T | undefined } // undocumented 'truth'

I've updated this PR — does this match your proposal? Or is there a part of your proposal that isn't reflected in this PR?

The previous MutableRefObject<T> was just { current: T } and RefObject<T> was { current T | null }. Assigning type MutableRefObject<T> = RefObject<T> changes its effective type to { current: T | null } and the change as it stands changes the return value of useRef to an effective { current: T | null } from its current { current: T }.

This is what I meant that the current types are confusing. I need to tend to other things this evening but I would be happy to put up a PR tomorrow if you want.

Ahh I see, I missed this. Yeah if you don't mind taking this on, I'd appreciate that a lot! Thanks.

#64896 for an alternate proposal.

Keeping RefObject since that's shorter. RefObject will become mutable an no longer contain | null. We'll deprecate MutableRefObject later.

@gaearon I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Apr 26th (in a week) if the issues aren't addressed.

@gaearon To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!