Skip to content

Improve error message for invalid return type of JSX component #32702

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

Merged
merged 12 commits into from
Mar 30, 2020

Conversation

uhyo
Copy link
Contributor

@uhyo uhyo commented Aug 4, 2019

Fixes #30850.

This PR improves the error messages shown when the return/instance type of the function/class used as a JSX component is not assignable to JSX.Element or JSX.ElementClass.

I thought the problem is not specific to void; referring to constructor function is always misleading.
So this PR fully replaces the current message with new ones.
The new error messages refer to return type, instance type or element type depending on what call signatures the component has.

Also this PR changes the span of the error from whole JSX element to only its tag name so the actual cause is clearer.

Example

source code:

namespace JSX {
  export interface Element {
    type: 'element';
  }
  export interface ElementClass {
    type: 'element-class';
  }
}

const FunctionComponent = () => ({
  type: 'string',
});

class ClassComponent {
  foo = "bar"
}

const MixedComponent = Math.random() ? FunctionComponent : ClassComponent;

const elem1 = <FunctionComponent />;
const elem2 = <ClassComponent />;
const elem3 = <MixedComponent />;

current behavior (TS3.5.3):

test.tsx:20:15 - error TS2605: JSX element type '{ type: string; }' is not a constructor function for JSX elements.
  Type '{ type: string; }' is missing the following properties from type 'Element': props, key

20 const elem1 = <FunctionComponent />;
                 ~~~~~~~~~~~~~~~~~~~~~

test.tsx:21:15 - error TS2605: JSX element type 'ClassComponent' is not a constructor function for JSX elements.
  Type 'ClassComponent' is missing the following properties from type 'ElementClass': type, render, context, setState, and 4 more.

21 const elem2 = <ClassComponent />;
                 ~~~~~~~~~~~~~~~~~~

test.tsx:22:15 - error TS2605: JSX element type 'ClassComponent | { type: string; }' is not a constructor function for JSX elements.
  Type 'ClassComponent' is not assignable to type 'Element | ElementClass'.
    Type 'ClassComponent' is not assignable to type 'ElementClass'.

22 const elem3 = <MixedComponent />;
                 ~~~~~~~~~~~~~~~~~~


Found 3 errors.

new behavior:

index.tsx:20:16 - error TS2774: 'FunctionComponent' cannot be used as a JSX component.
  Its return type '{ type: string; }' is not a valid JSX element.
    Types of property 'type' are incompatible.
      Type 'string' is not assignable to type '"element"'.

20 const elem1 = <FunctionComponent />;
                  ~~~~~~~~~~~~~~~~~

index.tsx:21:16 - error TS2774: 'ClassComponent' cannot be used as a JSX component.
  Its instance type 'ClassComponent' is not a valid JSX element.
    Property 'type' is missing in type 'ClassComponent' but required in type 'ElementClass'.

21 const elem2 = <ClassComponent />;
                  ~~~~~~~~~~~~~~

  index.tsx:6:5
    6     type: 'element-class';
          ~~~~
    'type' is declared here.

index.tsx:22:16 - error TS2774: 'MixedComponent' cannot be used as a JSX component.
  Its element type 'ClassComponent | { type: string; }' is not a valid JSX element.
    Type 'ClassComponent' is not assignable to type 'Element | ElementClass | null'.
      Type 'ClassComponent' is not assignable to type 'ElementClass'.

22 const elem3 = <MixedComponent />;
                  ~~~~~~~~~~~~~~


Found 3 errors.

Alternative Option

I chose to separate the new error messages to two parts: one is 'ComponentName' cannot be used as a JSX component.and the other isIts return type '{0}' is not a valid JSX element.or similar. Another option is to merge them into one so the new message is'ComponentName' cannot be used as a JSX component because its return type '{0}' is not a valid JSX element.`

I chose the first option for two reasons. One is that in this way we get the same top-level error message for all the three cases.
The other reason is that the merged message, especially the part before '{0}', is too long. The cause of the error (the return type {0} in this case) should be reachable as easily as possible. This is achieved by chaining two messages as done in this PR.

@uhyo uhyo force-pushed the issue-30850-jsx-return-void branch from fecfd5a to 64b5bed Compare August 15, 2019 07:41
@@ -1,9 +1,11 @@
tests/cases/conformance/jsx/inline/index.tsx(5,1): error TS2741: Property '__predomBrand' is missing in type 'import("tests/cases/conformance/jsx/inline/renderer").dom.JSX.Element' but required in type 'import("tests/cases/conformance/jsx/inline/renderer2").predom.JSX.Element'.
tests/cases/conformance/jsx/inline/index.tsx(21,40): error TS2322: Type 'import("tests/cases/conformance/jsx/inline/renderer").dom.JSX.Element' is not assignable to type 'import("tests/cases/conformance/jsx/inline/renderer2").predom.JSX.Element'.
tests/cases/conformance/jsx/inline/index.tsx(21,40): error TS2605: JSX element type 'MyClass' is not a constructor function for JSX elements.
Property '__domBrand' is missing in type 'MyClass' but required in type 'ElementClass'.
tests/cases/conformance/jsx/inline/index.tsx(21,41): error TS2774: This expression cannot be used as a JSX component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go back to naming the expression? For example, 'MyClass' cannot be used as a JSX component.?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah,

The original suggestion in #30850 suggests to include {component name} in the error message, but I found it hard to stringify JSX tag expressions for use by error messages. Instead I used This expression.

Maybe another reviewer can help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRosenwasser Sorry, my investigation wasn't enough. I found a utility function which can do it.
I updated the PR to include component name in error messages.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@uhyo uhyo force-pushed the issue-30850-jsx-return-void branch from e50046c to 7cf4d9a Compare February 18, 2020 15:05
@uhyo uhyo force-pushed the issue-30850-jsx-return-void branch from 7cf4d9a to 2957ec7 Compare February 18, 2020 15:15
@weswigham weswigham merged commit 1f56ab0 into microsoft:master Mar 30, 2020
@uhyo uhyo deleted the issue-30850-jsx-return-void branch March 31, 2020 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Better error message for void-returning functions used as JSX elements
4 participants