microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.75k stars 12.46k forks source link

Support passing generics to tagged template string #11947

Closed patrick91 closed 6 years ago

patrick91 commented 7 years ago

I was trying to write some type definition for the styled-components project, but it looks like there is no way to specify a generic when using a tagged template. So... here is a subset of the typedef I'm writing:

declare module 'styled-components' {
    import * as React from "react";

    export function input<P>(values: TemplateStringsArray): React.StatelessComponent<P>;
}

This is how I'd like to use it:

import * as React from 'react';

/// <reference path="../../../typings/styled-components/styled-components.d.ts" />

import styled from 'styled-components';

interface Props {
  placeholder: string;
}

const Input = styled.input<Props>`
  font-size: 1.5em;
  text-align: center;
  color: palevioletred;
`;

I know that it can also be possible to override Input.propTypes manually[1], but I was wondering if this could be an useful addition to Typescript.

TypeScript Version: 2.0.3

[1] Or even write a custom component and just use the styled(Component)style`` syntax

blakeembrey commented 7 years ago

From twitter, turns out this issue already exists: https://twitter.com/drosenwasser/status/797222514715824128. I think this is the correct syntax to use and the same thing I tried when I wanted to use generics with a template tag.

DanielRosenwasser commented 7 years ago

If all you're doing is specifying the type argument for an output type (and the type system can't make any checks on that), that's not really a case for generics - that seems like a better case for a type assertion.

The one thing that I do see as a benefit in the original use case is that you don't have to write React.StatelessComponent every time, which is definitely a pain.

blakeembrey commented 7 years ago

@DanielRosenwasser The use-case doesn't bother me, it's only the feature I really care about 😄 I could very well want to restrict the types of parameters the tag can use to just values/keys of the type, for instance (E.g. current nightly features).

Edit: I'd love to have the generic discussion somewhere, where's the best place to do that? Personally I like specify the return type in generics (where the return is valuable) for a couple of reasons:

  1. Without it, coercion is tricky (assigning a semi-compatible type that looks similar) and messy (e.g. have to wrap it specifically in parens when using chaining) and a new developer can get it wrong.
  2. Adding it in the future would currently require breaking backward compatibility, so introducing it once features like the keyed types or members of lands on stable would break current users.
rossipedia commented 7 years ago

I just ran into this trying to put together a small template library:

image

It would be really nice if the first example worked (and would be the least astonishing, IMHO)

Igorbek commented 7 years ago

Since it's awaiting more feedback, here's my feedback. I am pretty sure the use case is not only for return type. Consider signature:

interface SimplifiedStyledFunction<P> {
  <TNewProps>(strings: TemplateStringsArray, ...interpolations: ((props: P & TNewProps) => string)[]): StyledComponent<P & TNewProps>;
}
DanielRosenwasser commented 7 years ago

To give some technical background on this, there are four operations that we consider flavors of function invocations in the compiler:

The latter two do not allow explicit type arguments. My feeling is that both should be allowed, but given the demand, work to support tagged templates is reasonable and should have some more priority.

DanielRosenwasser commented 7 years ago

If anyone does decide to send a PR, give us all a heads up here first.

DanielRosenwasser commented 7 years ago

So I remembered the real reason we didn't do this originally.

Right now, tagged templates are considered MemberExpressions. Those bind just a bit more tightly than a CallExpression or NewExpression.

Since tagged templates will always bind more tightly, we'll always try to parse tagged template type arguments, even when we're going to parse out a call expression.

If you take the following:

foo<Bar>()

A naive approach would do the following:

  1. Let me try to parse out a MemberExpression
    1. I have a foo. Keep parsing out the rest of a MemberExpression.
    2. I see <! Let me look try parsing out type arguments for a tagged template.
    3. I have type arguments! Let me see if I have a template string.
    4. I have an open paren - let me discard all the work I've done to get those type parameters. 😢
  2. I was able to get foo as a MemberExpression. Let me see if I can parse out a call.
    1. I see a <. I should parse out type arguments as part of a CallExpression.
    2. I got type arguments! Now I see an open paren! I'll try parsing out the call arguments.

This approach means that for every call expression with generics, we have to perform steps 1.ii, through 1.iv even though we're going to throw away the result.

While it's rare that you have to supply type arguments, it's less-than-ideal to do a re-parse every time.

Igorbek commented 7 years ago

So, can a tagged template be treated as a variation of a CallExpression then?

DanielRosenwasser commented 7 years ago

It already sort of is from the perspective of type-checking - during overload resolution and whatnot, we create a "virtual" argument for the template strings object.

But it's important to keep the two syntactically separate. The solution is probably to make an intermediate parsing production that sits between MemberExpression and CallExpression. This is more a technical issue more than anything else.

sergeysova commented 7 years ago

Some progress here?

aminroosta commented 7 years ago

@DanielRosenwasser

we have to perform steps 1.ii, through 1.iv even though we're going to throw away the result.

This makes perfect sense and as you already said, not ideal.

How much impact this will really have on parser performance?
Is there a parsing approach or trick that can reduce this?
I mean is it really a deal breaker?

yordis commented 6 years ago

@mhegazy is somebody working on this one? I would love to know the state of this issue. Working with styled-components so.

mhegazy commented 6 years ago

