Types: Missing overload for default export from @emotion/styled ?

karfau commented 4 years ago

The problem

I wanted to generate types for rebass modules using the existing js files (for more details read the Additional Context section). But the way the rebass lib uses @emotion/styled doesn't seem to be supported by the current types, when running tsc --declaration (with some other basic configuration to get everything together) the following error is reported:

src/index.js:50:28 - error TS2769: No overload matches this call. ...

I would love be able to add the missing overload to the types, but after looking at the nested modules the are involved I was kinda lost when reaching https://github.com/emotion-js/emotion/blob/master/packages/serialize/src/index.js#L342

My understanding of the argument types passed to styled in @rebass/reflexbox are

styled(
  string, // referring to one of JSX.IntrinsicElements keys
  propsToMerge // seems to be quite generic but has an impact on the props of the resulting component?
)(
  defaultPropsToBeTransformed // just a guess here, didn't check emotion code about it
  ...transformers // tons of functions 
  // looks like they are executed in the given order 
  // to modify the props after merging and before passing them to rendering?
)

Maybe these transformers are of type FunctionInterpolation but the allowed types would be Interpolation (from the same file)?

Proposed solution

Adding the missing overload to the types.

Alternative solutions

Help me to understand/find the related documentation so I can create a PR.

Additional context

