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

JS classes extending generic TS classes should not use specified default type parameters #17104

Closed
gagarski opened this issue Jul 11, 2017 · 16 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@gagarski
Copy link

gagarski commented Jul 11, 2017

See minified description and example at the comment below

Here is original description involving React and JSX



~~~#14558 and #14881 already addressed this issue, however the fixes work only if legacy component does not have any properties.~~~

~~~**Code**~~~

~~~tsconfig.json~~~

```json
/* See minifed example below */
{
  "compilerOptions": {
    "module": "es2015",
    "target": "esnext",
    "jsx": "react-native",
    "strictNullChecks": true,
    "allowSyntheticDefaultImports": true,
    "allowJs": true,
    "noErrorTruncation": true,
    "lib": [
      "es5",
      "es6",
      "es7",
      "es2017",
      "dom"
    ],
    "outDir": "dist"
  },
  "include": [
    "**/*.ts",
    "**/*.tsx",
    "**/*.js",
    "**/*.jsx"
  ],
  "exclude": [
    "node_modules"
  ]
}
```

~~~hello.jsx~~~

```jsx
/* See minifed example below */
import React from 'react';

export default class Hello extends React.Component {
  render() {
    return <div>{this.props.baz}</div>;
  }
}
```

~~~index.tsx~~~

```tsx
/* See minifed example below */
import React from 'react';
import Hello from './hello';

class A extends React.Component<void, void> {
  render() {
    return (
      <div>
        <Hello baz="world">
          <div></div>
        </Hello>
      </div>
    );
  }
}
```

~~~Repo with this example: https://github.com/gagarski/tsx-jsx~~~

~~~**Expected behavior:**~~~

~~~No problems~~~

~~~**Actual behavior:**~~~

~~~TS compiler thinks that `Hello` properties type is `{}`. Thus, `Hello` component cannot be created in tsx file using JSX syntax. We can work this around using explicit `React.createComponent(...)` with type assertion or temporary properties variable but this is not very nice solution.

```
index.tsx(8,16): error TS2339: Property 'baz' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Hello> & Readonly<{ children?: ReactNode; }> & Readonly<{}>'.
```
@gagarski
Copy link
Author

Another workaround is to use spread operator (see #16725). The bad thing that it is considered a bug.

@artemyarulin
Copy link

Can confirm with latest version of TS - works fine only if JS component has no props. Really sad issue and breaks whole migration process from JS to TS :(

I beleive it can be solved via https://www.typescriptlang.org/docs/handbook/jsx.html and specifying typings for comonents but would be much easier if all JS components could be imported as any. We have 400 JS components, so migration is not 1 step for us

@artemyarulin
Copy link

artemyarulin commented Jul 18, 2017

Just tried with latest '2.5.0-dev.20170712' and with master - same issue

@paul-sachs
Copy link

Running into the exact same issue. Absolute blocker for converting our app to typescript. Can get around it with something like this:

import _VText from 'components/input/textComponent'; const VText = _VText as any;

But obviously hacky and prevents good linting practice. Any movement on this?

@paul-sachs
Copy link

So I believe I understand the core of the problem. The default generics for React Component specify propShape as {}, which is what typescript assumes to be used as the default props for any jsx component. Thus, typescript validates the components, thinking that they shouldn't have any props. You can get around this by creating your own base react class:

// baseClass.ts
import React, {Component} from 'react';

export default class ReactComponent<P = any, S = any> extends Component<P, S> {

}

Your jsx components will need to extend this class instead. Not ideal.

Another solution I haven't investigated is to see if there is a way to override the generics of @types/react/index.d.ts:

interface Component<P = {}, S = {}> extends ComponentLifecycle<P, S> { }

to this:

interface Component<P = any, S = any> extends ComponentLifecycle<P, S> { }

unfortunately, you can't just override these definitions with declaration merging.

@gagarski
Copy link
Author

@psachs21

Yeah, seems like the underlying reason is that Typescript respects default generic types when processing JS classes extending TS generic classes.

So, here is more minimalistic example without React and JSX

generic.ts

export default class Generic<T> {
    f(x: T)  {
        return x;
    }
}

genericDef.ts

export default class GenericDef<T = {}>{
    f(x: T)  {
        return x;
    }
}

hello.js

import Generic from "./generic";

export class Hello extends Generic {}

helloDef.js

import GenericDef from "./genericDef";

export class HelloDef extends GenericDef {}

index.ts

import {Hello} from './hello';
import {HelloDef} from './helloDef';
import Generic from "./generic";
import GenericDef from "./genericDef";

let x: Generic<{x: number}> = new Hello();
// ^^ OK
let y: GenericDef<{x: number}> = new HelloDef();
// ^^
// index.ts(7,5): error TS2322: Type 'HelloDef' is not assignable to type 'GenericDef<{ x: number; }>'.
//     Types of property 'f' are incompatible.
//     Type '(x: {}) => {}' is not assignable to type '(x: { x: number; }) => { x: number; }'.
//     Type '{}' is not assignable to type '{ x: number; }'.
//     Property 'x' is missing in type '{}'.

tsconfig.json

{
  "compilerOptions": {
    "module": "es2015",
    "target": "esnext",
    "strictNullChecks": true,
    "allowSyntheticDefaultImports": true,
    "allowJs": true,
    "lib": [
      "es5",
      "es6",
      "es7",
      "es2017",
      "dom"
    ],
    "outDir": "dist"
  },
  "include": [
    "**/*.ts",
    "**/*.js"
  ],
  "exclude": [
    "node_modules"
  ]
}

A repo with this example: https://github.com/gagarski/ts-js

@gagarski gagarski changed the title TS2339 error when importing using class components with properties from legacy jsx files JS classes extending generic TS classes use specified default parameters Aug 18, 2017
@gagarski gagarski changed the title JS classes extending generic TS classes use specified default parameters JS classes extending generic TS classes use specified default type parameters Aug 19, 2017
@gagarski gagarski changed the title JS classes extending generic TS classes use specified default type parameters JS classes extending generic TS classes should not use specified default type parameters Aug 19, 2017
@billti
Copy link
Member

billti commented Aug 19, 2017

Does annotating your JavaScript class to specify the generic arguments help (e.g. #15251 (comment) )?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 21, 2017

In a .js file does there is no mean to specify generic type arguments, that is why the compiler tries to figure them out as much as it can, and does not report errors for them. use @augments to override the generic type argument.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 21, 2017
@gagarski
Copy link
Author

@billti @mhegazy
Yes, @augments helps and for the initial use-case (migrating from JS to TS) it's much easier to add @augments React.Component<any, any> to each JS file instead of rewriting the whole project to Typescript in one day.

However, default generic parameters do not seem a good source of truth for generic parameters of JS classes. Generic parameters in JS are omitted exactly because there is not mean to specify them, not because JS developer intended to rely on TS default generic parameters (he probably thought something like "Typescript? What is Typescript?" when he was writing this code). In this case the most we can rely on is any, as if there were no default generic parameters.

If it is intended behaviour and the guess about default generic parameters is right in most cases, then we probably should close this issue.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 21, 2017

The default comes from the author of the declaration file. the default can be any. the compiler does not have a better source of information here. there is an optional type parameter, it has a default, the .js file did not specify it, then it uses that.

@gagarski
Copy link
Author

I still think that it's better to consider generic parameters information lost when the typed class goes through a typeless JS gap than to make inaccurate guess based on default argument, but maybe I totally misunderstand the whole purpose of default generic parameters :)

Also React typings seem correct for me (if you don't specify state type, you probably don't want state at all).

@gagarski
Copy link
Author

@psachs21 probably the @augments solution is much cleaner than hacking base classes in all of your JS files.

@paul-sachs
Copy link

paul-sachs commented Aug 21, 2017 via email

@ekoneko
Copy link

ekoneko commented Aug 25, 2017

This code is also working:

const helloProps = { baz: "world" }
<Hello {...helloProps} />

However at least it doesn't need to edit jsx files

@mhegazy
Copy link
Contributor

mhegazy commented Sep 8, 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 Sep 8, 2017
@jcreamer898
Copy link

I think I'm having a similar issue...

/**
 * @type {Reducer<IPoiState, any>}
 */
const poi = handleActions({
  [ADD_BOOKMARK_START]: (state) => {
    return { ...state };
  },
}, {});

The handleActions type comes from Definitey Typed and looks like...

export function handleActions<State>(
    reducerMap: ReducerMap<State>,
    initialState: State
): Reducer<State, any>;

So, I'm getting an error saying that...

Type 'Reducer<{}, any>' is not assignable to type 'Reducer<IPoiState, any>'.
  Type '{}' is not assignable to type 'IPoiState'.

Is there anyway to force the generic through?

@mhegazy

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

7 participants