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

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

Open
karfau opened this issue Nov 17, 2019 · 16 comments
Open

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

karfau opened this issue Nov 17, 2019 · 16 comments

Comments

@karfau
Copy link

karfau commented Nov 17, 2019

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):

karfau added a commit to karfau/rebass that referenced this issue Nov 17, 2019
+ latest `typescript` as `devDependency` on the root level
+ `tsconfig.json` & types for dependencies per package
+ npm script `types` per package

issue with types from `@emotion/styles` described in emotion-js/emotion#1640
@Andarist
Copy link
Member

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
Copy link
Author

karfau commented Nov 18, 2019

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
Copy link
Member

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 added a commit to karfau/rebass that referenced this issue Nov 18, 2019
@karfau
Copy link
Author

karfau commented Nov 20, 2019

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
Copy link
Member

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

@Andarist
Copy link
Member

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
Copy link
Author

karfau commented Nov 25, 2019

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
Copy link
Member

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 #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
Copy link
Author

karfau commented Nov 26, 2019

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
Copy link
Member

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
Copy link
Member

Andarist commented Dec 7, 2019

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:

css?: Interpolation<Theme>

and it's not compatible with this:
Interpolation<
ComponentProps &
SpecificComponentProps &
StyleProps &
AdditionalProps & { theme: Theme }
>

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
Copy link
Author

karfau commented Dec 7, 2019

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
Copy link
Member

Andarist commented Dec 7, 2019

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):

declare module 'react' {
interface DOMAttributes<T> {
css?: Interpolation<Theme>
}
}

@jxnblk
Copy link

jxnblk commented Dec 11, 2019

@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
Copy link
Author

karfau commented Dec 11, 2019

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

@Andarist
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants