deadcoder0904 commented 4 years ago

TS noob here. I have installed @types/theme-ui as well as have the following in theme.ts:

export const theme = {
  fonts: {
    body:
      'system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", sans-serif',
    heading: '"Avenir Next", sans-serif',
    monospace: 'Menlo, monospace',
  },
  colors: {
    text: '#000',
    background: 'white',
    primary: '#bf1',
  },
  modes: {
    dark: {
      text: '#fff',
      background: '#000',
      primary: '#0cf',
    },
  },
}

How do I convert it to TS so I can get autocomplete?

I tried the following:

import {Theme} from 'theme-ui'

export const theme: Theme = {
  fonts: {
    body:
      'system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", sans-serif',
    heading: '"Avenir Next", sans-serif',
    monospace: 'Menlo, monospace',
  },
  colors: {
    text: '#000',
    background: 'white',
    primary: '#bf1',
  },
  modes: {
    dark: {
      text: '#fff',
      background: '#000',
      primary: '#0cf',
    },
  },
}

But I get red squiggly lines on modes with the error

Object literal may only specify known properties, and ‘modes’ does not exist in type ‘Theme’

I tried finding what should be the type in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/theme-ui/index.d.ts but I can’t find it. Then I saw https://github.com/system-ui/theme-ui/issues/668 to wonder if it’s even complete. I just want the theme object to have correct keys :)

TheFirstMe commented 4 years ago

@deadcoder0904 modes should be under colors? Source

deadcoder0904 commented 4 years ago

Woah looks like it. I did that & it worked. Not sure where I found the above code but I vividly remember copying it from somewhere online 😂

Anything else that needs to be using TypeScript?

TheFirstMe commented 4 years ago

I’m not sure I don’t do TypeScript.

erikdstock commented 4 years ago

When you are starting a project from scratch, the more you can keep to using types the better - use any as an escape hatch when necessary, and learn how to contribute missing types (it is confusing for sure). For your theme interface what you have looks fine to me!

deadcoder0904 commented 4 years ago

Idk where else I need to use types in theme-ui. The only part I can think of is the theme object which I’ve already done.

A TS guide would’ve been helpful :)

hasparus commented 4 years ago

A TS guide would’ve been helpful :)

@deadcoder0904 Do you have any specific questions? I'd love to help with them and start drafting the guide in the process.

deadcoder0904 commented 4 years ago

@hasparus basically if you include theme-ui in a project which has TS, for what things you add static typing? Like check out Overmind's Typescript guide

flo-sch commented 4 years ago

@hasparus any thought regarding merge() types?

Not TS expert here neither: what would be the recommended types to provide when overriding a preset (assuming all presets should be valid Theme instances...)?

import { merge, Theme } from 'theme-ui';
import { base as preset } from '@theme-ui/presets';

export const DefaultTheme: Theme = merge(
  preset as Theme, // is this safe? recommended?
  {
    colors: {
      background: 'white',
      text: 'black',
      primary: 'blue'
    }
  } as Partial<Theme>, // is this safe?
) as Theme; // is this needed?

https://codesandbox.io/s/theme-ui-typescript-theme-85jru?file=/src/CustomTheme.ts

hasparus commented 4 years ago

@hasparus basically if you include theme-ui in a project which has TS, for what things you add static typing? Like check out Overmind's Typescript guide

The perfect case answer would be

You don't need to worry about it, if you misspell any variable name or mix something up, TypeScript will tell you.

We ain't there yet. I hope we'll come closer to this experience with the stable version. Theme UI types in their most basic form should just work (TM). It should be as easy and seamless as using @emotion/core.

