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

Support passing generics to tagged template string #11947

Closed
patrick91 opened this Issue Oct 30, 2016 · 33 comments

Comments

@patrick91
Copy link

patrick91 commented Oct 30, 2016

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

This comment has been minimized.

Copy link
Contributor

blakeembrey commented Nov 12, 2016

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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Nov 12, 2016

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

This comment has been minimized.

Copy link
Contributor

blakeembrey commented Nov 13, 2016

@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

This comment has been minimized.

Copy link

rossipedia commented Dec 12, 2016

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

This comment has been minimized.

Copy link
Contributor

Igorbek commented Jan 10, 2017

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>;
}
  • there's no way to infer TNewProps from the usage in interpolations
  • no way to cast return type (it wouldn't match)
  • workaround: specify type of props in each interpolation in form of Props & TNewProps, which will be incredibly annoying
@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Mar 7, 2017

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

  • call expression: foo()
  • new expressions: new Foo()
  • tagged templates: foo `stuff`
  • decorators: @FooComponent class C { }

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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Mar 7, 2017

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

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Contributor

Igorbek commented Mar 8, 2017

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

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Mar 8, 2017

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

This comment has been minimized.

Copy link

sergeysova commented Jul 23, 2017

Some progress here?

@aminroosta

This comment has been minimized.

Copy link

aminroosta commented Sep 6, 2017

@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?

@Havret

This comment has been minimized.

Copy link

Havret commented Apr 8, 2018

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

@malimccalla

This comment has been minimized.

Copy link

malimccalla commented Apr 8, 2018

@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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Apr 14, 2018

@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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Apr 14, 2018

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

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Apr 16, 2018

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

@nickbreaton

This comment has been minimized.

Copy link

nickbreaton commented Apr 19, 2018

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

This comment has been minimized.

Copy link

ralexand56 commented Apr 20, 2018

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

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Apr 21, 2018

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

This comment has been minimized.

Copy link

beshanoe commented Apr 22, 2018

@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(styled-components/styled-components#1697 (comment)), 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

This comment has been minimized.

Copy link

marvinhagemeister commented Apr 22, 2018

@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

This comment has been minimized.

Copy link

beshanoe commented Apr 22, 2018

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

@beshanoe

This comment has been minimized.

Copy link

beshanoe commented May 18, 2018

@DanielRosenwasser could you please comment on #11947 (comment)

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented May 29, 2018

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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented May 29, 2018

@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>

@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.
You can’t perform that action at this time.