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.

Sign up for GitHub

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

RFC: Consult new JSX.ElementType for valid JSX element types #51328

Merged
merged 21 commits into from
Apr 14, 2023
Merged

RFC: Consult new JSX.ElementType for valid JSX element types #51328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
26e25ae
jsx: Consult new JSX.ElementType for valid JSX element types
eps1lon Oct 27, 2022
71a0843
Stricter impl of `JSX.ElementType`
eps1lon Oct 27, 2022
ca648a8
Update baseline
Feb 14, 2023
8bda707
Remove props type param
Feb 14, 2023
8a3307e
(failing) test for host components
Feb 14, 2023
511180b
f
Feb 14, 2023
c69d4de
Add support for host components
Feb 14, 2023
4e39127
Include Promises in React18Node
eps1lon Apr 2, 2023
4a04633
Refine error message
eps1lon Apr 2, 2023
4b351fd
Merge branch 'main' into feat/jsx-element-type
eps1lon Apr 2, 2023
a4c8220
Fix up comment for linter.
DanielRosenwasser Apr 11, 2023
315e0b2
Sanity checks for render elements and class components
eps1lon Apr 13, 2023
af48970
higher-order component compat
eps1lon Apr 13, 2023
9adca32
Drop the `getApparentType`
eps1lon Apr 13, 2023
68a7335
my-web-component -> my-custom-element
eps1lon Apr 13, 2023
320f52f
Add test for unknown host elements
eps1lon Apr 13, 2023
79c22f9
Use literal type instead of abstract string
eps1lon Apr 13, 2023
14bd3fd
Unresolved element types are still rejected
eps1lon Apr 13, 2023
897c643
Merge branch 'main' into feat/jsx-element-type
Apr 13, 2023
b7a552e
Unescape tag name for literal type
eps1lon Apr 14, 2023
d02bc61
Add Preact inspired test
eps1lon Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30328,7 +30328,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
/**
* Returns true iff React would emit this tag name as a string rather than an identifier or qualified name
*/
function isJsxIntrinsicIdentifier(tagName: JsxTagNameExpression): boolean {
function isJsxIntrinsicIdentifier(tagName: JsxTagNameExpression): tagName is Identifier {
return tagName.kind === SyntaxKind.Identifier && isIntrinsicJsxName(tagName.escapedText);
}

Expand Down Expand Up @@ -30793,6 +30793,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}

function getJsxElementTypeTypeAt(location: Node): Type | undefined {
const type = getJsxType(JsxNames.ElementType, location);
if (isErrorType(type)) return undefined;
return type;
}

/**
* Returns all the properties of the Jsx.IntrinsicElements interface
*/
Expand Down Expand Up @@ -30861,7 +30867,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const jsxOpeningLikeNode = node ;
const sig = getResolvedSignature(jsxOpeningLikeNode);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this would only be for library authors providing a JSX factory of some sort, right? I don't have a strong opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it should be a literal type. The literal type will be assignable to string, and forwarding the literal type lets library authors filter the allowed types to only actually existing ones.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, in fact it has to be a literal if it's called ElementType type because Preact already declares ElementType

https://github.com/preactjs/preact/blob/90042a1a012855ef975309485534595705c777c5/src/jsx.d.ts#L34-L40

Which is why the following tests break in DefinitelyTyped.

#51328 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Also, given that ElementType is generic for them, wouldn't we have to instantiate with default arguments? Do you know if that's necessary @weswigham?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it as an abstract string since the checker later checked against known host element types in JSX.IntrinsicElements anyway and still kept the old error message (see #51328 (comment)).

So unknown host elements are still rejected and I think we still need JSX.IntrinsicElements anyway for props assignability.

I added a test to verify that unknown host element types are still rejected and then switched to getStringLiteralType(tagName.escapedText as string) which didn't change any behavior as far as I can tell.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can do tagName.escapedText as string because the escaping won't correctly match up in a case like:

namespace JSX {
    // ...
    type ElementType = "__";
}