The types for the main rebass module are "only" existing in DefinitelyTyped, are incomplete and hardly maintained and the types for other @rebass/* modules don't exist as far as I know.

So I wanted to experiment if there is a(n "easy") way to create and maintain the types by generating the definition files from the js source files. It's possible to do that and since the underlying modules all offer types I was quite hopefull.

On the lowest level of rebass is @rebass/reflexbox so I started with it. (I pushed my changes to my fork: https://github.com/karfau/rebass/tree/generating-types)

Potentially related issues (I'm not sure):

Andarist commented 4 years ago

Hi! Current typings (on master) are less than ideal - we have improvements in the works though. They are available on the next branch. Could you try them out to see if you are able to build rebass typings on top of them?

karfau commented 4 years ago

Hi, I tried to switch just @rebass/reflexbox to @emotion/(core|styled)@11.0.0-next.5 from the package repository and the error message didn't change.

Here is the exact output (using typescript 3.7.2):

$ tsc --declaration
src/index.js:27:20 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(...styles: Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>[]): StyledComponent<...>', gave the following error.
    Argument of type '(props: any) => CSSObject' is not assignable to parameter of type 'Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
      Type '(props: any) => CSSObject' is not assignable to type 'FunctionInterpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
        Type 'CSSObject' is not assignable to type 'Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
          Type 'CSSObject' is not assignable to type 'Keyframes'.
            Type 'CSSObject' is missing the following properties from type '{ name: string; styles: string; anim: number; toString: () => string; }': name, styles, anim
  Overload 2 of 2, '(template: TemplateStringsArray, ...styles: Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { ...; } & { ...; }>[]): StyledComponent<...>', gave the following error.
    Argument of type '{ boxSizing: string; margin: number; minWidth: number; }' is not assignable to parameter of type 'TemplateStringsArray'.
      Object literal may only specify known properties, and 'boxSizing' does not exist in type 'TemplateStringsArray'.

One more detail: I just verified that the problem is in the "second half" of the expression. by extracting styled('div, {shouldForwardProps}) into an own const the returned value is inferred to CreateStyledComponentIntrinsic<string, {}, any> (is that the expected one?). and the ts error now points to the call to that function. (Yesterday I missed the first argument in the description, updated it accordingly, named them defaultPropsToBeTransformed.)

PS: It seems a bit odd that the error message didn't change, I'll check again the the updates have been applied as expected, but not sure how soon I'm able to do that. Might take some days.

Andarist commented 4 years ago

It would be great if you could share the updated version of this so we could inspect the repro case quickly (without setting things ourselves - which we could mess up).

karfau commented 4 years ago

ping (just in case the above commit didn't trigger any notification): the fork has been updated with your suggestion. And I also checked that they have been applied as expected.

So steps to reproduce:

// after git cloning my fork and switching to branch generating-types
yarn
cd packages/reflexbox
yarn types

Andarist commented 4 years ago

Thanks for the ping - referencing the issue from a commit didnt trigger any notification. I’ll try look into it this week

Andarist commented 4 years ago

I've started looking into this. I see the error, but I'm wondering about one thing. Are you trying to generate types here from vanilla JS file?

karfau commented 4 years ago

Yes, exactly:

I wanted to generate types for rebass modules using the existing js files (for more details read the Additional Context section).

But also not only "vanilla JS" files: the types for all the things that are imported are available to typescript.

They are also generated, but when there is an error like the one I described it just put's any as the type, which is not very helpful.

(And there are also ways to add more type information using jsdoc, so without changing the actual code.)

But for this to work properly I need to understand what kind of call this actually is. (Maybe it can just not be inferred but could be added in another way.)

Andarist commented 4 years ago

I've made some progress - you had 2 copies of csstype in your node_modules and they were conflicting with each other. I've bumped our dependency range and published a new version - it didn't quite help though. So I've removed csstype entries from yarn.lock and run yarn to regenerate the lockfile+dep tree. This has fixed the problem of duplicated csstype and I believe that it has also fixed the reported problem, but there is still another one.

I get now:

  Overload 1 of 2, '(...styles: Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>[]): StyledComponent<...>', gave the following error.
    Argument of type '(props: { theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }) => InterpolationWithTheme<...>' is not assignable to parameter of type 'Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
      Type '(props: { theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }) => InterpolationWithTheme<...>' is not assignable to type 'FunctionInterpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
        Type 'InterpolationWithTheme<any>' is not assignable to type 'Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
          Type 'ArrayInterpolation<undefined>' is not assignable to type 'Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
            Type 'ArrayInterpolation<undefined>' is not assignable to type 'ObjectInterpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { theme: any; } & { theme: any; variant: any; tx?: string | undefined; }>'.
              Types of property 'filter' are incompatible.
                Type '{ <S extends Interpolation<undefined>>(callbackfn: (value: Interpolation<undefined>, index: number, array: Interpolation<undefined>[]) => value is S, thisArg?: any): S[]; (callbackfn: (value: Interpolation<...>, index: number, array: Interpolation<...>[]) => unknown, thisArg?: any): Interpolation<...>[]; }' is not assignable to type 'string | string[] | undefined'.
                  Type '{ <S extends Interpolation<undefined>>(callbackfn: (value: Interpolation<undefined>, index: number, array: Interpolation<undefined>[]) => value is S, thisArg?: any): S[]; (callbackfn: (value: Interpolation<...>, index: number, array: Interpolation<...>[]) => unknown, thisArg?: any): Interpolation<...>[]; }' is missing the following properties from type 'string[]': pop, push, concat, join, and 24 more.
  Overload 2 of 2, '(template: TemplateStringsArray, ...styles: Interpolation<{ theme?: any; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { ...; } & { ...; }>[]): StyledComponent<...>', gave the following error.
    Argument of type '{ boxSizing: string; margin: number; minWidth: number; }' is not assignable to parameter of type 'TemplateStringsArray'.
      Object literal may only specify known properties, and 'boxSizing' does not exist in type 'TemplateStringsArray'.

which is caused by props => props.css. If you remove it then it compiles OK.

I'm not sure how to fix it but in general Theme-related types are being reworked now in https://github.com/emotion-js/emotion/pull/1609 and also I've noticed that there is some mess around Interpolation & CSSInterpolation types. I plan to dig deeper into those and cleanup this and I hope both of those will fix a problem with that props => props.css.

karfau commented 4 years ago

Thx for digging into it to this level. I only see a very small difference in the error message, but it might of course be an step forward.

I subscribed to the PR you mentioned, so I'll be informed when it lands. Beside that I'm currently not able/knowing how to support this case in any way. But if you want me to try something just let me know.

Andarist commented 4 years ago

While digging into it I've even encountered:

Type 'CSSObject' is not assignable to type 'CSSObject'.

😅

That got me to realize there are 2 conflicting versions of csstype. I believe that last part (that props => props.css) should be rather easily typeable - it's not any weird pattern. I need to analyze how our current typings are structured to fix this though, probably need to draw a dependency tree for them to see what depends on what and what is supposed to be used where. I don't think this is that hard to do - just time-consuming :s

Andarist commented 4 years ago

I've released 11.0.0-next.8 and it got better a little - but it fails on that props => props.css. I'm not sure if this is possible to work because it has an incompatible type. It gets inferred as this: https://github.com/emotion-js/emotion/blob/0362fd2dc691e9084cf8b7b59df15377cc60dfdd/packages/core/types/index.d.ts#L99 and it's not compatible with this: https://github.com/emotion-js/emotion/blob/0362fd2dc691e9084cf8b7b59df15377cc60dfdd/packages/styled/types/base.d.ts#L74-L79

But I'm also not sure if you should try to compose it like this. I don't know much about styled-system so maybe I'm missing something, but at least from emotion's perspective, css prop is never passed to interpolations. It gets evaluated before that happens. So it is never receives in props.css

karfau commented 4 years ago

If I understood you correctly you are not sure why https://github.com/rebassjs/rebass/blob/51e10d992430edcd85b3a229556ecbe4a8d9d60b/packages/reflexbox/src/index.js#L37 is there/ what it's purpose or function is?

Maybe we can invite @jxnblk to the discussion to help us understand that? (Not regarding types, just the code.)

Andarist commented 4 years ago

If I understood you correctly you are not sure why https://github.com/rebassjs/rebass/blob/51e10d992430edcd85b3a229556ecbe4a8d9d60b/packages/reflexbox/src/index.js#L37 is there/ what it's purpose or function is?

Yes. I suppose styled-system might use css prop differently than we do. But currently its type (in your file) comes from us (we mutate global types): https://github.com/emotion-js/emotion/blob/0362fd2dc691e9084cf8b7b59df15377cc60dfdd/packages/core/types/index.d.ts#L97-L101

jxnblk commented 4 years ago

@karfau the manually added css prop in Rebass is for use where it's not possible to use the Emotion createElement function, such as MDX, which has its own. If you have Emotion's JSX pragma set, it will use Emotion's implementation in JS files, so this is mostly used as a fallback

karfau commented 4 years ago

Thx for the reply @jxnblk. I hope this information helps @Andarist to understand what is going on.

Andarist commented 4 years ago

Ok, it makes sense - but it still doesn't play OK with defined types because we want to target a styled component with this but this css is typed for a css prop.

I'm not entirely sure about this - but maybe you could add inline annotations (using comments - I think TS supports this) to cast this to any (for starters) and maybe try to type it to an expected type. Don't have a better idea than this right now.

Because of mismatching versions of emotion in that repository with current v11 I had to do extra manual steps each time to get to that props.css problem (because I had to resolve mismatching problems first). I would prefer waiting for a v11 release (hopefully this month) and then trying to work this out when rebass starts using it.

emotion-js / emotion