I am not aware of anyone working on this. We are open to taking a PR for this feature if anyone is interested in taking it on.

ralexand56 commented 6 years ago

It's little things like this that makes working with Typescript such a pain some times and cause it to be abandon. I thought tagged template strings were part of the javascript spec. Am I missing something?

Havret commented 6 years ago

Really looking forward to it!

MichalLytek commented 6 years ago

I've created a small workaround that is more developer-friendly than styled-components-ts:

export type Styled<
  Props,
  element extends (keyof JSX.IntrinsicElements) | null = null
> = SFC<
  Props &
    (element extends keyof JSX.IntrinsicElements
      ? JSX.IntrinsicElements[element]
      : React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement>)
>;

export function Styled<Props>() {
  return <TElement extends keyof JSX.IntrinsicElements>(
    element: TElement,
  ): Styled<Props, TElement> => {
    return props => createElement(element, props);
  };
}

So the usage is like this:

interface ButtonProps {
  fontSize?: number;
}
const ButtonCompManual: Styled<ButtonProps> = ({ fontSize, ...props }) => (
  <button {...props} />
);
const ButtonCompAutomatic = Styled<ButtonProps>()('button');

The Styled function is curried due to the limitation of default and inferred types in generics.

malimccalla commented 6 years ago

Wondering the status on this. I'm currently working around this with a custom withProps functions

export const withProps = <U>() => <P, T, O>(
  fn: styledComponents.ThemedStyledFunction<P, T, O>
) => fn as styledComponents.ThemedStyledFunction<P & U, T, O & U>;

Usage is like this

const Icon = withProps<{ size?: string }>()(styled.span)`
  height: ${props => (props.size ? props.size : '1.4rem')};
  width: ${props => (props.size ? props.size : '1.4rem')};

  display: flex;
  align-items: center;
  justify-content: center;
`;

This ends up causing a bunch of other problems though. I'm very new to typescript but can help out where possible to get template string working well.

beshanoe commented 6 years ago

Yeah, I'm also interested! @DanielRosenwasser you've mentioned that in order to support this feature, some changes should be made to the way parser works now. So I wonder, is it just not on your priority list and can be done with help from community, or it implies deep architectural changes which only can be done by TS team members and requires very solid investigation?

Havret commented 6 years ago

@malimichael I really like your solution but what kind of other problems do are you referring to?

malimccalla commented 6 years ago

@Havret The other problems happening now are more just inconveniences than anything else. Things like syntax highlighting and auto-completion no longer working for components wrapped by withProps; reasons I choose to use Typescript in the first place

DanielRosenwasser commented 6 years ago

@beshanoe I don't think it's something that is confined to the TypeScript team illuminati 😄. The community can help here, but I definitely had some difficulties when I tried fixing it a while back. It's totally possible I'm missing something if you want to take a stab at it. I might give it another shot some time soon.

DanielRosenwasser commented 6 years ago

The problem actually seems easier now that I look at it again. I'm going to take a second swing at it.

DanielRosenwasser commented 6 years ago

Check out the fix at #23430 and let me know what you think!

nickbreaton commented 6 years ago

Thank you so very much @DanielRosenwasser and everyone else who assisted. I know myself and many others are extremely excited to use this when it is released.

I am sure the styled-components community in particular will be very grateful. @styled-components/typers

ralexand56 commented 6 years ago

Yes, Thanks, Daniel! So how long does it take for something like this to go live?

DanielRosenwasser commented 6 years ago

You're welcome! You can use it with the most recent nightly builds! To try it out quickly:

npm install -g typescript@next

or to try it out locally

yarn add typescript@next

# or...

npm install typescript@next
beshanoe commented 6 years ago

@DanielRosenwasser looks like there's a little bug with an implementation. Apparently, the compiler doesn't choose correct version of method among overloads.

interface IFooFn {
  (strings: TemplateStringsArray): Promise<{}>;
  <T>(strings: TemplateStringsArray): Promise<T>;
}

declare const fooFn: IFooFn;

export const promise = fooFn<number>``; // promise is of type Promise<{}>

Such a code is now used in styled-components typings(https://github.com/styled-components/styled-components/issues/1697#issuecomment-383284043), and it should work out-of-the-box but it doesn't, because the overloaded fn without generic always hits first. if we call fooFn as a plain function, it works as expected

marvinhagemeister commented 6 years ago

@beshanoe Curious as to why the overload is needed. Wouldn't a default parameter for the generic solve this?

type IFooFn<T = any> = (strings: TemplateStringsArray): Promise<T>;
beshanoe commented 6 years ago

@marvinhagemeister I'm also interested why does styled-components typings have this line at all, but at least it revealed this buggy behavior

beshanoe commented 6 years ago

@DanielRosenwasser could you please comment on https://github.com/Microsoft/TypeScript/issues/11947#issuecomment-383367001

DanielRosenwasser commented 6 years ago

Thanks for catching that, and sorry we missed this, but please don't comment on closed issues. It's almost impossible for us to read everything.

DanielRosenwasser commented 6 years ago

@beshanoe As a workaround, you can use a generic default:

interface IFooFn {
    <T = {}>(strings: TemplateStringsArray): Promise<T>;
}

declare const fooFn: IFooFn;

export const promise = fooFn<number>``; // promise is of type Promise<number>