// ...

function __() {
    return <div />
}

let x = <__></__>;

checkDeprecatedSignature(sig, node);
checkJsxReturnAssignableToAppropriateBound(getJsxReferenceKind(jsxOpeningLikeNode), getReturnTypeOfSignature(sig), jsxOpeningLikeNode);

DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
const elementTypeConstraint = getJsxElementTypeTypeAt(jsxOpeningLikeNode);
if (elementTypeConstraint !== undefined) {
const tagName = jsxOpeningLikeNode.tagName;
const tagType = isJsxIntrinsicIdentifier(tagName)
? getStringLiteralType(unescapeLeadingUnderscores(tagName.escapedText))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? getStringLiteralType(tagName.escapedText as string)
? unescapeLeadingUnderscores(tagName.escapedText)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI isIntrinsicJsxName is also not using unescapeLeadingUnderscores which is why I didn't use that originally. I don't know the difference so I just trust you that unescaping is the correct thing to do here.

I assume you meant

Suggested change
? getStringLiteralType(tagName.escapedText as string)
? getStringLiteralType(unescapeLeadingUnderscores(tagName.escapedText))

though?

: checkExpression(tagName);
checkTypeRelatedTo(tagType, elementTypeConstraint, assignableRelation, tagName, Diagnostics.Its_type_0_is_not_a_valid_JSX_element_type, () => {
const componentName = getTextOfNode(tagName);
return chainDiagnosticMessages(/*details*/ undefined, Diagnostics._0_cannot_be_used_as_a_JSX_component, componentName);
});
}
else {
checkJsxReturnAssignableToAppropriateBound(getJsxReferenceKind(jsxOpeningLikeNode), getReturnTypeOfSignature(sig), jsxOpeningLikeNode);
}
}
}

Expand Down Expand Up @@ -48800,6 +48820,7 @@ namespace JsxNames {
export const ElementAttributesPropertyNameContainer = "ElementAttributesProperty" as __String; // TODO: Deprecate and remove support
export const ElementChildrenAttributeNameContainer = "ElementChildrenAttribute" as __String;
export const Element = "Element" as __String;
export const ElementType = "ElementType" as __String;
export const IntrinsicAttributes = "IntrinsicAttributes" as __String;
export const IntrinsicClassAttributes = "IntrinsicClassAttributes" as __String;
export const LibraryManagedAttributes = "LibraryManagedAttributes" as __String;
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7760,5 +7760,9 @@
"Non-abstract class '{0}' does not implement all abstract members of '{1}'": {
"category": "Error",
"code": 18052
},
"Its type '{0}' is not a valid JSX element type.": {
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
"category": "Error",
"code": 18053
}
}
200 changes: 200 additions & 0 deletions tests/baselines/reference/jsxElementType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
tests/cases/compiler/jsxElementType.tsx(34,2): error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
tests/cases/compiler/jsxElementType.tsx(36,16): error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.
tests/cases/compiler/jsxElementType.tsx(40,2): error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
tests/cases/compiler/jsxElementType.tsx(42,15): error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.
tests/cases/compiler/jsxElementType.tsx(46,2): error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
tests/cases/compiler/jsxElementType.tsx(48,15): error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.
tests/cases/compiler/jsxElementType.tsx(52,2): error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
tests/cases/compiler/jsxElementType.tsx(54,14): error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.
tests/cases/compiler/jsxElementType.tsx(59,2): error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
tests/cases/compiler/jsxElementType.tsx(61,16): error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.
tests/cases/compiler/jsxElementType.tsx(70,2): error TS2769: No overload matches this call.
Overload 1 of 2, '(props: Readonly<{ title: string; }>): RenderStringClass', gave the following error.
Property 'title' is missing in type '{}' but required in type 'Readonly<{ title: string; }>'.
Overload 2 of 2, '(props: { title: string; }, context?: any): RenderStringClass', gave the following error.
Property 'title' is missing in type '{}' but required in type 'Readonly<{ title: string; }>'.
tests/cases/compiler/jsxElementType.tsx(72,20): error TS2769: No overload matches this call.
Overload 1 of 2, '(props: Readonly<{ title: string; }>): RenderStringClass', gave the following error.
Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
Overload 2 of 2, '(props: { title: string; }, context?: any): RenderStringClass', gave the following error.
Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
Property 'excessProp' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
tests/cases/compiler/jsxElementType.tsx(78,1): error TS2339: Property 'boop' does not exist on type 'JSX.IntrinsicElements'.
tests/cases/compiler/jsxElementType.tsx(79,1): error TS2339: Property 'my-undeclared-custom-element' does not exist on type 'JSX.IntrinsicElements'.
tests/cases/compiler/jsxElementType.tsx(91,2): error TS2786: 'ReactNativeFlatList' cannot be used as a JSX component.
Its type '(props: {}, ref: ForwardedRef<typeof ReactNativeFlatList>) => null' is not a valid JSX element type.
Type '(props: {}, ref: ForwardedRef<typeof ReactNativeFlatList>) => null' is not assignable to type '(props: any) => React18ReactNode'.
Target signature provides too few arguments. Expected 2 or more, but got 1.
tests/cases/compiler/jsxElementType.tsx(95,11): error TS2322: Type '{}' is not assignable to type 'LibraryManagedAttributes<T, {}>'.
tests/cases/compiler/jsxElementType.tsx(98,2): error TS2304: Cannot find name 'Unresolved'.
tests/cases/compiler/jsxElementType.tsx(99,2): error TS2304: Cannot find name 'Unresolved'.


