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

[@types/react] RefObject.current should no longer be readonly #31065

Closed
4 tasks done
bschlenk opened this issue Dec 4, 2018 · 52 comments
Closed
4 tasks done

[@types/react] RefObject.current should no longer be readonly #31065

bschlenk opened this issue Dec 4, 2018 · 52 comments

Comments

@bschlenk
Copy link
Contributor

bschlenk commented Dec 4, 2018

It is now okay to assign to ref.current, see example: https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables

Trying to assign a value to it gives the error: Cannot assign to 'current' because it is a constant or a read-only property.

@ferdaber
Copy link
Contributor

ferdaber commented Dec 5, 2018

A PR would be appreciated! It should be a very easy one line change 😊

@jagatstha
Copy link

@ferdaber,
Hi, I like to pick this issue. As I am new to typescript (just a week) and it will be my first contribution to open source.

I have gone through node_modules/@types/react/index.d.ts and there is the following code at line no 61 which declare current as readonly.

interface RefObject<T> {
        readonly current: T | null;
    }

if this is the issue, then I can solve. can you guide for my first PR?
Thank you for your time :)

@ferdaber
Copy link
Contributor

Yes, it's just removing the readonly modifier on that line.

@pshrmn
Copy link
Contributor

pshrmn commented Dec 11, 2018

React.useRef returns a MutableRefObject whose current property is not readonly. I suppose the question is whether the ref object types should be unified.

@ferdaber
Copy link
Contributor

They should be unified, the React runtime has no limitation on the current property created by React.createRef(), the object itself is sealed but not frozen.

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

It's not. It'a intentionally left readonly to ensure correct usage, even if it's not frozen. Refs initialized with null without specifically indicating you want to be able to assign null to it are interpreted as refs you want to be managed by React -- i.e. React "owns" the current and you're just viewing it.

If you want a mutable ref object that starts with a null value, make sure to also give | null to the generic argument. That will make it mutable, because you "own" it and not React.

Perhaps this is easier for me to understand as I have worked with pointer-based languages often before and ownership is very important in them. And that's what refs are, a pointer. .current is dereferencing the pointer.

@ferdaber
Copy link
Contributor

That's fair, I've been using React.createRef() as a helper function to just create a pointer, as React won't manage it unless it's passed in as a ref prop, but you can just as well create a simple pointer object with the same structure.

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

We could modify createRef to have the same overload logic, but because there is no argument it'd have to be with a conditional type return. If you include | null it'd return MutableRefObject, if you don't it'd be (immutable) RefObject.

@ferdaber
Copy link
Contributor

Would that work for those who don't have strictNullTypes enabled, though?

@Jessidhia
Copy link
Member

🤔

Should we really make it easier to write incorrect code for those that do have strictNullTypes enabled?

@bschlenk
Copy link
Contributor Author

When I created this issue I didn't realize there already was this | null overload. It isn't a very common pattern so I didn't expect something like that to exist. I don't know how I missed it in the documentation though, it is very well documented and explained. I'm fine with closing this as a non-issue unless you want to unify useRef and createRef.

@ferdaber
Copy link
Contributor

ferdaber commented Dec 12, 2018

Since the issue itself was based on useRef and not createRef, I'm also okay with closing this as very few people out there directly modify the dereferenced pointer value in createRef.

@ferdaber
Copy link
Contributor

I do wonder though if this overload pattern will pan out well, looking at the hooks documentation we see a few uses of useRef with a default value, in which case the value of .current may never be null but it can be kind of annoying for users to constantly use the non-null assertion operator to get past it.

@bschlenk
Copy link
Contributor Author

Would it make more sense for the return value to be mutable if an initial value is given, but immutable if it is omitted? If an initial value is given, then the ref probably isn't going to be passed to a component via ref, and used more like an instance variable. On the other hand, when creating a ref solely to be passed to a component, then an initial value probably won't be provided.

@ferdaber
Copy link
Contributor

That's kind of what I was thinking. @Kovensky what are your thoughts?

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

@ferdaber useRef as it is defined right now will always be mutable and not nullable unless you explicitly both give it a generic argument that does not include | null and an initial value of null.