(@jxnblk correct me if I'm wrong)

A bit off-topic right now, but the Overmind guide you linked may be relevant one day. The declare module trick as seen in Overmind and fp-ts typeclass instances can be used to implement "TypeScript: Type check property values against theme scales" todo @jxnblk mentioned in https://github.com/system-ui/theme-ui/issues/832.

When we're using Theme UI in statically typed codebase, Ii's good to notice we have two concepts of theme, ergo two types.

1. The general idea of theme.

This one is interesting for libraries and for apps where user can edit the theme. This is the interface Theme defined in @theme-ui/css.

We can't read theme.fonts.monospace, because we have no idea if there is a fonts scale in the theme and even if its is, we don't know if it has anything under monospace key.

We can use optional chaining for fonts (theme?.fonts), but we can't read monospace because fonts could be an array. This is where get function from lodash or @theme-ui/css is helpful.

2. A particular theme, defined in the source code.

It's known at compile time, so we'd like to autocomplete keys from scales when the user writes their styles for nicer DX.

TS support for this is not implemented yet.
It will require some additional "overhead"
- either by injection of your type into theme-ui types, like in Overmind or fp-ts
- or by a "theme constructor function" on user side and generics plumbing through theme-ui code.

This is the type we get when we write

const theme = {
  fonts: {
    monospace: 'Fira Code'
  }
} as const

type MyTheme = typeof theme

(as const means "Hey TS, make the type here as narrow as possible!")

MyTheme is a subtype of Theme, i.e. all specific branded themes are assignable to Theme.

Notice we get no autocomplete inside the theme object in snippet above.

My clumsy fingers do at least two typos per second. This is a serious problem when I post on Twitter, but it doesn't have to be a problem in my source code. I solved it in my blog with a cleverly typed identity function.

import { Theme } from "theme-ui";

const makeTheme = <ExactTheme extends Theme>(t: ExactTheme): ExactTheme => {
  return t;
};

It allows me to say "my argument t is of type ExactTheme which is a subtype of the Theme I just imported". After we compile TS it's just identity, so it can be thrown out by an optimizer (e.g. Closure Compiler Advanced Optimizations).

const theme = makeTheme({
  space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
});

export default theme;
export type MyTheme = typeof theme;

We claim more control over the type of theme. The tradeoff is that we lose extra property checks. i.e.

/* Type '{ spaceX: number[]; }' is not assignable to type 'Theme'.
  Object literal may only specify known properties, but 'spaceX' does not exist in type 'Theme'.
  Did you mean to write 'space'? ts(2322) */
const theme1: Theme = {
  spaceX: [0, 4, 8, 16, 32, 64, 128, 256, 512],
}

/* totally okay, no error */
const theme2 = makeTheme({
  spaceX: [0, 4, 8, 16, 32, 64, 128, 256, 512],
})

hasparus commented 4 years ago

@flo-sch It's great you've found this. This is a bug! Even two!

The first problem is -- merge is typed as what it does and not what it's supposed to be used like. Guess who's on git blame 🙋. That's correct TypeScript, but it's lousy Public API. Sorry!

The signature is too universal I think.

export const merge = <T>(a: Partial<T>, b: Partial<T>): T =>
  deepmerge(a, b, { isMergeableObject, arrayMerge })

It isn't exactly documented as such on the webiste, but it would be useful to make it a domain specific deep merge function.

/**
 * Deeply merge themes
 */
export const merge = (a: Theme, b: Theme): Theme =>
  deepmerge(a, b, { isMergeableObject, arrayMerge })

We should also test @theme-ui/core in TypeScript. I just noticed we don't do it.

I removed your assertions and worked around the problem by passing generic parameter explicitly.

(This is uncool. In every place we pass a generic parameter explicitly or do assertions in TypeScript, we'd possibly have to live with broken editor support in JS files instead. We want our types to make JS developer experience better.)

export const CustomTheme = merge<Theme>(
  preset,
  {
    colors: {
      background: "white",
      text: "black",
      primary: "blue"
    },
    alerts: {
      ...VARIANTS
    },
    buttons: {
      ...VARIANTS
    },
    messages: {
      ...VARIANTS
    }
  }
)

Sweet. New error appears.

Argument of type >A_BIG_OBJECT_TYPE< is not assignable to parameter of type 'Partial<Theme>'.
  The types of 'styles.th' are incompatible between these types.
    Type '{ textAlign: string; borderBottomStyle: string; }' is not assignable to type 'SystemCssProperties | CSSPseudoSelectorProps | CSSSelectorObject | VariantProperty | UseThemeFunction | undefined'.
      Type '{ textAlign: string; borderBottomStyle: string; }' is not assignable to type 'CSSSelectorObject'.
        Property 'textAlign' is incompatible with index signature.
          Type 'string' is not assignable to type 'SystemStyleObject'.ts(2345)

It says that textAlign is a string, but it shouldn't be, and the last type the typechecker tried is SystemStyleObject. And what is the type of value we could pass as textAlign?

So it should be either one of these strings or a nested object which corresponds to this

textAlign {
  color: blue;
}

perfectly legal and entirely useless CSS :D

@flo-sch Do you wanna do a PR for some of this or should I do it?

flo-sch commented 4 years ago

Great explanations @hasparus, I never pay attention to those signature types... Need to become better at that 😅

it's lousy Public API. Sorry!

Don't be, the project is fantastic already, thanks for all your effort in there! An API cannot cover every single use-case from the start anyway.

Your screenshot makes me wonder though: is merge intended for external use then? In my case I think I would always override a specific preset, so I thought that would be the way to go, but perhaps there is something else that is better to use?

Anyway, if you don't mind, I would be interest by trying to help with that, it feels like a good TypeScript exercice for me :D But I will probably need help since I am not that familiar with neither MDX nor theme-ui types (yet). Could I tag you directly in the PR perhaps and we take it from there?

Just not sure what you mean by:

We should also test @theme-ui/core in TypeScript. I just noticed we don't do it.

And:

I removed your assertions and worked around the problem by passing generic parameter explicitly.

flo-sch commented 4 years ago

Also, since this is an issue about TypeScript, something I think I would be interested by is to ensure the variant props accepts only known variants (that are defined in the theme).

// This would be valid
<Button variant="primary">Click here</Button>

// This would not
<Button variant="someUndefinedVariantName">Click here</Button>

But since type definition is by nature static and the theme dynamic (provided from the context), I guess there is no smart way to achieve that?

hasparus commented 4 years ago

the project is fantastic already, thanks for all your effort in there!

That's Brent and the other guys. I just added errors :D

something I think I would be interested by is to ensure the variant props accepts only known variants (that are defined in the theme). But since type definition is by nature static and the theme dynamic (provided from the context), I guess there is no smart way to achieve that?

This could be part of this "strict mode" I think. We could typecheck this to some point if we knew what variants do you have. The hard part is the themeKey and path access:

  <Box variant="forms.input">
    <SearchIcon />
    <input {...inputProps} />
  </Box>

We can't do string operations on the typelevel so this is impossible without codegen.

I didn't read https://github.com/system-ui/theme-ui/pull/823 thoroughly. The problem is solvable, but not trivial.

Anyway, if you don't mind, I would be interest by trying to help with that, it feels like a good TypeScript exercice for me :D But I will probably need help since I am not that familiar with neither MDX nor theme-ui types (yet). Could I tag you directly in the PR perhaps and we take it from there?

Sure thing! I'd love that.

Just not sure what you mean by:

We should also test @theme-ui/core in TypeScript. I just noticed we don't do it.

and

I removed your assertions and worked around the problem by passing generic parameter explicitly.

So for the second sentence. I got rid of as Theme and passed Theme as T to merge. Notice that I called merge<Theme>(a, b). You can omit Theme here and let it be inferred from function arguments, what's often preferred.

For the first one: We didn't cover merge types in unit tests 😢 I'm not sure but I think we have a case that would catch your error.

TBH, I think it's not internal anymore :D It leaked.

My reasoning is:

It's exported.
There is merge mentioned in READMEs. Usually when it appears it's imported from lodash.
Since our merge is exported and typed, people can see an example and VSCode autoimport our merge instead of the one from lodash. I did it a few times!

Let's just fix the type, add a nice jsdoc comment, and describe it in the docs.

It's probably a nicer solution than stripping it from public typings or renaming to merge_internal_do_not_use.
It can help people avoid problems with other merge functions. For example. we don't expect arrays to be concatenated, and that's what deepmerge does by default.

This could be the "go-to theme merging function". Need to merge some themes? That's the one. One import from theme-ui away.

There is also a possibility that merging "design graphs" in a way that makes sense is actually harder than recursively merging objects and this also opens the way to think about it.

@jxnblk what do you think about making merge public?

jxnblk commented 4 years ago

what do you think about making merge public?

If it makes usage easier, I think it could safely be exposed as the "go-to theme merging function" like you mention -- it's only a small/configured wrapper around deepmerge so I don't think ongoing maintenance would be too much of a hassle

flo-sch commented 4 years ago

Great, I will give it a try then :)

deadcoder0904 commented 4 years ago

Hey, @hasparus I just installed @theme-ui/core@next & I couldn't import Theme from it. Where do I find Theme so I can use it like I used to with theme-ui:

import {Theme} from 'theme-ui'

export const theme: Theme = {
  fonts: {
    body:
      'system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", sans-serif',
    heading: '"Avenir Next", sans-serif',
    monospace: 'Menlo, monospace',
  },
  colors: {
    text: '#000',
    background: 'white',
    primary: '#bf1',
  },
  modes: {
    dark: {
      text: '#fff',
      background: '#000',
      primary: '#0cf',
    },
  },
}

deadcoder0904 commented 4 years ago

Another error I have is on ThemeProvider's theme:

On hover on theme, it shows the following error:

Type '{ breakpoints: string[]; space: number[]; fonts: { body: string; heading: string; monospace: string; }; fontSizes: number[]; fontWeights: { body: number; heading: number; bold: number; }; lineHeights: { body: number; heading: number; }; letterSpacings: { ...; }; colors: { ...; }; text: { ...; }; styles: { ...; }; }' is not assignable to type 'Theme | ((outerTheme: Theme) => Theme)'. Type '{ breakpoints: string[]; space: number[]; fonts: { body: string; heading: string; monospace: string; }; fontSizes: number[]; fontWeights: { body: number; heading: number; bold: number; }; lineHeights: { body: number; heading: number; }; letterSpacings: { ...; }; colors: { ...; }; text: { ...; }; styles: { ...; }; }' is not assignable to type 'Theme'. The types of 'styles.th' are incompatible between these types. Type '{ textAlign: string; borderBottomStyle: string; }' is not assignable to type 'ThemeUICSSProperties | CSSPseudoSelectorProps | CSSSelectorObject | VariantProperty | UseThemeFunction | undefined'. Type '{ textAlign: string; borderBottomStyle: string; }' is not assignable to type 'CSSSelectorObject'. Property 'textAlign' is incompatible with index signature. Type 'string' is not assignable to type 'ThemeUIStyleObject'.ts(2322) index.d.ts(25, 5): The expected type comes from property 'theme' which is declared here on type 'IntrinsicAttributes & ThemeProviderProps'

hasparus commented 4 years ago

Hey @deadcoder0904, Theme is in @theme-ui/css. We should probably reexport it from core.

How does the theme causing the error look like? It seems like a similar error to https://github.com/davidkpiano/xstate/issues/310. Does annotating your theme (theme: Theme = ...) help?

deadcoder0904 commented 4 years ago

@hasparus Then I'd have to install @theme-ui/css, right? Although it's bundled with @theme-ui/core do I need to install it separately?

The theme looks like:

import { Theme } from '@theme-ui/css'

export const theme: Theme = {
    breakpoints: ['640px', '768px', '1024px', '1280px'],
    space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
    fonts: {
        body:
            "system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, 'Noto Sans', sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji'",
        heading: "'Inter var', Georgia, Cambria, 'Times New Roman', Times, serif",
        monospace: "Menlo, Monaco, Consolas, 'Liberation Mono', 'Courier New', monospace",
    },
    fontSizes: [12, 14, 16, 20, 24, 32, 48, 64],
    fontWeights: {
        body: 400,
        heading: 700,
        bold: 700,
    },
    lineHeights: {
        body: 1.5,
        heading: 1.125,
    },
    letterSpacings: {
        body: 'normal',
        caps: '0.2em',
    },
    colors: {
        text: '#000000',
        background: '#ffffff',
        primary: '#87EAF2',
        secondary: '#F0F4F8',
        accent: '#F29B9B',
        highlight: '#F7D070',
        muted: '#BED0F7',
    },
    text: {
        heading: {
            fontFamily: 'heading',
            lineHeight: 'heading',
            fontWeight: 'heading',
        },
    },
    styles: {
        root: {
            fontFamily: 'body',
            lineHeight: 'body',
            fontWeight: 'body',
        },
        h1: {
            variant: 'text.heading',
            fontSize: 5,
        },
        h2: {
            variant: 'text.heading',
            fontSize: 4,
        },
        h3: {
            variant: 'text.heading',
            fontSize: 3,
        },
        h4: {
            variant: 'text.heading',
            fontSize: 2,
        },
        h5: {
            variant: 'text.heading',
            fontSize: 1,
        },
        h6: {
            variant: 'text.heading',
            fontSize: 0,
        },
        pre: {
            fontFamily: 'monospace',
            overflowX: 'auto',
            code: {
                color: 'inherit',
            },
        },
        code: {
            fontFamily: 'monospace',
            fontSize: 'inherit',
        },
        table: {
            width: '100%',
            borderCollapse: 'separate',
            borderSpacing: 0,
        },
        th: {
            textAlign: 'left',
            borderBottomStyle: 'solid',
        },
        td: {
            textAlign: 'left',
            borderBottomStyle: 'solid',
        },
    },
}

hasparus commented 4 years ago

@hasparus Then I'd have to install @theme-ui/css, right? Although it's bundled with @theme-ui/core do I need to install it separately?

You should. ESLint will yell at you but the import will work. I'd install it though.

The theme looks like:

Huh, this is weird then. Both theme variable and theme prop are typed as Theme. TBH I have no idea what could cause this, maybe except a version mismatch.

deadcoder0904 commented 4 years ago

You should. ESLint will yell at you but the import will work. I'd install it though.

I think it's weird that I have to install @theme-ui/css just to get a type definition of Theme. Feels weird.

If it helps, I just installed all packages from @theme-ui/* & all packages are @next.

deadcoder0904 commented 4 years ago

The red squiggly line on theme is now gone & now I have that same line on components with an error:

Type '{ children: Element[]; theme: Theme; components: { h1: (props: HTMLAttributes) => Element; }; }' is not assignable to type 'IntrinsicAttributes & ThemeProviderProps'. Property 'components' does not exist on type 'IntrinsicAttributes & ThemeProviderProps'

theme-ui

I didn't do anything to make it go away. Just closed my laptop & slept & re-opened today to check the error was gone.

Also, I did do Restart TS server multiple times today & yesterday but that definitely didn't solve it.

The mdComponents referenced in the above image refers to:

components/mdx/index.ts

import { H1 } from './H1'

export const mdComponents = {
  h1: H1,
}

components/mdx/H1.tsx

import { HTMLAttributes } from 'react'

export const H1 = (props: HTMLAttributes<HTMLHeadingElement>) => (
  <h1 style={{ color: 'tomato' }} {...props} />
)

So I'm not sure what's the problem here?

hasparus commented 4 years ago

The mdComponents referenced in the above image refers to:

Oh, this one may actually be right. Did you import your ThemeProvider from @theme-ui/core?

There are two ThemeProviders.

One is in @theme-ui/core.
- it accepts theme prop and children,
- passes your theme to EmotionContext and ThemeUIContext
The other one is in @theme-ui/theme-provider.
- accepts theme, children, and components props,
- renders the provider from @theme-ui/core,
- brings in ColorModeProvider
- applies styles.root styles to with BodyStyles
- passes components to MDXProvider

The second one is reexported from theme-ui.

deadcoder0904 commented 4 years ago

Oh I imported (1) from @theme-ui/core 🤦‍♂️

Goddamn, was confused for a long time. I don't think this was in the docs. Maybe a revamp is in the works?

lachlanjc commented 3 years ago

Closing as a discussion/several issues mentioned here have been solved

system-ui / theme-ui

Usage of Theme UI with TypeScript #834

1. The general idea of theme.

2. A particular theme, defined in the source code.

components/mdx/index.ts

components/mdx/H1.tsx