==== tests/cases/compiler/jsxElementType.tsx (18 errors) ====
/// <reference path="/.lib/react16.d.ts" />
import * as React from "react";

type React18ReactFragment = ReadonlyArray<React18ReactNode>;
type React18ReactNode =
| React.ReactElement<any>
| string
| number
| React18ReactFragment
| React.ReactPortal
| boolean
| null
| undefined
| Promise<React18ReactNode>;

// // React.JSXElementConstructor but it now can return React nodes from function components.
type NewReactJSXElementConstructor<P> =
| ((props: P) => React18ReactNode)
| (new (props: P) => React.Component<P, any>);

declare global {
namespace JSX {
type ElementType = string | NewReactJSXElementConstructor<any>;
interface IntrinsicElements {
['my-custom-element']: React.DOMAttributes<unknown>;
}
}
}

let Component: NewReactJSXElementConstructor<{ title: string }>;

const RenderElement = ({ title }: { title: string }) => <div>{title}</div>;
Component = RenderElement;
<RenderElement />;
~~~~~~~~~~~~~
!!! error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:32:37: 'title' is declared here.
<RenderElement title="react" />;
<RenderElement excessProp />;
~~~~~~~~~~
!!! error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
!!! error TS2322: Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.

const RenderString = ({ title }: { title: string }) => title;
Component = RenderString;
<RenderString />;
~~~~~~~~~~~~
!!! error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:38:36: 'title' is declared here.
<RenderString title="react" />;
<RenderString excessProp />;
~~~~~~~~~~
!!! error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
!!! error TS2322: Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.

const RenderNumber = ({ title }: { title: string }) => title.length;
Component = RenderNumber;
<RenderNumber />;
~~~~~~~~~~~~
!!! error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:44:36: 'title' is declared here.
<RenderNumber title="react" />;
<RenderNumber excessProp />;
~~~~~~~~~~
!!! error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
!!! error TS2322: Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.

const RenderArray = ({ title }: { title: string }) => [title];
Component = RenderArray;
<RenderArray />;
~~~~~~~~~~~
!!! error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:50:35: 'title' is declared here.
<RenderArray title="react" />;
<RenderArray excessProp />;
~~~~~~~~~~
!!! error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
!!! error TS2322: Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.

