Skip to content

react.d.ts: Change union type to intersection type#6926

Closed
Strate wants to merge 2 commits intoDefinitelyTyped:masterfrom
Strate:react_v014_react_instance_fix
Closed

react.d.ts: Change union type to intersection type#6926
Strate wants to merge 2 commits intoDefinitelyTyped:masterfrom
Strate:react_v014_react_instance_fix

Conversation

@Strate
Copy link
Contributor

@Strate Strate commented Nov 24, 2015

type ReactInstance = Component<any, any> | Element is wrong. Trying to accessing any Element's property gives me a compilation error:

this.refs['refname'].scrollHeight // error TS2339: Property 'scrollHeight' does not exist on type 'Component<any, any> | Element'.

This happening because of typescript union type spec:

The type A|B has a property P of type X|Y if A has a property P of type X and B has a property P of type Y.
(from Properties section)

Instead of using union type, there should be intersection type, which completely fits to this case:

The type A & B has a property P if A has a property P or B has a property P.
(from Properties section)

So, there should be type ReactInstance = Component<any, any> & Element

See also #6205

@dt-bot
Copy link
Member

dt-bot commented Nov 24, 2015

react/react.d.ts

to authors (Asana (account can't be detected) AssureSign (account can't be detected) Microsoft (account can't be detected)). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@johnnyreilly
Copy link
Member

Please could you fix the tests?

@jbrantly @vsiao @pspeter3 could you take a look when done? Cheers.

@Strate
Copy link
Contributor Author

Strate commented Nov 24, 2015

I fixed the tests, and I reverted change of ReactInstance type in favor of change to type of Component.refs element. I think I should write test for that, but I can not find out how to do that. Could anyone to help me with test?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong; it should not be an intersection type; union was the right type. you have to narrow the type from Component<any, any> | Element to just Element to get your code to work

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. This is definitely a type union

@adidahiya
Copy link
Contributor

👎

@johnnyreilly
Copy link
Member

Closed based on feedback

@Strate
Copy link
Contributor Author

Strate commented Nov 24, 2015

ahh, I got it. It really can be either component or element, for base react elements, such as div element. Sorry for disturb, guys.

@johnnyreilly
Copy link
Member

No problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants