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

Handling Props with react and spread attributes #4682

Closed
kuon opened this issue Sep 7, 2015 · 13 comments
Closed

Handling Props with react and spread attributes #4682

kuon opened this issue Sep 7, 2015 · 13 comments
Labels
Fixed A PR has been merged for this issue

Comments

@kuon
Copy link

kuon commented Sep 7, 2015

Given the following parent component:

import React = require('react');
import ChildView = require('./child');

interface Props extends React.Props<View> {
  foo: string;
}

interface State {
}

class View extends React.Component<Props, State> {

    private removeItem = (evt: React.DragEvent) => {
    }

    render() {
        return <ChildView {...this.props} />;
    }
}

export = View;

And the following child component:

import React = require('react');

interface Props extends React.Props<View> {
  foo: string;
}

interface State {
}

class View extends React.Component<Props, State> {

    private removeItem = (evt: React.MouseEvent) => {
    }

    render() {
        return <div>{this.props.foo}</div>;
    }
}

export = View;

I get an error like this:

Error TS2606: Property 'ref' of JSX spread attribute is not assignable to target property.
  Type 'string | ((component: View) => any)' is not assignable to type 'string | ((component: View) => any)'.
    Type '(component: View) => any' is not assignable to type 'string | ((component: View) => any)'.
      Type '(component: View) => any' is not assignable to type '(component: View) => any'.
        Types of parameters 'component' and 'component' are incompatible.
          Type 'View' is not assignable to type 'View'.
            Property 'removeItem' is missing in type 'View'.

This is caused by the ref attribute of the React.Props having different View (my classes)

Here is the interface defined:

    interface Props<T> {
        children?: ReactNode;
        key?: string | number;
        ref?: string | ((component: T) => any);
    }

I could declare props like interface Props extends React.Props<React.Component<any, any>> but I feel like something should be done to be able to "omit" certain attributes when using spread attributes.

Something like:

<div {...this.props omit React.Props<View>} />

Or handling key and ref specially in JSX and having them checked automatically, allowing the props definition to be just

interface Props {
   foo: string;
}

while allowing key and ref to be used implicitly..

@kuon kuon changed the title Handling Props with react and spread attributes Handling Props with react and spread attributes Sep 7, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Sep 8, 2015

i am not getting repro for this with latest react.d.ts from definitely typed.

here are my two files

/// <reference path="c:\\clones\\DefinitelyTyped\\react\\react.d.ts" />
import React = require("react");
import ChildView = require('child');

interface Props extends React.Props<View> {
    foo: string;
}

interface State {
}

class View extends React.Component<Props, State> {

    render() {
        return <ChildView {...this.props} />;
    }
}

export = View;

and

/// <reference path="c:\\clones\\DefinitelyTyped\\react\\react.d.ts" />
import React = require("react");

interface Props extends React.Props<View> {
    foo: string;
}

interface State {
}

class View extends React.Component<Props, State> {

    private removeItem = (evt: React.MouseEvent) => {
    }

    render() {
        return <div>{this.props.foo}</div>;
    }
}

export = View;

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Sep 8, 2015
@kuon
Copy link
Author

kuon commented Sep 8, 2015

Haa, I messed up my example, sorry, the two classes must be incompatible in some way. I edited my example: I added an incompatible removeItem method in the parent view.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 17, 2015

sorry for the delay. but this seems to make sense to me, the declaration of ref requires the parameter to be of the same type. which it is not. i think this is an error for the typings, possibly ref?: string | ((component: React.Component<any,any>) => any); instead?

also pinging @RyanCavanaugh

@kuon
Copy link
Author

kuon commented Sep 18, 2015

For some reason I did not consider that the react.d.ts was wrong. I guess your fix is what makes the most sense.

@jbrantly
Copy link

jbrantly commented Oct 4, 2015

I'm not able to duplicate using the current files listed in the OP and the latest definitions from DT (and TS 1.6.2). I believe the DT definitions are correct here. The ref prop for a particular component returns an instance of that component and the typings are (mostly) set up to model that.

@kuon
Copy link
Author

kuon commented Oct 4, 2015

I still get the issue with TS 1.6.2 and latest (as of october 4) https://github.com/kuon/DefinitelyTyped/blob/master/react/react.d.ts

You must ensure the parent and child component don't have the same method signatures for the bug to manifest.

@jbrantly
Copy link

jbrantly commented Oct 5, 2015

No idea why I wasn't able to duplicate the first time, but I was able to this time.

I see the issue, but I think the typings are mostly correct for the time being and likely shouldn't be changed. Instead, for now, I'd suggest working around the issue by not extending from React.Props in your child component. You're not actually using ref there currently.

The issue is that in the React world there is a difference between the props that you pass in to createElement and the props that the component receives. Namely, the ref and key props that are passed in are not actually given to the instantiated component and are instead stripped off by createElement. However, in TypeScript land it's currently modeled that both the props that you pass in and the props that the component receives (this.props) share the same interface and that's causing this issue. Long term I'd like to change the definitions to correctly model this but there are some blockers (see #4362 and this comment).

@kuon
Copy link
Author

kuon commented Oct 5, 2015

Yes that's what I meant with my last paragraph (that key and ref should get a "special" handling).

I tried to change the definition but I soon find out it wasn't a good idea. My current workaround is, as you said, to no extend React.Props.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 19, 2016

fixed; it's no longer required to extend React.Props.

@mhegazy mhegazy closed this as completed Feb 19, 2016
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Needs More Info The issue still hasn't been fully clarified labels Feb 19, 2016
@dsifford
Copy link
Contributor

dsifford commented Apr 9, 2016

@mhegazy Can you please point me in the direction of where this is documented? Can't seem to find it.

@DanielRosenwasser
Copy link
Member

I think we changed the way things worked in #5478, where instead of users needing to extend Props or put in their own ref & key, we just picked this up from the JSX.IntrinsicAttributes and JSX.IntrinsicClassAttributes interfaces.

@DanielRosenwasser
Copy link
Member

@dsifford sorry that wasn't documented. I've filed microsoft/TypeScript-Handbook#237 on your behalf.

@dsifford
Copy link
Contributor

dsifford commented Apr 9, 2016

@DanielRosenwasser Thanks so much! 😄

cwalv referenced this issue in DefinitelyTyped/DefinitelyTyped Jun 28, 2016
@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
Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

5 participants