// React Server Component
const RenderPromise = async ({ title }: { title: string }) => "react";
Component = RenderPromise;
<RenderPromise />;
~~~~~~~~~~~~~
!!! error TS2741: Property 'title' is missing in type '{}' but required in type '{ title: string; }'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:57:43: 'title' is declared here.
<RenderPromise title="react" />;
<RenderPromise excessProp />;
~~~~~~~~~~
!!! error TS2322: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & { title: string; }'.
!!! error TS2322: Property 'excessProp' does not exist on type 'IntrinsicAttributes & { title: string; }'.

// Class components still work
class RenderStringClass extends React.Component<{ title: string }> {
render() {
return this.props.title;
}
}
Component = RenderStringClass;
<RenderStringClass />;
~~~~~~~~~~~~~~~~~
!!! error TS2769: No overload matches this call.
!!! error TS2769: Overload 1 of 2, '(props: Readonly<{ title: string; }>): RenderStringClass', gave the following error.
!!! error TS2769: Property 'title' is missing in type '{}' but required in type 'Readonly<{ title: string; }>'.
!!! error TS2769: Overload 2 of 2, '(props: { title: string; }, context?: any): RenderStringClass', gave the following error.
!!! error TS2769: Property 'title' is missing in type '{}' but required in type 'Readonly<{ title: string; }>'.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:64:51: 'title' is declared here.
!!! related TS2728 tests/cases/compiler/jsxElementType.tsx:64:51: 'title' is declared here.
<RenderStringClass title="react" />;
<RenderStringClass excessProp />;
~~~~~~~~~~
!!! error TS2769: No overload matches this call.
!!! error TS2769: Overload 1 of 2, '(props: Readonly<{ title: string; }>): RenderStringClass', gave the following error.
!!! error TS2769: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
!!! error TS2769: Property 'excessProp' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
!!! error TS2769: Overload 2 of 2, '(props: { title: string; }, context?: any): RenderStringClass', gave the following error.
!!! error TS2769: Type '{ excessProp: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.
!!! error TS2769: Property 'excessProp' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<RenderStringClass> & Readonly<{ children?: ReactNode; }> & Readonly<{ title: string; }>'.

// Host element types still work
<div />;
<my-custom-element />;
// Undeclared host element types are still rejected
<boop />;
~~~~~~~~
!!! error TS2339: Property 'boop' does not exist on type 'JSX.IntrinsicElements'.
<my-undeclared-custom-element />;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2339: Property 'my-undeclared-custom-element' does not exist on type 'JSX.IntrinsicElements'.

// Highlighting various ecosystem compat issues
// react-native-gesture-handler
// https://github.com/software-mansion/react-native-gesture-handler/blob/79017e5e7cc2e82e6467851f870920ff836ee04f/src/components/GestureComponents.tsx#L139-L146
interface ReactNativeFlatListProps<Item> {}
function ReactNativeFlatList(
props: {},
ref: React.ForwardedRef<typeof ReactNativeFlatList>
) {
return null;
}
<ReactNativeFlatList />;
~~~~~~~~~~~~~~~~~~~
!!! error TS2786: 'ReactNativeFlatList' cannot be used as a JSX component.
!!! error TS2786: Its type '(props: {}, ref: ForwardedRef<typeof ReactNativeFlatList>) => null' is not a valid JSX element type.
!!! error TS2786: Type '(props: {}, ref: ForwardedRef<typeof ReactNativeFlatList>) => null' is not assignable to type '(props: any) => React18ReactNode'.
!!! error TS2786: Target signature provides too few arguments. Expected 2 or more, but got 1.

// testing higher-order component compat
function f1<T extends (props: {}) => React.ReactElement<any>>(Component: T) {
return <Component />;
~~~~~~~~~
!!! error TS2322: Type '{}' is not assignable to type 'LibraryManagedAttributes<T, {}>'.
}

<Unresolved />;
~~~~~~~~~~
!!! error TS2304: Cannot find name 'Unresolved'.
<Unresolved foo="abc" />;
~~~~~~~~~~
!!! error TS2304: Cannot find name 'Unresolved'.

Loading