Refs that are passed to a component should be initialized with null because that's what React sets refs to when they are being released (e.g. when unmounting a component that is conditionally mounted). useRef without passing a value would cause it to start undefined instead, so now you'd also have to add | undefined to something that only had to care about | null.

The problem in React's documentation and using useRef without an initial value is that the React team doesn't seem to much care about the difference between null and undefined. That might be a Flow thing.


Anyway, the way I defined useRef fits exactly @bschlenk's use case, only that null is the "ommitted" value, for the reasons I mention above.

@ferdaber
Copy link
Contributor

Ah, looking more closely shows that, and it's interesting that it's initialized as undefined in the React source without a parameter. Cool so everything's all peachy, it sounds like 👍

@bschlenk
Copy link
Contributor Author

I think it is fine the way it is, just a bit odd that whether you include the | null, the type of current still can be null, just the mutability has changed. Also, the React docs always explicitly pass null, so is it even valid to omit the initialValue?

@Jessidhia
Copy link
Member

It's generally not, and at least in the past when I pointed that out in some piece of documentation the React team said that "even if it works" it's not intended to be ommitted.

It's why the first argument to createContext is required, for example, even if you do want a context to start with undefined. You have to actually pass undefined.

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

@ferdaber that's because they're not caring about types there. What should the type of current in a useRef() (no arguments) be? undefined? any? Whatever it is, even if you give a generic argument, it'll have to be |ed with undefined. And if it is a ref that you use as a react element ref (not just as thread local storage) then you also have to | null because React may write a null there.

Now you find yourself the current being T | null | undefined instead of just T.

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

// you should always give an argument even if the docs sometimes miss them
// $ExpectError
const ref1 = useRef()
// this is a mutable ref but you can only assign `null` to it
const ref2 = useRef(null)
// this is also a mutable ref but you can only assign `undefined`
const ref3 = useRef(undefined)
// this is a mutable ref of number
const ref4 = useRef(0)
// this is a mutable ref of number | null
const ref5 = useRef<number | null>(null)
// this is a mutable ref with an object
const ref6 = useRef<React.CSSProperties>({})
// this is a mutable ref that can hold an HTMLElement
const ref7 = useRef<HTMLElement | null>(null)
// this is the only case where the ref is immutable
// you did not say in the generic argument you want to be able to write
// null into it, but you gave a null anyway.
// I am taking this as the sign that this ref is intended
// to be used as an element ref (i.e. owned by React, you're only sharing)
const ref8 = useRef<HTMLElement>(null)
// not allowed, because you didn't say you want to write undefined in it
// this is essentially what would happen if we allowed useRef with no arguments
// to make it worse, you can't use it as an element ref, because
// React might write a null into it anyway.
// $ExpectError
const ref9 = useRef<HTMLElement>(undefined)

@ferdaber
Copy link
Contributor

Yeah this DX works for me, and the documentation is A++ so I don't think most people will get confused on it. Thanks for this elaboration! Honestly you should just permalink this issue in the typedoc :)

@Jessidhia
Copy link
Member

Jessidhia commented Dec 12, 2018

There could be a case for supporting useRef<T>() and making it behave the same as useRef<T | undefined>(undefined); but whatever ref you make with that still can't be used as an element ref, just as thread local storage.

The problem is... what happens if you don't give a generic argument, which is allowed? TypeScript will just infer {}. The correct default type is unknown but we can't use it.

@hielfx
Copy link

hielfx commented Jan 13, 2019

I'm getting this error with the following code:

// ...
let intervalRef = useRef<NodeJS.Timeout>(null); // also tried with const instead of let
// ... 
useEffect( () => {
    const interval = setInterval( () => { /* do something */}, 1000);
    intervalRef.current = interval; // In this line I'm getting the error

    return () => {
        clearInterval(intervalRef.current);
    }
})
// ...

And when I remove the readonly in here it works:

interface RefObject<T> {
    readonly current: T | null;
}

I'm new with both reack hooks and typescript (just trying them out together) so my code could be wrong

@ferdaber
Copy link
Contributor

By default if you create a ref with a null default value and specify its generic parameter you're signaling your intent to have React "own" the reference. If you want to be able to mutate a ref that you own you should declare it like so:

const intervalRef= useRef<NodeJS.Timeout | null>(null) // <-- the generic type is NodeJS.Timeout | null

