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

TSX and React Context API broke on nightly #27948

Closed
pocesar opened this issue Oct 17, 2018 · 15 comments
Closed

TSX and React Context API broke on nightly #27948

pocesar opened this issue Oct 17, 2018 · 15 comments
Assignees
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Domain: JSX/TSX Relates to the JSX parser and emitter Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@pocesar
Copy link

pocesar commented Oct 17, 2018

TypeScript Version: 3.2.0-dev.20181017

Search Terms: react, tsx, jsx, construct, call signatures.

Code

import React from 'React'

const Context = React.createContext<{}>({});

const El = <Context.Provider>{() => null}</Context.Provider>

Expected behavior:

Works as in 3.1.3

Actual behavior:

Giving JSX element type 'Context.Provider' does not have any construct or call signatures.

@pocesar pocesar changed the title TSX and React typings broke on nightly TSX and React Context API broke on nightly Oct 17, 2018
@asvetliakov
Copy link

Another example with same error:

import React from "react";
declare const Comp: React.SFC | React.ComponentClass;
<Comp />

@weswigham
Copy link
Member

Righto, with #27627 we removed the ability JSX tags had to attempt to resolve multiple call or construct signatures using the same expressions (and this was intentional because this is very unsafe, since we annotate types on those expressions during contextual checking). We need #7294 as a general solution to solve the problem for all kinds of calls (although JSX throws an extra wrench in that bucket by having methods of resolving both constructors and functions at the same time).

Right now a Provider (and a Consumer for that matter) claims to be a ComponentType - they should probably claim to one of either be a StatelessComponent or a ComponentClass (SFC is simpler and so would be my preference), not either or, honestly. Though in reality its neither.

@weswigham weswigham added Breaking Change Would introduce errors in existing code Working as Intended The behavior described is the intended behavior; this is not a bug labels Oct 17, 2018
@asvetliakov
Copy link

asvetliakov commented Oct 17, 2018

@weswigham

How to express HOC for both SFC and Component class now?

export function HOC(WrappedComponent: React.ComponentType) {
    // error
    <WrappedComponent />
}

declare function HOC1(WrappedComponent: React.SFC);
declare function HOC2(WrappedComponent: React.ComponentClass);


HOC1(A);
// NON-SFC are not accepted
HOC1(B);

// SFC are not accepted
HOC2(A)
HOC2(B);

Edit: Looks like adding overload signature works

@weswigham
Copy link
Member

import React from "react";
declare const Comp: React.SFC | React.ComponentClass;
<Comp />

Yes, we explicitly removed the code that allowed that to function in #27627 as what that codepath was doing was unstable in the presence of contextually typed attributes (ie, function children or attributes with untyped parameters). We're looking into fixing #7294 in a more general, safer way as a replacement.

@weswigham
Copy link
Member

weswigham commented Oct 17, 2018

(So yes, it's broken right now, yes, we know, and yes, we'd like to fix it, however the break was intentional to fix a mountain of other problems caused by the hackish way it was accomplished.)

@ulrichb
Copy link

ulrichb commented Oct 22, 2018

@weswigham

I get the same error ("JSX element type 'React.Fragment' does not have any construct or call signatures") for the following code.

import * as React from "react";

const fragment = (
    <React.Fragment key={42}>
        <div>a</div>
        <div>b</div>
    </React.Fragment>
);

Does this mean that the declaration for React.Fragment has to change?

The current (Definitly Typed) declaration is as follows.

const Fragment: ComponentType;
type ComponentType<P = {}> = ComponentClass<P> | StatelessComponent<P>;

@brieb
Copy link

brieb commented Oct 22, 2018

@ulrichb Yep, Fragment is defined in the same way as Provider and Consumer, using ComponentType. StrictMode would also need to be changed.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3ea5ad14c4b453916df64fbec2bf3530d15c9184/types/react/index.d.ts#L272

@ulrichb
Copy link

ulrichb commented Oct 22, 2018

@brieb Thanks!

@Jessidhia
Copy link

Jessidhia commented Oct 24, 2018

Since I was adding a special SFC type definition for React.memo in DefinitelyTyped/DefinitelyTyped#29990 I went and also fixed the types of Fragment and Context.Provider / Context.Consumer.

However, I'm not sure how... feasible it is to just make ComponentType unusable as a JSX tag. It makes declaring higher order functions really difficult, declaring reusable types for higher order functions impossible (TS can't infer the type of the assignment), and trips tslint checks that guard against otherwise redundant overloads.

type WorksTodayHOC<P> = (Component: React.ComponentType<P>) => React.SFC<P>;
const AHoc: WorksTodayHOC<{}> = Component => props => <Component {...props}/>;

type BrokenInNextTooHOC<P> = {
    // trips tslint rule, "should be" React.Component<P> | React.SFC<P>
    (Component: React.Component<P>): React.SFC<P>
    (Component: React.SFC<P>): React.SFC<P>
};
// RHS doesn't get inferred so Component and props are implicit anys
const BHoc: BrokenInNextTooHOC<{}> = Component => props => <Component {...props}/>;

@weswigham
Copy link
Member

However, I'm not sure how... feasible it is to just make ComponentType unusable as a JSX tag.

So yes, it's broken right now, yes, we know, and yes, we'd like to fix it,

Just authoring a solution that feels as little like a react/jsx-specific hack as possible, so it's taking a bit. We actually have a laundry list of issues to do with calling unions of types with signatures, so I've been looking at fixing it in the context of fixing those at the same time.

@weswigham
Copy link
Member

(I do personally like the exotic component change, tho (I feel like it lies a bit less) - ideally we wouldn't use the call signature at all and there could just but a factory function overload that works over exotic components; but we're not there yet)

@Jessidhia
Copy link

With the advent of ref forwarding, it's also becoming more useful to have access to the actual type of the input component, so the ideal generic would actually be <TComponent extends React.ComponentType<infer TProps>>, but infer doesn't work outside conditional types yet.

@weswigham
Copy link
Member

Today's nightly aughta fix all the common cases of this (like ComponentType or other HOC-likes where both the component constructor and function component take similar arguments). You still can't have a tag of type (p: {x: string}) => Element | (p: {y: number}) => Element and have it resolve when you call with {x: "", y: 12} anymore; but that JSX ever did that at all was quite strange, and most certainly needs a full fix of #7294 to happen safely.

@calebeby
Copy link

calebeby commented Nov 5, 2018

const El = i.href ? 'a' : 'div'
return <El class={style.row} key={icon} {...i}/>

JSX element type 'El' does not have any construct or call signatures

Also seeing this in 3.2.0-dev.20181103, which worked in 3.1. Since both a and div are valid html elements (and they have the same attributes, at least in the Preact type definitions), can you make this not create an error?

@weswigham
Copy link
Member

weswigham commented Nov 15, 2018

@calebeby I just opened a PR (#28557) that reenables some patterns like that, but not for the pair a and div like you've linked - the attributes types for those two don't have a strict type/subtype relationship in that case and at present our union call handling logic can't handle resolving a call against signatures like that (that JSX attempted to before was a fountain of many bugs). A full fix will be pending a fix of #7294.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Domain: JSX/TSX Relates to the JSX parser and emitter Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants