Skip to content

TSC returns an error for TSX code where the plain TS equivalent compiles fine #14036

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

Closed
avonwyss opened this issue Feb 13, 2017 · 15 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@avonwyss
Copy link

TypeScript Version: 2.1.5

Code

type SelectProps<T> = { items: T[] }
class Select<T> extends React.Component<SelectProps<T>, any> { }

const someItems: [1, 2, 3, 4];

function renderJsx() {
return <Select items={someItems} />; // Error: Generic type 'Select<T>' requires 1 type argument(s).
}

function renderJs() {
return React.createElement(Select, { items: someItems }); // No error
}

Expected behavior:
TS should infer the generic type in the renderJsx function just as it does in the renderJs function.

Actual behavior:
TS requires a generic type argument which unfortunately cannot be specified because this is not supported by the parser.

See also #3960 for some related discussions.

@basarat
Copy link
Contributor

basarat commented Feb 15, 2017

Probably worth raising here : https://github.com/facebook/jsx/issues/new for some explicit syntax in JSX 🌹

@avonwyss
Copy link
Author

@basarat I'm not sure what you are suggesting; it has nothing to do with the original JSX syntax IMHO.
The type inference is related to TypeScript only, and I do think that it would solve most (if not all) JSX generic component use cases without requiring any syntax enhancement (or kludges as discussed in #3960).

@basarat
Copy link
Contributor

basarat commented Feb 15, 2017

I understand 🌹

@MastroLindus
Copy link

+1 for this or at least #6395

@ksmutny
Copy link

ksmutny commented Mar 13, 2017

+1 Yes please, type inference would be IMHO the correct (and welcomed) solution.

@Pajn
Copy link

Pajn commented Apr 1, 2017

It would be great if TypeScript at least could infer generics even though you can't write them explicitly.
See this:

export function AsyncList<D extends {loading: boolean}>(props: {data: D, getItems: (data: D) => Array<any>}) {
  return (
    <div>
    </div>
  )
}

const data = {loading: false, items: [1, 2, 3]}

function a() {
  return <AsyncList
    data={data}
    getItems={data => data.items}
  />
}
function b() {
  return AsyncList({
    data,
    getItems: data => data.items
  })
}

a fails with

Type '{ loading: boolean; items: number[]; }' is not assignable to type 'D'.
Property 'items' does not exist on type 'D'.

but b have no issues.
I think TS should be able to infer D as typeof data in the JSX case just as it does int the call/object literal case.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2017

There is not really a place for inference to happen here. there is no reason to assume that the T in Select<T> is the propTypes, unless we hard-code the React component handling, which is something we would not want to do.

The best option here is #6395, where you can explicitly specify your type arguments.

@mhegazy mhegazy added the Duplicate An existing issue was already created label Apr 27, 2017
@avonwyss
Copy link
Author

avonwyss commented Apr 28, 2017

@mhegazy Maybe "type inference" is the wrong terminology. Nevertheless, since the renderJs is the code equivalent to what should be generated by the compiler for the renderJsx case, I don't get why the first is acceptable without type argument while the JSX case is not. Basically, since they are equivalent AFAIK they should also both be accepted the same by the compiler.

Basically, since we do have the JSX mode and the JSX factory settings, the compiler should IMHO generate the same AST in both cases, and whenever the React typings are available also react (no pun intended) the same - no hardcoding needed.

@mhegazy
Copy link
Contributor

mhegazy commented May 8, 2017

@mhegazy Maybe "type inference" is the wrong terminology. Nevertheless, since the renderJs is the code equivalent to what should be generated by the compiler for the renderJsx case, I don't get why the first is acceptable without type argument while the JSX case is not. Basically, since they are equivalent AFAIK they should also both be accepted the same by the compiler.

There is no inference happening in the second one either (renderJsx), the type parameters both go to any. you just not getting an error. you can get the same behavior by not having the component be generic, and setting T to any.

@avonwyss
Copy link
Author

avonwyss commented May 8, 2017

@mhegazy OK, even if any is used for T, it still does check that the properties of the props are as required by the component. See Playground.

type SelectProps<T> = { items: T[] }
class Select<T> extends React.Component<SelectProps<T>, any> { }

const someItems = [1, 2, 3, 4];

function renderJsOk() {
    return React.createElement(Select, { items: someItems }); // No error
}

function renderJsFail() {
    return React.createElement(Select, { itemz: someItems }); // Error
}

The question I'm asking really is this: why does the plain and the JSX version not behave the same, e.g. why is there a compile error in the JSX case? If the JSX case was acting the same as the plain version I'd be pretty happy already, as it would allow people to use JSX syntax throughout and achieve the same level of type safety as with the plain version, e.g. checking that the passed in properties are valid (even if it is using any for T).

Note that this is only equivalent to making the component non-generic on the call site. Being generic makes sure that the code of the component itself will not be littered with any in places where one specific type is expected, and thus make the component code much more typesafe.

@avonwyss avonwyss changed the title Type inference when using JSX/TSX syntax TSC does returns an error for JSX code where the plain TS equivalent compiles fine May 8, 2017
@avonwyss avonwyss changed the title TSC does returns an error for JSX code where the plain TS equivalent compiles fine TSC returns an error for JSX code where the plain TS equivalent compiles fine May 8, 2017
@avonwyss avonwyss changed the title TSC returns an error for JSX code where the plain TS equivalent compiles fine TSC returns an error for TSX code where the plain TS equivalent compiles fine May 8, 2017
@mhegazy
Copy link
Contributor

mhegazy commented May 8, 2017

with #15455, you can get the same bahvuour with default paramters:

import * as React from "react";

type SelectProps<T > = { items: T[] }
class Select<T = any> extends React.Component<SelectProps<T>, any> { }

const someItems = [1, 2, 3, 4];
const someItems2 = ["ss"];

function renderJsx() {
    return <Select items={someItems} />; // OK
}

@avonwyss
Copy link
Author

avonwyss commented May 9, 2017

@mhegazy Thank you for the hint, I'll use that when it becomes available in the stable version.

Nevertheless it remains unclear to me why the plain and the JSX-style code do not behave the same, could you maybe shed some light on that?

@yuit
Copy link
Contributor

yuit commented May 9, 2017

@avonwyss The reason is on how we resolve JSX.Expression. The way we resolve React.Component in JSX-style syntax is different than how we do for normal function in particular it doesn't go through overload resolution logic (like how new call expression does).

In 2.3, however, stateless-function component does go through overload resolution and so if you declare Select as SFC and using 2.3 we will be able to infer type parameter

function Select<T>(attr: SelectProps<T>) {...}

We are considering using overload resolution when we are trying to resolve JSX expression that is declared as React Component class.

@avonwyss
Copy link
Author

avonwyss commented May 9, 2017

Thank you @yuit for explaining the difference. Indeed, I think that using the same overload resolution would be the correct way to do things, so that behavior remains consistent (even more so in the light of using other JSX factories).

@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed May 23, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

7 participants