@hielfx
Copy link

hielfx commented Jan 13, 2019

@ferdaber Thanks for that!

@RoystonS
Copy link
Contributor

Taking this a stage further and looking at the return type of current, should it perhaps be T rather than T | null? With the advent of hooks, we don't always have the case that all refs might be null, particularly in the frequent case where useRef is called with a non-null initializer.

Carrying on from the excellent example list in #31065 (comment), if I write:

const numericRef = useRef<number>(42);

what should the type of numericRef.current be? There's no need for it to be number | null.

If we defined the types and functions as follows:

interface RefObject<T> {
  current: T;
}

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

function createRef<T>(): RefObject<T | null>;

that would produce the following usages and types:

const r1 = useRef(42);
// r1 is of type RefObject<number>
// r1.current is of type number (not number | null)

const r2 = useRef<number>(null);
// r2 is of type RefObject<number | null>
// r2.current is of type number | null

const r3 = useRef(null);
// r3 is of type RefObject<null>
// r3.current is of type null

const r4 = createRef<number>();
// r4 is of type RefObject<number | null>
// r4.current is of type number | null

Is anything there wrong?

For the answer to "what is the type of an unparameterised useRef?", the answer is that that call (despite the documentation) is incorrect, according to the React team.

@Jessidhia
Copy link
Member

Jessidhia commented Jan 25, 2019

I added the overload with |null specifically as a convenience overload for DOM/component refs, because they always start null, they are always reset to null on unmount, and you never reassign the current yourself, only React.

The readonly is there more to guard against logic errors than a representation of true JavaScript frozen object / getter-only property.

You can only make it by accident when you both give a generic argument that says you do not accept null while initializing the value to null anyway. Any other case will be mutable.

@RoystonS
Copy link
Contributor

RoystonS commented Jan 25, 2019

Ah, yes, I see now that MutableRefObject<T> already has the | null case removed, compared to RefObject<T>. So the useRef<number>(42) case already works correctly. Thanks for the clarification!

What do we need to do with createRef? Right now it returns an immutable RefObject<T>, which is causing problems in our codebase as we'd like to pass them around and use them in the same way as (mutable) useRef ref objects. Is there a way we could tweak the createRef typing to allow it to form mutable ref objects?

(And the ref attribute is defined to be of type Ref<T> which includes RefObject<T>, thereby making everything immutable. This is a big problem for us: even if we get a mutable ref out of useRef, we can't make use of the fact that it's immutable through a forwardRef call.)

@aappddeevv
Copy link

Did another comment about forwardRef components get created? Essentially, you can't forward a ref and change it's current value directly, which I think is part of the point of forwarding the ref.

@naorye
Copy link

naorye commented Oct 29, 2019

I created the following hook:

export const useCombinedRefs = <T>(...refs: Ref<T>[]) =>
    useCallback(
        (element: T) =>
            refs.forEach(ref => {
                if (!ref) {
                    return;
                }

                if (typeof ref === 'function') {
                    ref(element);
                } else {
                    ref.current = element; // this line produces error
                }
            }),
        refs,
    );

And I get an error: "Cannot assign to 'current' because it is a read-only property."
Is there a way to solve it without changing current to writeable?

@Jessidhia
Copy link
Member

For that, you're going to have to cheat.

(ref.current as React.MutableRefObject<T> ).current = element;

Yes, this is a bit unsound, but it's the only case I can think of where this kind of assignment is deliberate and not an accident -- you're replicating an internal behavior of React and thus have to break the rules.

@guirenpei
Copy link

We could modify createRef to have the same overload logic, but because there is no argument it'd have to be with a conditional type return. If you include | null it'd return MutableRefObject, if you don't it'd be (immutable) RefObject.

thanks a lot

@ArthurClemens
Copy link

ArthurClemens commented Mar 7, 2020

(ref.current as React.MutableRefObject<T>).current = element;

should be:

(ref as React.MutableRefObject<T>).current = element;

Pajn added a commit to Pajn/DefinitelyTyped that referenced this issue Mar 20, 2020
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.)
- [ ] Add or edit tests to reflect the change. (Run with `npm test`.)
- [x] Follow the advice from the [readme](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#common-mistakes).
- [ ] 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: DefinitelyTyped#31065, DefinitelyTyped#39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  DefinitelyTyped#39062
Pajn added a commit to Pajn/DefinitelyTyped that referenced this issue Mar 20, 2020
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.)
- [ ] Add or edit tests to reflect the change. (Run with `npm test`.)
- [x] Follow the advice from the [readme](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#common-mistakes).
- [ ] 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: DefinitelyTyped#31065, DefinitelyTyped#39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  DefinitelyTyped#39062
Pajn added a commit to Pajn/DefinitelyTyped that referenced this issue Mar 20, 2020
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](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#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: DefinitelyTyped#31065, DefinitelyTyped#39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  DefinitelyTyped#39062
Pajn added a commit to Pajn/DefinitelyTyped that referenced this issue Mar 20, 2020
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](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#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: DefinitelyTyped#31065, DefinitelyTyped#39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  DefinitelyTyped#39062
uniqueiniquity pushed a commit that referenced this issue Mar 31, 2020
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](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#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: #31065, #39062
- [x] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`. If for reason the any rule need to be disabled, disable it for that line using `// tslint:disable-next-line [ruleName]` and not for whole package so that the need for disabling can be reviewed.

Fixes  #39062
@s-h-a-d-o-w
Copy link
Contributor

you're replicating an internal behavior of React

Does that mean the docs here are outdated? Because they clearly describe this as an intended workflow, not internal behavior.

@ferdaber
Copy link
Contributor

ferdaber commented Jul 8, 2020

The docs in that case is referring to a ref that you own, but for refs that you pass as a ref attribute to an HTML element, then they should be readonly to you.

function Component() {
  // same API, different type semantics
  const countRef = useRef<number>(0); // not readonly
  const divRef = useRef<HTMLElement>(null); // readonly

  return <button ref={divRef} onClick={() => countRef.current++}>Click me</button>
}

@s-h-a-d-o-w
Copy link
Contributor

s-h-a-d-o-w commented Jul 8, 2020

My bad, should've scrolled up further. Got an error about readonly for a ref I owned and assumed that that was the same case. (I use a different workflow now and can't reproduce the error any more, unfortunately...)

Anyway, thank you very much for the explanation!

@sylvanaar
Copy link

I'm unclear if the createRef portion of the topic moved - but I will post here also.

I use a popular navigation library for React Native (React Navigation). In its documentation it commonly calls createRef and then mutates the ref. I am sure that this is because React isn't managing them (there is no DOM).

Should the type for React Native be different?

See: https://reactnavigation.org/docs/navigating-without-navigation-prop

@Imperyall
Copy link

@sylvanaar
I also faced this problem when initializing navigation, have you found a solution?

@mexmirror
Copy link

Summarising what I just read: The only way to set a value to a ref created by createRef<T> is to cast it every time you use it? What's the reasoning behind that? In that case the readonly is practically preventing settings a value to the ref at all, therefore defeating the whole purpose of the function.

@jeffvandyke
Copy link

TLDR: If your initial value is null (details: or something else outside of the type parameter), then add | null to your type parameter, and that should make .current able to be assigned to like normal.

@caleb-harrelson
Copy link

@sylvanaar / @Imperyall, I couldn't get typescript to be happy with const isReadyRef = createRef<boolean | null>(). I ended up changing the bit that sets the ref to true/false to cast it first.

const mutableRef = isReadyRef as React.MutableRefObject<boolean>
mutableRef.current = true

@Njaah-0
Copy link

Njaah-0 commented Dec 16, 2020

I came to this issue because I got same error when using functions as a type. This was my code:

const pagesInitListener = useRef<() => void | null>(null);
pagesInitListener.current = () => { ... }

Solution was to wrap function into parenthesis, like this:
const pagesInitListener = useRef<(() => void) | null>(null);

@jdlk07
Copy link

jdlk07 commented Jan 2, 2021

I was trying to handle React Navigation's Navigating without the navigation prop initialization as well, I ended up manually assigning the type like so since the type is surely going to be either a boolean or null.

export const isReadyRef: React.MutableRefObject<boolean | null> = React.createRef()

@orta
Copy link
Collaborator

orta commented Aug 3, 2021

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.

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

No branches or pull requests