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

[Basic] useRef Hook Documentation. #388

Closed
rajivpunjabi opened this issue Jan 24, 2021 · 11 comments · Fixed by #389
Closed

[Basic] useRef Hook Documentation. #388

rajivpunjabi opened this issue Jan 24, 2021 · 11 comments · Fixed by #389
Assignees
Labels
BASIC Basic Cheatsheet

Comments

@rajivpunjabi
Copy link

rajivpunjabi commented Jan 24, 2021

What cheatsheet is this about? (if applicable)

Basic cheatsheet

What's your issue or idea?

According to the documentation useRef Hook has two options.

const ref1 = useRef<HTMLElement>(null!); 
const ref2 = useRef<HTMLElement | null>(null);

The first option will make ref1.current read-only. But after trying out some examples I feel there are some issues. As it shows MutableRefObject for the first option. So ref1.current is not read-only.

I am exploring TypeScript with React so maybe I am missing something. Can someone please help me with this?

(If applicable) Reproduction of issue in TypeScript Playground

Playground

@rajivpunjabi rajivpunjabi added the BASIC Basic Cheatsheet label Jan 24, 2021
@swyxio
Copy link
Collaborator

swyxio commented Jan 24, 2021

very nice issue! clearly we are wrong in our useRef notes.

i think @ferdaber might have some insight as to why MutableRefObject is inferred in that example. however based on this I am inclined to change our documented example to the one in your playground const ref2 = React.useRef<HTMLSpanElement>(null);

@rajivpunjabi
Copy link
Author

rajivpunjabi commented Jan 24, 2021

According to useRef Hook Type Definitions.

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

From Playground.

const ref1 = React.useRef<HTMLSpanElement>(null!); //MutableRefObject
const ref2 = React.useRef<HTMLSpanElement>(null); //RefObject
const ref3 = React.useRef<HTMLSpanElement | null>(null); //MutableRefObject

These ref matches the first two type definitions.

  • For ref1 we are defining the initialValue as not null ( non-null assertion operator ) so it matches the first type and returns a MutableRefObject.
  • For ref2 we are defining the initialValue as null so it matches the second type and returns a RefObject.
  • For ref3 we are defining the generic parameter as either of HTMLSpanElement | null and the initialValue is null so it matches the first type and returns a MutableRefObject.

@swyxio
Copy link
Collaborator

swyxio commented Jan 25, 2021

ok so check out this example though: (playground)

export default function MyComponent() {
  const ref1 = React.useRef<HTMLDivElement>(null!); // MutableRefObject
  const ref2 = React.useRef<HTMLDivElement>(null); // RefObject
  React.useEffect(() => {
    console.log(ref1.current.innerHTML);
    // TypeScript won't require null-check e.g. ref1 && ref1.current
    console.log(ref2.current.innerHTML);
    // Type error: Object is possibly 'null'.
  });
  return [
    <div ref={ref1}> etc </div>,
    <div ref={ref2}> etc </div>
  ];
}

the goal is to not require null checks because the ref is always bound to the element in this case. i think this is why i made the original recommendation.

@rajivpunjabi
Copy link
Author

If the goal is not to require null checks then its absolutely correct but stating ref1.current will be read-only is wrong. As it is a MutableRefObject so it is not read-only.

@swyxio
Copy link
Collaborator

swyxio commented Jan 26, 2021

alright :)

swyxio added a commit that referenced this issue Jan 26, 2021
@swyxio
Copy link
Collaborator

swyxio commented Jan 26, 2021

i made those tweaks here: https://github.com/typescript-cheatsheets/react/pull/389/files , lmk if any changes but otherwise thank you for the feedback :)

swyxio added a commit that referenced this issue Jan 27, 2021
@bchenSyd
Copy link

would this be more explanatory?

import * as React from "react";

export default function MyComponent() {
  const ref1 = React.useRef<HTMLDivElement>(null!); // MutableRefObject
  const ref2 = React.useRef<HTMLDivElement>(null); // RefObject
  React.useEffect(() => {
    ref1.current=ref2.current!;
    // ok
    ref2.current=ref1.current;
    //Cannot assign to 'current' because it is a read-only property.(2540)
    console.log(ref1.current.innerHTML);
    // TypeScript won't require null-check e.g. ref1 && ref1.current
    console.log(ref2.current.innerHTML);
    // Type error: Object is possibly 'null'.
  });
  return [
    <div ref={ref1}> etc </div>,
    <div ref={ref2}> etc </div>
  ];
}

@swyxio
Copy link
Collaborator

swyxio commented Feb 20, 2021

@bochen2014 i dont feel strongly about this code sample 🤷🏽

@thien-do
Copy link
Contributor

thien-do commented Apr 30, 2021

@sw-yx I think we should not encourage the null check bypassing here. The reason is that check is valid in real life, because the developer may forget to assign the ref to an element in his render, or if the ref-ed element is conditionally rendered:

const Foo = () => {
  const ref1 = useRef<HTMLDivElement>(null); // RefObject
  useEffect(() => {
    console.log(ref1.current); // This will be null
  });
  return <div> etc </div>; // Oops, I forgot to assign the ref
}
const Foo = () => {
  const ref1 = useRef<HTMLDivElement>(null); // RefObject
  useEffect(() => {
    console.log(ref1.current); // Only works on Monday!
  });
  return isMonday ? <div ref={ref1} /> : <div> etc </div>;
}

@sw-yx would you mind if I have a PR to clarify this more?

@swyxio
Copy link
Collaborator

swyxio commented Apr 30, 2021

haha nice example @thien-do. yes PR welcome, but i think giving the 3 examples and letting people decide based on the pros and cons feels right. otherwise i'll just keep getting more and more issues fighting back and forth haha

thien-do pushed a commit to thien-do/react that referenced this issue May 1, 2021
This is my suggestion follow a [comment](typescript-cheatsheets#388 (comment)) in typescript-cheatsheets#388 .

The problem with the current guide is that most newcomers will use the `null!` approach, which could cause hidden bug in practice, when the developer forget to assign the ref or conditionally render the ref-ed element. I saw this a lot, because this guide is the official reading recommendation in my company, and I keep seeing that bug.

My proposal is to not bypassing TypeScript, but let TypeScript does its job, by correctly tell it whether we need a RefObject and MutableRefObject. This also follows React's official guides, where there are 2 corresponding use cases (DOM access and mutable variable)
@thien-do
Copy link
Contributor

thien-do commented May 1, 2021

@sw-yx totally understand. In fact, I don't want to add any more use cases, but want to simplify the usage there :D

Anyway, I think code speaks better so I made a PR for you to review: #412 :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BASIC Basic Cheatsheet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants