facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
225.53k stars 45.98k forks source link

Bug: useState with class as initial state throws an error #21057

Open jneuendorf opened 3 years ago

jneuendorf commented 3 years ago

Using a JavaScript class as initial state in useState throws an error. This is because mountState check if the initial state is a function by typeof initialState === 'function' and classes are technically functions. However since ES6+, I feel like most developers don't consider classes functions because of the less prototype-inheritance feel since ES6 and the class keyword.

React version: 17.0.2

Steps To Reproduce

  1. Define an ES6 class
  2. Use the class as initialState argument to useState

Code example:

import { useState } from "react";
import ReactDOM from "react-dom";

class A {}

function App() {
  const [cls, setCls] = useState(A);
  return <h2>{cls.name}</h2>;
}

ReactDOM.render(<App />, document.body);

The current behavior

TypeError: Cannot call a class as a function
    at _classCallCheck (eval at z (eval.js:42), <anonymous>:3:11)
    at A (VM302 index.js:19)
    at mountState (react-dom.development.js:10436)
    at Object.useState (react-dom.development.js:10919)
    at useState (react.development.js:954)
    at App (VM302 index.js:25)

Alternatively, the error can be TypeError: Class constructor X cannot be invoked without 'new'.

The expected behavior

As mentioned in the description above, I would

I guess checking, if the initial state is actually a non-class function, could cause instances of subclasses of functions not to work (depending how the check is done).

bvaughn commented 3 years ago

I don't consider this a bug.

As the issue description mentions, useState supports two formats:

  1. Eagerly initialize the value (e.g. useState('value'))
  2. Lazily initialize the value with a function (e.g. useState(() => 'value'))

React decides which one you've done using the typeof operator. (I am not aware of an alternative to this approach but please feel free to share one if you have an idea.)

In JavaScript, the typeof a class is "function" so React thinks you're using the second format and tries to call the "function" which causes an error to be thrown. To fix this, you can wrap your class value with a function, like so:

useState(() => A);

I'll tag this as a discussion for now, but I'm not sure this is something that we'll change.

jneuendorf commented 3 years ago

@bvaughn Thanks for the quick reply. I agree that this is technically not a bug. I think, I mainly stumbled upon this, because I didn't know about the 2nd signature of useState that you mentioned.

My main concern was that future React developers probably start learning ES6 or newer, potentially not being aware of the fact that classes are just syntactic sugar.

React decides which one you've done using the typeof operator.

One of these nice little yellow notes like this

Screenshot 2021-03-23 at 20 38 15

stating that

typeof is used and any non-function callable, i.e. classes, can lead to unexpected behavior and should be wrapped in a function

would probably do, because the scenario should be pretty uncommon and the wrap in a function workaround is easily implementable for React users.

I am not aware of an alternative to this approach but please feel free to share one if you have an idea.

I searched a bit and found that there seems to be no robust / cross-browser way to achieve this. So that, additionally, makes the "note solution" score, IMO.

gaearon commented 3 years ago

Let's see if this comes up again. This is the first time someone reported being confused by this, so my impression is it's not very common to put class instances in state in the first place (e.g. typically you'd hold them in refs instead since you probably don't want to read from a mutable class instance during rendering, so there's no point in holding it in state in the first place).

If it keeps coming up we can add a warning if the prototype of the function you passed is not empty. But I don't have a clear sense that this is actually a common pitfall.

eps1lon commented 3 years ago

I recall similar issues with putting function components into state which is even harder to detect. Maybe static analysis + types might do the trick:

  1. Assume only inline function expressions are intended for functional updates i.e. setState(() => {})
  2. Reject every other callable passed

Note that the TypeScript types will already show you that the type of the state is not what you expect it to be. We could even go further and reject newables entirely in useState(value):

Screenshot of above playground when hovering over `useState` in line 14

gaearon commented 3 years ago

Tbh this just feels like some better addressed by a linter.

Or maybe we could warn if the constructor name starts with a capital letter?

gaearon commented 3 years ago

I’d like to see a PR that warns if setState or useState is passed a function whose first letter is capital. That’s enough of a hint to find the common cases.

abhijit-hota commented 3 years ago

Hello, @gaearon and @eps1lon ! 👋

If that PR being talked about isn't being worked upon, I can work on it. I suppose I have to work here? Any pointers will be helpful.