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

Using lookahead to detect generic JSX elements? #6395

Closed
dead-claudia opened this issue Jan 8, 2016 · 32 comments
Closed

Using lookahead to detect generic JSX elements? #6395

dead-claudia opened this issue Jan 8, 2016 · 32 comments
Assignees
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Effort: Difficult Good luck. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@dead-claudia
Copy link

Initially from a couple different comments, but elaborated here.

Currently, in order to use a generic JSX element, you must alias both the type and interface to a non-generic specialization.

class Select<T> extends React.Component<SelectProps<T>, any> {}

// Alias
type StringSelect = new () => Select<string>
const StringSelect = <StringSelect> Select

const Form = () => <StringSelect items={['a','b']} />

It would be a lot more convenient and consistent to use something like this:

const Form = () => <Select<string> items={['a', 'b']} />

In order to differentiate the latter, you can use a single token of lookahead in the parser to know if it's a generic JSX element:

  • If the next token is a <, like in < Select <, then consume the rest of the type as a generic type. After consuming that:
    • If the next token is a >, then finish the type as a generic type cast.
    • If the next token is an identifier or a { (for spread attributes, when they get implemented), then parse the rest as a JSX element.
    • Otherwise, parse the rest as a type cast.
  • If the next token is a >, like in < Select >, then finish the type as a simple type cast.
  • If the next token is an identifier or a {, then parse the rest as a JSX element.
  • Otherwise, consume the rest as a type cast.

I don't know of any reason that wouldn't be feasible to do, considering async arrow functions, which are already implemented, require potentially a whole call expression of parsing to differentiate between let f = async (x, y), which is a call expression, and let f = async (x, y) => x, which is an async arrow function. (I feel sorry for whoever ends up implementing that in V8, which doesn't support any backtracking or much lookahead for memory reasons.)

/cc @RyanCavanaugh

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jan 8, 2016
@RyanCavanaugh
Copy link
Member

We've gotten this request a number of times and it does seem reasonable in terms of parsing.

@DanielRosenwasser DanielRosenwasser added the Domain: JSX/TSX Relates to the JSX parser and emitter label Jan 8, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Effort: Difficult Good luck. and removed In Discussion Not yet reached consensus labels Feb 1, 2016
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Feb 1, 2016
@RyanCavanaugh
Copy link
Member

Accepting PRs.

This should be easy to parse but I think it's going to be some rather difficult work in the checker.

@stepancar
Copy link

@RyanCavanaugh whats status of this issue?
It's very impotant functionality.

import * as React from 'react';

interface MyTestProps<T> {
    values: Array<T>;
    selectHandler: (selectedVal: T) => void;
}

export class MyTest<T> extends React.Component<MyTestProps<T>, {}> {

    render() {
        return (
            <ul>
                { this.props.values.map(val =>
                    <li>{val}</li>
                ) }
            </ul>
        );
    }
}

class TestMyTest extends React.Component<{}, {}> {
    render() {
        return (
            <MyTest values={[1, 2, 3, 4]} selectHandler={(val) => { } } />
        );
    }
}

compiler throws error "type number[] is not assignable to type T[]"

But in this example

import * as React from 'react';

interface MyTestProps<T> {
    values: Array<T>;
    selectHandler: (selectedVal: T) => void;
}

export class MyTest<T> extends React.Component<MyTestProps<T>, {}> {

    render() {
        return (
            <ul>
                { this.props.values.map(val =>
                    <li>{val}</li>
                ) }
            </ul>
        );
    }
}

new MyTest({ values: ['a', 'b'], selectHandler: (v) => { v.length } });

selectHandler param v have type string;

@donaldpipowitch
Copy link
Contributor

donaldpipowitch commented Sep 15, 2016

@RyanCavanaugh Could this be part of a future planned milestone? 2.1 or 2.2?

Maybe a nicer API as workaround: we introduce a static function of on components with generics:

import React, { Component } from 'react';
interface FooProps<T> {
  data: T;
  someCallback: (data: T) => void;
}

class Foo<T> extends Component<FooProps<T>, void> {
  // `of` is created here
  static of<T>() {
    return Foo as new () => Foo<T>;
  }

  render() {
    // make something with this.props.data here...
    return (
      <div onClick={() => this.props.someCallback(this.props.data)}>
        Click me!
      </div>
    );
  }
}

interface CoolData {
  baz: string;
}

const coolData: CoolData = {
  baz: 'Hello'
};

// `of` is used here
const FooWithCoolData = Foo.of<CoolData>();

const Bar = () =>
  <FooWithCoolData
    data={coolData}
    someCallback={data => console.log('I know data is CoolData here!')}
  />;

Or if we don't want to use <T> syntax, but use the actual object, we do this (I'm still surprised this works):

// new `of` inside `class Foo<T>`
  static of<T>(t: T) {
    return Foo as new () => Foo<T>;
  }

// new usage of `of`
const FooWithCoolData = Foo.of(coolData);

@RyanCavanaugh
Copy link
Member

We'll be basically redoing JSX typechecking in 2.1 or 2.2 and can probably look at this at that time.

@donaldpipowitch
Copy link
Contributor

Cool. Thank you for the update! :)

@BANG88
Copy link

BANG88 commented Dec 14, 2016

Any update ?

@alitaheri
Copy link

@RyanCavanaugh Please consider supporting at least type inference when possible. No need to add new syntax. Just having a generic component infer its type parameters solve a huge number use-cases.

@RyanCavanaugh
Copy link
Member

@alitaheri I believe the existing JSX work going on at #12107 will enable that scenario

@MastroLindus
Copy link

MastroLindus commented Feb 23, 2017

Still not working on [email protected].
The typescript team is making working with typescript and react a very fantastic experience, but unfortunately this issue is one of the most annoying that still remains unsolved.

The workaround works fine (

const BooleanSelect: new () => SingleColumnSelect<boolean | null> = SingleColumnSelect;
  <BooleanSelect values={[null, true, false]}/>

) but it's not only ugly and un-elegant, it is also really unclear for newcomers to react&typescript

Either solution (allowing to specify the generic inside jsx or using type inference) would be really really welcome

@zzzev
Copy link

zzzev commented Feb 23, 2017

@MastroLindus in the meantime you might consider this workaround, which I find to be a bit simpler to read and write:

class Select<T> extends React.Component<...> {...}
class BooleanSelect extends Select<boolean> {}
class SomeComponent extends React.Component<{}, {}> {
  render() {
    return <BooleanSelect />; // this works fine
  }
}

@MastroLindus
Copy link

@zzzev I am aware of it( or others, like just a simple cast) but to define a new class just for the workaround somehow it feels even dirtier to me, even if in practice the two forms are not that much different... I simply cannot find one that I am really satisfied about

@yuit
Copy link
Contributor

yuit commented May 9, 2017

We are still need a way to infer type parameter in the original example for generic React class component but here are some possible work around in some cases:

(Note this is for 2.3 compiler)
** Using SFC **
as mentioned by @RyanCavanaugh's comment, compiler now can infer type parameter in SFC

import * as React from "react";

type SelectProps<T > = { items: T[] }
function Select<T>(attr: SelectProps<T>) {...}

const someItems = [1, 2, 3, 4];
const someItems2 = ["ss"];

function renderJsx() {
    return <Select items={someItems} />; // Ok
}

Nightly & future 2.3.3
Using default type parameter (in this example, it may not be desirable to do so but the compiler will not give you error for being generic)

import * as React from "react";

type SelectProps<T > = { items: T[] }
class Select<T = any> extends React.Component<SelectProps<T>, any> { }

const someItems = [1, 2, 3, 4];
const someItems2 = ["ss"];

function renderJsx() {
    return <Select items={someItems} />; // OK
}

@MichaelTontchev
Copy link

Would love to have this feature - would enable a lot of good scenarios :)

