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

React typings allow false for class components, but not for stateless #18912

Closed
4 tasks done
magnushiie opened this issue Aug 13, 2017 · 25 comments · Fixed by #65135
Closed
4 tasks done

React typings allow false for class components, but not for stateless #18912

magnushiie opened this issue Aug 13, 2017 · 25 comments · Fixed by #65135

Comments

@magnushiie
Copy link
Contributor

magnushiie commented Aug 13, 2017

As mentioned in React documentation, a stateless component can return false for empty content. Currently StatelessComponent is defined as:

interface StatelessComponent<P = {}> {
        (props: P & { children?: ReactNode }, context?: any): ReactElement<any> | null;

whereas Component has

class Component<P, S> {
        constructor(props?: P, context?: any);
        // ...snip...
        render(): JSX.Element | null | false;

Shouldn't these types be the same (including eliminating the ReactElement vs JSX.Element difference)? And to avoid future such issues (#14064, this one) it should probably be a named type, e.g. RenderResult.

A search for | null also comes up with ComponentSpec.render which probably should also have the same result type.

@magnushiie
Copy link
Contributor Author

Note: React 16 will allow arrays and strings and arrays of strings also. So at one point it would be similar to ReactNode.

Also, if unifying with ReactNode, the recursive definition can be done as described by @ahejlsberg in microsoft/TypeScript#3496 (comment)

@vbfox
Copy link
Contributor

vbfox commented Aug 13, 2017

I mentioned it here #17021 (comment) but forgot to follow or PR.

vbfox added a commit to vbfox/DefinitelyTyped that referenced this issue Aug 13, 2017
@gaspard
Copy link
Contributor

gaspard commented Sep 11, 2017

@vbox Using a type for this is a good idea. Please also add a test for any new elements in this return type (like false). :-)

@adrienharnay
Copy link

Hey, is anyone working on this? If not I could help finish this if needed :)

@mgol
Copy link
Contributor

mgol commented Dec 5, 2018

Currently, the class Component type specifies its render method type as:

render(): ReactNode;

Shouldn't FunctionComponent type be changed from:

(props: P & { children?: ReactNode }, context?: any): ReactElement<any> | null;

to:

(props: P & { children?: ReactNode }, context?: any): ReactNode;

as well? As far as I understand both return types should be identical. Current types make it hard to refactor a class component as a function one as then suddently the return types change.

@Hotell
Copy link
Contributor

Hotell commented Dec 11, 2018

@Kovensky did this PR AFAIR , any reasons why FC returns ReactElement | null instead of ReactNode

I guess it might be related that ReactNode contains {} and undefined which makes compoennt returns type checking hard https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L171

@Jessidhia
Copy link
Member

Jessidhia commented Dec 11, 2018

That is because the compiler is still dumb in this respect and thinks function components have to return JSX.Element | null.

function Test() { return null }
function Test2() { return false }

const jsx = (
  <>
    <Test />
    <Test2
      // type error
      // "JSX element type 'boolean' is not a constructor function for JSX elements."
      // (looks like the error message the bug causes is itself buggy???)
    />
  </>
)

This is unrelated to the type of FunctionComponent, it's "knowledge" hardcoded into the compiler, and correcting the type in FunctionComponent would actually make it impossible to use things typed or union-ed with FunctionComponent as JSX tags.

image

This also extends to returning string, number, or ReadonlyArray<React.ReactElement<any> | string | number | boolean | null | undefined>, all valid return types in React 16.

(Note that React.ReactNode is not a valid return type, btw)


The other problem I wanted to fix is that class components' render()s are currently allowed to return undefined but that's a runtime error. They're also allowed to return {}, functions, and even Symbol(); could be a fun game to try to figure out if they crash at runtime or if React throws a proper warning. They're also allowed to not specify a render() at all and that's also a runtime error; class Component should be made abstract.

To fix the root cause of that problem I need that big 600+ files PR that takes too long to build for Travis (50min and it's not even 20% done). I'm just leaning more and more towards making a hard breaking change in the React types, rewriting and throwing everything legacy away. I wonder if typesVersions would provide a way to do that without breaking DefinitelyTyped itself -- dependent types could be updated on demand, on likely much smaller PRs. Right now the react types are just getting more and more ossificated.

@adrienharnay
Copy link

Hey, any news on this?

@eps1lon
Copy link
Collaborator

eps1lon commented Feb 1, 2019

This can be fixed once microsoft/TypeScript#21699 is resolved.

@TomasHubelbauer
Copy link
Contributor

TomasHubelbauer commented Mar 19, 2019

This seems to be the reason for why React.FunctionComponent (and React.SFC) cannot return ReactNode (because it contains false and null and undefined), but could they return ReactChild instead of ReactElement?

Right now this is an error:

const test: React.FunctionComponent<{}> = props => 'test';

I would like to propose that FunctionComponent returns ReachChild instead, but I am not sure whether that's the way to go. Can anyone advise?

@AviVahl
Copy link
Contributor

AviVahl commented Apr 2, 2019

Almost opened another duplicate... () => 'Hello World' should be a valid FunctionComponent. :/

@rodoabad
Copy link

rodoabad commented Dec 7, 2020

What can we do to move forward with this?

@rodoabad
Copy link

It's 2021 and this is still an open issue, is there anything we can do to help move this forward?

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 25, 2021

This can be fixed once microsoft/TypeScript#21699 is resolved.

This is still the latest state of this issue. We can't do anything unless TypeScript folks move forward with microsoft/TypeScript#21699

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 5, 2021

Duplicate of #18051

@ryami333
Copy link

ryami333 commented Feb 7, 2022

Can anyone explain to my why this is necessarily blocked by the mentioned Typescript issue when this is replicable without using JSX? This, for example, creates a type-error, because FunctionComponent is not permissive enough to allow functional components to return numbers, booleans or undefined.

const ComponentWhichReturnsOne = () => 1;

React.createElement(ComponentWhichReturnsOne);

@eps1lon
Copy link
Collaborator

eps1lon commented Feb 7, 2022

@ryami333 By allowing it in createElement but not JSX we would cause even more confusion. Especially since JSX is the common way to create elements as opposed to manually calling JSX factories.

@ryami333
Copy link

ryami333 commented Feb 7, 2022

Right, so it's impossible in JSX because of upstream constraints, but it's only #wontfix in vanilla because equality+solidarity?

@eps1lon
Copy link
Collaborator

eps1lon commented Feb 8, 2022

This was just the first argument that come to mind. We already have inconsistencies between JSX and no-JSX so it may not be as relevant.

Feel free to go through the typings for createElement to see if a fix makes sense. We would probably end up with another overload that accepts something like "ActualFunctionComponent" that allows returning everything that's allowed at runtime. In other words, we would have to add types that we really shouldn't need but now have to maintain for an edge case. That doesn't sound like the cost-benefit ratio is great.

@bottd
Copy link

bottd commented May 19, 2022

Commenting to point out that I've been running into this with my team somewhat often. It'd be great to get the type corrected at some point, the common use case of returning false for me is allowing returns of short circuited components.

return condition && <div>My markup</div>

The current types force this code to be rewritten as a ternary to return null instead of false. I haven't contributed to DefinitelyTyped before, but I may try taking a look into this. It doesn't seem too promising considering this issue has been open for quite a while though.

@eps1lon
Copy link
Collaborator

eps1lon commented May 19, 2022

It'd be great to get the type corrected at some point

This is still blocked by microsoft/TypeScript#21699. I recommend subscribing to microsoft/TypeScript#21699 instead of asking for updates.

@kevinbarabash
Copy link
Contributor

I modified @types/react/index.d.ts locally replacing

interface Element extends React.ReactElement<any, any> { }

with

type Element = React.ReactNode;

inside of the JSX namespace.

The following code type checks for me:

let Foo = () => "hello, world";
let foo = <Foo />;

Is this a bad solution? If so, why? I don't see JSX.Element used anywhere in index.d.ts.

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 20, 2023

JSX.Element is used by TypeScript itself. You essentially told TypeScript that <div /> is a React.ReactNode.

@user72356
Copy link

Now that microsoft/TypeScript#21699 is fixed, should this issue and #60242 be fixed as well?

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 14, 2023

Still need to wait for a stable release but I prepare support for adding pre 5.1. and 5.1 support in @types/react and then I'm hopeful we can close since once 5.1 is out (which is scheduled for June 2023)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment