Fixes #35572, and the problem that using useRef with ref={} currently yields error

[staeke]

Fixes #35572, and the problem that using

const a = useRef<HTMLDivElement>(); 
return <div ref={a}/>

currently yields error

Please fill in this template.

• [ x ] Use a meaningful title for the pull request. Include the name of the package modified.
• [ x ] Test the change in your own code. (Compile and run.)
• [ x ] Add or edit tests to reflect the change. (Run with npm test.)
• [ x ] Follow the advice from the readme.
• [ x ] Avoid common mistakes.
• [ x ] Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• [ x ] Provide a URL to documentation or source code which provides context for the suggested changes: https://reactjs.org/docs/hooks-reference.html#useref

[typescript-bot]

@staeke Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon - 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.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[eps1lon]

My issue with this change is that the overloaded ref type is only encountered before the ref is initialized. With this change the "type lifecycle" becomes undefined - HTMLDivElement - null (- HTMLDivElement - null)*. Since most of the use cases will not special case the very first cycle (undefined) I think it's fine to require explicit type annotation (HTMLDivElement | undefined) if you're interested in it or require const divRef = useRef<HTMLDivElement>(null).

As it stands right now this would be a breaking change if previous code strictly compared to null (current === null) since this would only narrow down to HTMLDivElement | undefined.

[typescript-bot]

@staeke 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 or comments. Thank you!

[staeke]

Thanks for reviewing @eps1lon .

I agree and disagree. Since the change is potentially breaking I agree we should be bumping the major. I don't think status quo is acceptable though. But I do think we can reach a compromise that provides reasonable results for all use cases

1.

First, and to the point with this PR - I don't think it's acceptable to require actual code changes from users (as opposed to typing changes) to accommodate ts issues - I'm thinking about adding a null parameter to useRef. That goes against the fundamental idea with typescript. Then, to your first suggestion. Writing:

const ref = useRef<HTMLDivElement|undefined>()
return <div ref={ref}/>

...yields an error, so I don't see how your proposed solution works. Also, I personally think it's dirty to require |undefined for such a common use case to be sprinkled everywhere.

But, I'm all fine with changing RefAttributes and ClassAttributes instead, like below (undefined added):

interface RefAttributes<T> extends Attributes {
    ref?: Ref<T | undefined>;
}
interface ClassAttributes<T> extends Attributes {
    ref?: LegacyRef<T | undefined>;
}

Would you be ok with that?

2.

Secondly, I don't think it's acceptable to see that a textbook example yields a typescript error, and one that is also not correct. I'm thinking about:

const ref = useRef()
return <div ref={ref}/>

I think this use case is best solved by having useRef default its type parameter to any. Which, I argue, is fundamentally what we know here. Not using a type parameter, should result in weaker typing, but not compile failure. But, not addressed in this PR. I could though.

3.

Third, I think the null overload of useRef returning a non-mutable ref object is an error. It's just as mutable and we should change it. Do you agree?

[staeke]

I have yet another problem with not including undefined with useRef being used without parameters; it simply doesn't reflect how React works. The output of useRef in reality IS undefined if you don't pass parameters. Hiding this fact is just producing worse typings, and more bugs. I think we have two options, either require undefined to be part of the type if no parameters are passed (seems overly unsmart to me). Or, simply, draw the conclusion, that it has to be part of the return type. Take the example:

const n = useRef<number>()
n.current.toFixed()

Currently yields no ts error. Breaks at runtime though

Now that could be fixed by amending MutableRefObject like I did, or e.g. change the useRef signature. Maybe that's better. How about:

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

And, to be clear, and summarize it all, this would my preferred overloads:

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

[Jessidhia]

There is a reason why the overloads are like they are.

Maybe I need to write a lot more comments in the .d.ts files to elaborate; this is something that comes up time and again, and the problem is always the same.

First, RefObject can never hold an undefined when you create it with createRef. It's initialized with null, and you are never supposed to write into it.

As for the overloads, here are the current ones:

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

The first overload is what you should hit whenever your initial value is sufficient to infer the generic, or when it is directly compatible with the explicit generic. This is what will happen with, for example, useRef<0>(0), as useless as that is. Or useRef<number>(0), which is the same as what's inferred by useRef(0). This is the common case.

The second overload is the convenience overload when you want something be passed as a ref={} value. useRef<RefType>(null). It's explicitly not a mutable object because its intended use is to be written to by React. From your point of view, .current is a read-only value. If you want to make a mutable version of it, then just specify | null on your generic like in useRef<RefType | null>(null) to hit the first overload.

The third overload is there when you give no arguments (i.e. the "first argument" is undefined), and it's what'd be used by useRef<Foo>(). This of course prevents you from giving it as a ref={} because React will write null into it when the ref-ed component unmounts, but never write undefined. You can also just specify useRef<Foo | null>() here, which does exactly what you ask for in your comments, but if you pass it to a ref={} the .current will only ever be undefined on the initial render pass; React will set it to Foo | null exclusively once it has control over the ref={}. It will also never be undefined inside any Effect or LayoutEffect or their destructors, but this would force you to check for undefined anyway.

As I said in all cases, unless you want to use the ref={} convenience overload, then include the | null in your generic argument. Because React wants to write null into it, not specifying the null yourself would make the type incorrect. On the other hand, if you let the code assume null | undefined can be written there on all cases, like on this PR, then you create an overly-wide and less safe type when you use it as mutable fiber-local storage and not as a ref={} argument, and specify an outright unreachable type when used with createRef().

[mikew]

@Jessidhia So how does one create a ref object, with a generic argument, an initial value of null, and have TS complain when there's improper usage? I'd expect a TS error in this:

const ref = useRef<string>(null)
console.log(ref.current.toUpperCase())

But it happily compiles: https://codesandbox.io/s/sparkling-firefly-e6fjk

[staeke]

@mikew good point. Can you pls add it in the other thread - the open PR at #38260

[eps1lon]

@Jessidhia So how does one create a ref object, with a generic argument, an initial value of null, and have TS complain when there's improper usage? I'd expect a TS error in this:

const ref = useRef<string>(null)
console.log(ref.current.toUpperCase())

But it happily compiles: codesandbox.io/s/sparkling-firefly-e6fjk

codesandbox has likely strictNullChecks disabled. The type checker reports and error with strictNullChecks: true: http://www.typescriptlang.org/play/index.html#code/JYWwDg9gTgLgBAbzgVwM4FMBK6BmcC+cOUEIcARFOgIYDGM5A3ALABQbOyAdvcBF3AAq6VDAAUASkRs4KDNhwAeUVGBcA5gD4xXZABs9EgHS1kUKlxhGYEAKpgw6KAGFqGSS1ay0WXMpiqGtrGpuboltZ2Dk6u7hKeMnK+Sipq6nAAPihcACa4aug52tx5OAU5IWYWVjb2ji5u6B6JPgr+gelZJflchcEmVeE1UfWxTfFsLfJ+qRqZcLoG2ouGA2ERtdENcZ7e0ykBafMr-aHVkXUxjc2s+GxAA

[karimuddin-tech]

React.useRef<any | null>(); this is work for me