@AlexGalays
Copy link

Still as annoying as ever :D

@danielkcz
Copy link

So it's more than a year now since the issue was opened, we are going for a Typescript 2.8 and is not even on the roadmap? I don't get it...

@antanas-arvasevicius
Copy link

maybe not so trivial to do

@dead-claudia
Copy link
Author

dead-claudia commented Mar 8, 2018

@antanas-arvasevicius Apparently, the checker is the difficult part. If you read my initial bug, it was me addressing the other implementation aspect, mentioned in comments linked to from there.

@RyanCavanaugh
Copy link
Member

This should in theory become easier (possibly even trivial) once @weswigham 's work to refactor JSX typechecking is complete.

I checked, and idle complaining that we haven't implemented popular a feature request has not caused new engineer time to appear.

@weswigham
Copy link
Member

We actually do inference and generic instantiation for generic JSX components as of our last release, so we don't even actually really need anything more than the parse step for this anymore (I mean, ofc the call resolution need to be modified to check for type arguments before doing inference, but still - more done than before).

@weswigham
Copy link
Member

Also, the OP is a tad out of date; Unless I'm mistaken, we don't even support parsing angle bracket style typecasts in JSX files, so the described ambiguity wouldn't exist.

@RyanCavanaugh
Copy link
Member

<T>e expressions aren't supported in JSX. The only exception to <T> in a leading expression position starting a JSX element is a generic arrow function <T>(x: T) => e, which I believe has some limited lookahead to try to disambiguate (or maybe we required you to write T extends {} ? I'd have to check).

@weswigham
Copy link
Member

We require a constraint on the first type parameter.

@RyanCavanaugh
Copy link
Member

I stand corrected, idle complaining actually did improve the situation 😆

@antanas-arvasevicius
Copy link

antanas-arvasevicius commented Mar 8, 2018 via email

@weswigham
Copy link
Member

weswigham commented Mar 8, 2018

@antanas-arvasevicius We've merged code that allows us to look up the current global types in a namespace local to the factory function instead of globally already, which should be a significant improvement once libraries move to utilizing it. (And combined with the also recently introduced @jsx pragma will allow you to use multiple jsx variants in a project; just only one per-file)

We want to move to just using the factory function's signature, but there's a slight problem - to create a perfectly well-typed factory function that we can actually do inferences for, you need ReturnType (and InstanceType and FirstArgument) conditional type-types that are overload-aware (otherwise you need compiler internal magic that assumes how the factory function works or looks things up in a namespace, like we do now). As is, we could basically do it, but we'd break overloading on stateless function components (we'd always pick the last signature), which would be no bueno; so fully moving away from the JSX namespace (anywhere) might require that we either improve conditional types or implement real "call" types first. We're actively looking at it, though!

@antanas-arvasevicius
Copy link

antanas-arvasevicius commented Mar 8, 2018 via email

@mhegazy mhegazy modified the milestones: TypeScript 2.8, TypeScript 2.9 Mar 9, 2018
@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Mar 22, 2018
stephenh added a commit to stephenh/DefinitelyTyped that referenced this issue Apr 23, 2018
This allows both the previous hack of:

const StringFlatList = FlatList as { new(): FlatList<string> };

As well as should support upcoming generics in TSX:

microsoft/TypeScript#6395

E.g. <FlatList<string> data=... renderItem=... />
@sebmarkbage
Copy link

Since this is not compatible with the JSX spec as it is right now, it would’ve been good to have the discussion over on the JSX spec repo.

JSX already allows for nested elements in the attribute position <Foo foo=<Foo /> /> so it’s possible this wouldn’t play well with other additions. I don’t see an immediate problem but would be good to take a pass over there in the future like we agreed to do for other additions.

@weswigham
Copy link
Member

I don’t see an immediate problem but would be good to take a pass over there in the future like we agreed to do for other additions.

Apologies - we've had this issue open for over two years with a pretty straightforward design desired from the beginning (implementation of typechecking being the real blocker, which we quietly implemented without the syntax in 2.7); I wasn't aware we had (or needed, since it's a type-based syntax addition, like type arguments for call or new expressions) any renewed outreach set up around this (probably something to do with me not having worked on it 3 years ago XD).

JSX already allows for nested elements in the attribute position <Foo foo=<Foo /> />

As an aside, occasionally I've heard people familiar with the jsx spec point that out (within the team, I think it's a favorite "I bet you didn't know x was valid jsx" fact). We still don't parse it - it's been years since we first noticed it and still haven't gotten a serious issue for it. Still seems underused. It is still broken in babel 6. (fixed in the 7 beta though, so it's got that going for it, though fragments are broken right now) Given how it was spoken of the last time it came up, its future isn't exactly golden (FYI, babylon7 is the only parser on ASTExplorer that can parse the fragment production, but babel can't actually transpile it right now. heh.).

But it was brought up while I implemented this, IIRC 😉 We don't think there are any obvious conflicts (beyond the ones we already have to disambiguate with leading generic parameters and occasionally left shift token that occur elsewhere), either (unless someone's going to start suggesting silly things like jsx elements as property names in fragments or something). If you're looking for a brief on how it was implemented, in our case it's just, after the JSXElementName production in opening/self closing tags, (optionally) a one-token lookahead for <, followed by our TypeArgumentList production (I'm not sure how you'd try to represent leaving space for such a thing in the JSX spec) and a required closing > (before continuing with standard JSX production).

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Effort: Difficult Good luck. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests