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

Fixes #8657: Handles union typed React component. #8674

Merged
merged 7 commits into from
May 19, 2016

Conversation

evansb
Copy link
Contributor

@evansb evansb commented May 18, 2016

  • There is an associated issue that is labelled
    'Bug' or 'Accepting PRs' or is in the Community milestone
  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally
  • You've signed the CLA
  • There are new or updated unit tests validating the change

Fixes #8657

This pull request handles special case of union typed React component, namely type U = React.StatelessComponent<any> | React.ComponentClass<any>. Since StatelessComponent has call but no signature and ComponentClass has no call signature but no construct signature, the union between these two will result in a type with no call or construct signature.

Previously <U /> will fail because the type checker cannot find any call/construct signature on it, which is a false negative.

This patch handles union typed React component separately: we first unfold the type of U and apply the old construct/call signature + return type check.

This involves refactoring JSX Element type resolver to a new function getResolvedJsxType so that it can be applied recursively to each member of the union types.

Evan Sebastian.

@msftclas
Copy link

Hi @evansb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@evansb evansb changed the title Fixes #8657: Handles combination both stateless and stateful React component. (WIP) Fixes #8657: Handles combination both stateless and stateful React component. May 18, 2016
@msftclas
Copy link

@evansb, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@RyanCavanaugh
Copy link
Member

This doesn't seem like the right approach. What if you had a property of type string | ComponentClass ? It shouldn't be legal to use that as a React component.

@evansb
Copy link
Contributor Author

evansb commented May 18, 2016

You are right, I should've raised the error as soon as a type doesn't have constructor/call signature instead.

Anyway this is still pretty much WIP and doesn't solve the issue quite yet, we will have another problem with the different return type of StatelessComponent call and ComponentClass constructor.

At the moment StatelessComponent return type is React.Element. It isn't related with the current ElementClass, which fails to type check when you try to render it with JSX tags.

@evansb evansb changed the title (WIP) Fixes #8657: Handles combination both stateless and stateful React component. (WIP) Fixes #8657: Handles union typed React component. May 19, 2016
@evansb
Copy link
Contributor Author

evansb commented May 19, 2016

I think I'm done, would you mind reviewing it? @RyanCavanaugh

See description for patch summary

@evansb evansb changed the title (WIP) Fixes #8657: Handles union typed React component. Fixes #8657: Handles union typed React component. May 19, 2016
@RyanCavanaugh
Copy link
Member

Can you split out the test so there's a non-erroring test that shows the behavior you're trying to enable? When there's an error we don't generate a type baseline so it's harder to confirm the right thing is happening.

@RyanCavanaugh
Copy link
Member

👍

@mhegazy mhegazy merged commit 43b36d2 into microsoft:master May 19, 2016
@mhegazy
Copy link
Contributor

mhegazy commented May 19, 2016

thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSX: Typing both stateless component and component class in react component property gives error
4 participants