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

Typescript support #121

Closed
IniZio opened this issue Jun 23, 2019 · 58 comments
Closed

Typescript support #121

IniZio opened this issue Jun 23, 2019 · 58 comments

Comments

@IniZio
Copy link

IniZio commented Jun 23, 2019

I used rebass before and really like this new idea, especially since it allows even more flexibility.

Not sure if you prefer adding definition files only or refactoring into typescript? I can make pr for either approach 🤔

@PaulieScanlon
Copy link

i'd like to help with this as i'm having an issue with no type definitions. I've bodge a fix locally but adding an index.d.ts with declare module "theme-ui" to the dist folder in my local environment... This is probably bad but if someone can give me some pointers about where / how to add the types i'd be happy to help.

@jxnblk
Copy link
Member

jxnblk commented Jun 28, 2019

I think we'd be open to add definition files for TS, if anyone wants to start a small PR for the core package. A rewrite is probably out of scope for the time being, but we'd like to make sure this works well for TS users generally

@erikdstock
Copy link

erikdstock commented Jun 30, 2019

Also happy to help start on type definitions!

@erikdstock
Copy link

I started on something here. PRs or help welcome- I've never contributed to DT before.

@sebald
Copy link

sebald commented Jul 3, 2019

@erikdstock I can help you to get your first PR through to DT, if you like! 🙂 Can you make a PR against your own repo to open the code for comments?

here are some quick remarks:

  • Write tests! They're super helpful to get the PR through.
  • Add // TypeScript Version: 3.1 to top of the page, otherwise the liter can not parse csstype
  • styled-system__css -> @styled-system/css and add it as a dependency like you did with csstype. If DT can not resolve it you have to add a paths config to the tsconfig.json
  • If you want to support the sx prop, you have to add some global definitions, like emotion did with the css prop:

Emotion definitions:

declare module 'react' {
  interface DOMAttributes<T> {
    css?: InterpolationWithTheme<any>
  }
}

declare global {
  namespace JSX {
    /**
     * Do we need to modify `LibraryManagedAttributes` too,
     * to make `className` props optional when `css` props is specified?
     */

    interface IntrinsicAttributes {
      css?: InterpolationWithTheme<any>
    }
  }
}

@erikdstock
Copy link

erikdstock commented Jul 4, 2019

Thanks @sebald - On advice from a friend who has written lots of types I'd moved from working on these in pure theory in the DT repo to actually beginning to use them in a project (or rather, attempting a migration from rebass + styled-components on a new branch with typings/theme-ui.d.ts). However I've gotten stuck working with theme-ui on a number of points, so will take my work in its current state back to DT and make the PR as you suggested.

Edit: by supporting the sx prop, do you mean across all react components (assuming the custom pragma is loaded)? If so is it possible to test for the pragma? My current code was a bit more optimistic and just looked something like this:

  export const jsx: typeof React.createElement

  interface SxProps {
    sx: SystemStyleObject
  }

  type SxComponent = React.ComponentClass<SxProps>
  export const Box: SxComponent
  export const Container: SxComponent
  export const Flex: SxComponent
  export const Header: SxComponent
  export const Styled: { [k: string]: SxComponent }

@sebald
Copy link

sebald commented Jul 4, 2019

Not that I know of. Which is actually kinda annoying. I am currently working on a code base using emotion and the css prop is available everywhere ... at least TypeScript thinks that. This also makes it harder to overwrite the css prop or not allow people to use it on a custom component (where it is not processed at all).

@erikdstock
Copy link

Alright, I've got a start here.
erikdstock/DefinitelyTyped#1

@LekoArts
Copy link
Collaborator

Hi @erikdstock 👋 Would be super cool to have the types! Anything we can do to help you?

@erikdstock
Copy link

Hi @LekoArts - I'd love to keep working on this but got sidetracked as I didn't have an immediate use case for theme-ui. Is there a way we could collaborate on this through the branch I started and then submit it as one PR? I've never added a package to DT before so don't know how to determine when it is 'done.'

@LekoArts
Copy link
Collaborator

LekoArts commented Aug 2, 2019

I never added a package to DT, too! Maybe also @sebald , @IniZio or @PaulieScanlon can help with that – together we should be able to get this shipped 💪
Or we ask on Twitter if people want to help 👍

@wKovacs64
Copy link
Contributor

Activating the bat signal on Twitter sounds like a good idea.

@erikdstock
Copy link

I could stand up a simple hello world gatsby site, which we'd add types to through real use (filling the site with nonsense), and then we could move the typings into a @types pr. This was based on some advice I got early on that it's best to add types through real use than simply trying to catalog them.

@ifiokjr
Copy link

ifiokjr commented Aug 6, 2019

@erikdstock I really hope this doesn't step on any toes, but I was in urgent need of theme-ui in a TypeScript project so I used your types as a starting point and added the definition to (DefinitelyTyped/DefinitelyTyped#37210). It's not perfect but I am using it in a project right now without issues.

yarn add @types/theme-ui

Since most of it was your work your name is down as the main contributor.

I've got a few other types for the other libraries and I may not get around to doing a PR for them for a few days at least. If anyone wants to use them as a starting point or create a PR please feel free.

Code is available here.

@LekoArts
Copy link
Collaborator

LekoArts commented Aug 6, 2019

I personally think that's really cool, thanks for writing the definitions.

I'd say: Let's just try the package and see if we encounter issues. What is still missing in the definitions in your opinion @ifiokjr ?

Anyone interested in putting this information into the docs?

@ifiokjr
Copy link

ifiokjr commented Aug 6, 2019

What is still missing in the definitions in your opinion @ifiokjr ?

@LekoArts we should add a typography?: any; on the Theme interface export and better inline documentation for each export. I gleaned what I could but there are still parts of the API I didn't fully understand. I personally find that descriptive documentation within definition files are incredibly useful when diving into a new codebase.

Also right now the code supports nested tags:

const Success = () => (
  <div
    sx={{
      'body > div': {
        ':hover': {
          color: 'red',
        },
      },
    }}
  />
);

But not when the nested tag is within a media query:

const Failure = () => (
  <div
    sx={{
      '@media only screen and (max-width: 600px)': {
        'body > div': {
          ':hover': {
            color: 'red',
          },
        },
      },
    }}
  />
);

@erikdstock
Copy link

Hi and thank you @ifiokjr! not stepping on my toes at all, it was a side project that I couldn't follow through with- but i'll be happy to help maintain in the future. Glad my start was of some help.

@sleepyfran
Copy link

Hey @ifiokjr, thanks for the types! It's working pretty well but I found some parts that are either not implemented yet or I didn't figure out how to use them. Can I use the ColorMode and InitializeColorMode components? I had to do this little trick to make it work:

import * as themeUi from 'theme-ui'
const _ColorMode = require('theme-ui').ColorMode
const _InitializeColorMode = require('theme-ui').InitializeColorMode

declare module 'theme-ui' {
    export const ColorMode = _ColorMode
    export const InitializeColorMode = _InitializeColorMode
}

I also can't seem to make Styled tags work, since something like:

<Styled.a href="/">Test</Styled.a>

Generates the following error: Property 'href' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<SxProps, any, any>> & Readonly & Readonly<{ children?: ReactNode; }>'.

Is there anyway to make this work?

@ifiokjr
Copy link

ifiokjr commented Aug 13, 2019

@sleepyfran I've added a PR here DefinitelyTyped/DefinitelyTyped#37578

Test it out by copying the file contents from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d2f29c7d1eae3eee7a33b94cd6147a5c7aee95cd/types/theme-ui/index.d.ts

into

declare module 'theme-ui' {
    // New declaration here
}

and remove the current @types/theme-ui installation.

@sleepyfran
Copy link

Wow, that was fast, thanks @ifiokjr! Just checked it with the revision and it's working perfectly.

@sbardian
Copy link

sbardian commented Aug 21, 2019

Theme.colors.modes, ColorModes, doesn't appear to be typed correctly. I'm getting an error when trying to define my different theme.colors.modes. Shouldn't the ColorModes text and background values be nested under the mode name (dark/etc)?

would something like this work?

export interface ColorModes {
  [k: string]: {
    /**
     * This is required for a color mode.
     */
    text: string;

    /**
     * This is required for the color mode.
     */
    background: string;
    [k: string]: Partial<Omit<StyledSystemTheme['colors'], 'modes'>>;
  };
}

@erikdstock
Copy link

@sbardian (& everyone else)- as this issue is now technically resolved with the addition of @types/theme-ui would it make sense to close it and begin raising followup issues in https://github.com/DefinitelyTyped/DefinitelyTyped? You can see the current contributors for tagging at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/theme-ui/index.d.ts .

That said I do think you are right @sbardian - would you be able to make a PR?

@sbardian
Copy link

Will do! thanks @erikdstock

@jpdenford
Copy link

Fantastic @ifiokjr . I had the same issue, installed the @types/theme-ui and my sx props are working like a charm. Thanks very much to those who worked on this!

@jonavila
Copy link

@sbardian @erikdstock Hey guys, thanks so much for the theme-ui typings. I've got a quick question... Right now, I'm able to use the sx prop, but it's not typed checked. It seems like I can throw anything at the object styles and not generate an error. Here's how I've defined my component:

/** @jsx jsx */
import { Button as BlueprintButton, IButtonProps } from '@blueprintjs/core';
import { ButtonHTMLAttributes, FunctionComponent } from 'react';
import { jsx } from 'theme-ui';

export const Button: FunctionComponent<IButtonProps & ButtonHTMLAttributes<HTMLButtonElement>> = props => (
    <BlueprintButton
        {...props}
        sx={{
            lineHeight: '1em',
            boxSizing: 'border-box',
            boxShadow: 'unset',
            backgroundImage: 'none',
            variant: `buttons.${props.intent}.default`,

            '&:active, &:focus': {
                variant: `buttons.${props.intent}.active`,
                outline: 'none',
            },

            '&:hover': {
                variant: `buttons.${props.intent}.hover`,
            },
        }}
    />
);

Any thoughts on what might be going on?

@wKovacs64
Copy link
Contributor

@jonavila I chased that down at one point a while ago and discovered it's not limited to theme-ui or sx, it's an issue with Emotion's object styles, too. If I remember right, emotion-js/emotion#1267 is what's up.

TL;DR: it currently accepts any string to allow for nested selectors.

@erikdstock
Copy link

@jonavila @wKovacs64 would it be advisable to narrow the type for the sx prop? I recently made my own library inspired by theme-ui (essentially theme-ui without the explicit tie to mdx) and just called sx a SystemStyleObject

@jonavila
Copy link

@jonavila @wKovacs64 would it be advisable to narrow the type for the sx prop? I recently made my own library inspired by theme-ui (essentially theme-ui without the explicit tie to mdx) and just called sx a SystemStyleObject

@erikdstock That seems to work better, but maybe with the caveat that you can't type check when nesting more than one level deep?

Also, unrelated but missing in the types is the css utility describe here:
https://theme-ui.com/api#css

I'll create an issue in DefinitelyTyped

@wKovacs64
Copy link
Contributor

Using SystemStyleObject directly doesn't allow for combined pseudo-selectors like ':hover, :focus', either. Guessing that's why they went with string. Might be able to tighten that up, though?

@LekoArts
Copy link
Collaborator

LekoArts commented Jan 23, 2020

Some fixes were mentioned here, some in the recently closed issues. It would be awesome if those would get PR'ed upstream to DefinitelyTyped so that everyone can benefit from them :)

Like css from theme-ui, or Styled.a or the other things.

@hasparus
Copy link
Member

hasparus commented Jan 27, 2020

There's a bunch of changes in v0.3 (https://theme-ui.com/migrating).

Since tracking issues in DT may be a bit cumbersome, we could try to list needed changes here alongside problems @LekoArts mentioned.

An incomplete list of current issues I know about. Ping me or copy-paste it to add more?


BTW @jxnblk, I do realize I may sound like a door-to-door salesman (or like this), but would you be open to adopt TypeScript in simpler packages in the monorepo?

@theme-ui/preset-future is a good example.

  • The package is one object export, so "adopting TypeScript" means changing the extension to .ts and that's it.
  • microbundle supports TypeScript with one additional property in package.json.
    "types": "dist/index.d.ts",
    
  • @types/theme-ui__preset-future doesn't exist in DefinitelyTyped.
    There's a high chance someone will open an issue about it here.
    If we add it to DT and it gets out of sync, there's still a chance people will open issues about it here instead of DT. DefinitelyTyped has 3k open issues, what may be a bit scary 🤷

I'd be happy to help.

@hasparus
Copy link
Member

There's no "question" issue template, so I'll ask it here, because it's a bit connected.

Are get and merge public API? They're mentioned in the docs, but they're described as used internally.
Can I can use merge to extend themes and expect it to still be there in the patch update? Or is it internal and exported for convenience?

@isBatak
Copy link

isBatak commented Feb 10, 2020

@hasparus Could you add __css and __themeKey props to BoxOwnProps interface?

__themeKey?: string;
__css?: SxStyleProp;

@hasparus
Copy link
Member

I’ve been thinking about it, @isBatak.
It looks like a private API and I’m afraid of exposing it in types. I’d love to discuss it.

Are you using this in an app, or building a library?

@isBatak
Copy link

isBatak commented Feb 10, 2020

I'm using it for core components, I treat them as atoms for building other higher-order components. I'm building an app but I'm thinking about core components as a UI library.
Also, with __themeKey I can easily extend the Theme definition with new namespaces.

Maybe you could add a doc comment above those props:

/**
     * For internal use!
     * @internal
     */
    __themeKey?: string;
    /**
     * For internal use!
     * @internal
     */
    __css?: SxStyleProp;

@hasparus
Copy link
Member

hasparus commented Feb 10, 2020

Isn’t what you’re doing possible with jsx pragma or spreading received sx prop?

@isBatak
Copy link

isBatak commented Feb 10, 2020

yes, it is... I'm doing that in higher-order components, spreading sx props.
But for atomic it's more intuitive to use __css and there is no other way to extend theme except using a __themeKey.

@hasparus
Copy link
Member

hasparus commented Feb 10, 2020

Would it be okay for you to wait for more input on this?

I'd use an assertion and lock the version of theme-ui if I were you.
PRs to DefinitelyTyped take some time to merge, so we'd have to wait a bit either way.

I can think of few reasons not to include __css in the types.

  1. Precedent: __SECRET_INTERNALS are omitted from [@types/react]
    (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts).
  2. TypeScript has a stripInternal compiler option which strips properties marked with @internal from the declarations.
    I've made a repo with an example: https://github.com/hasparus/repro--ts-split-internal
  3. API Extractor doesn't include internals in .d.ts rollup.

I am not super strongly opposed to the idea. I'd just try to avoid doing it.

@isBatak
Copy link

isBatak commented Feb 10, 2020

I could leave without __css and spread sx, that's fine... but for __themeKey I don't see an alternative.

I found another problem...
It seems like Assign type is not working as expected.
For example:

import React, { forwardRef } from 'react';
import { Flex, BoxOwnProps } from '@theme-ui/components';

export interface IHeaderProps extends BoxOwnProps {
  title?: string;
}

export const Header = forwardRef<undefined, IHeaderProps>(({ title, sx, ...rest }, ref) => {
    return (
      <Flex
        ref={ref}
        sx={{ alignItems: 'center', px: 7, pt: 5, pb: '12px', borderBottom: '1px solid', ...sx }}
        {...rest}
      >

I get this TS error:

Type '{ children: Element[]; as?: ElementType<any>; variant: string; css?: InterpolationWithTheme<any>; m?: ResponsiveValue<string | number | symbol, Required<Theme<TLengthStyledSystem>>>; ... 32 more ...; sx: { ...; } | ... 3 more ... | { ...; }; }' is not assignable to type 'HTMLAttributes<HTMLDivElement>'.
  Types of property 'color' are incompatible.
    Type 'ResponsiveValue<string | number | symbol, Required<Theme<TLengthStyledSystem>>>' is not assignable to type 'string'.
      Type 'number' is not assignable to type 'string'.ts(2322)

It seems like color type from React.ComponentProps<'div'> is used instead color props from BoxOwnProps (extended ColorProps).

Also, it would be cool if we could do this

<Box<React.ComponentProps<'a'>> 
    as="a" 
    href="#" 
    sx={{ '&:hover': { color: 'primary' } }}
/>

...and get correct typings for href props or similar.

@jxnblk
Copy link
Member

jxnblk commented Feb 10, 2020

FWIW The __css and __themeKey props are private and might be removed in future releases. They are not documented and should not be used

get and merge can be considered "public" since they are documented and will not be removed without a major/breaking release

@hasparus
Copy link
Member

hasparus commented Feb 10, 2020

I am not sure <Box<"a">> isn't out of scope for DefinitelyTyped. I think I had problems passing dtslint.
We can trying pair programming on it this weekend if you'd like to @isBatak.
I can also test your PR on my projects, where I use theme-ui to see if anything blows up.

Reakit has correctly typed as prop.

Andrew Branch from TypeScript team has a nice blogpost about it: https://blog.andrewbran.ch/polymorphic-react-components/


color problem

BoxOwnProps extends SpaceProps and ColorProps, but it has nothing to do with "div".

BoxProps adds ComponentProps<"div">.

export interface BoxProps extends Assign<React.ComponentProps<'div'>, BoxOwnProps> 

Notice that your error persists if you change IHeaderProps to ColorBoxProps

export interface ColorBoxProps {
    color: object;
}

Changing BoxOwnProps to BoxProps should fix it for you.

export interface IHeaderProps extends BoxProps {
    title?: string;
}

export const Header = forwardRef<HTMLDivElement, BoxProps>(({ title, sx, ...rest }, ref) => {
    return (
        <Flex
            ref={ref}
            sx={{ alignItems: 'center', px: 7, pt: 5, pb: '12px', borderBottom: '1px solid', ...sx }}
            {...rest}
        />
    );
});

@hasparus
Copy link
Member

As of __themeKey, if I were you I'd add a similar behavior in my library.

pseudocode:

import { css, get } from '@theme-ui/css'

const myVariant = ({ theme, variant, __myThemeKey }) =>
  css(get(theme, __myThemeKey  + '.' + variant, get(theme, variant)))

It would still work when @jxnblk removes or renames __themeKey.

@hasparus
Copy link
Member

@isBatak Not sure if it will help you, but instead of as prop, I'm using styles from theme-ui like this.

example component

import { Link as BaseLink, LinkProps as BaseLinkProps } from 'my-router-of-choice';

export interface LinkProps extends BaseLinkProps {
  variant?: "button";
}
export const Link = ({ variant, ...rest }: LinkProps) => {
  const { theme } = useThemeUI();
  const variantStyles = variant === "button" ? theme.buttons.primary : {};

  return <BaseLink sx={variantStyles} {...rest} />;
};

theme.tsx

declare module "theme-ui" {
  export interface Theme {
    useCustomProperties?: boolean;
    forms: {
      textarea: themeUi.SxStyleProp;
    };
  }
  
  // app-level code: narrow the type of value returned from context to 
  // exactly my theme shape
  // eslint-disable-next-line @typescript-eslint/no-use-before-define
  export function useThemeUI(): { theme: typeof theme };
}

const makeTheme = <T extends themeUi.Theme>(t: T): T => t;

export const theme = makeTheme({
  // redacted
  buttons: {
    primary: {
      // redacted
    },
});

@isBatak
Copy link

isBatak commented Feb 10, 2020

@hasparus thx for everything :D
I need time to digest all of the information...
Although, I tried color problem fix and it worked like a charm. I thought I tried using BoxProps...
Regarding <Box<"a">> I'll try some ideas over the weekend.

And @jxnblk thx for clearing out things with __css and __themeKey :D

@jonavila
Copy link

@hasparus The current types have the following:

declare module 'react' {
    // tslint:disable-next-line: no-empty-interface
    interface DOMAttributes<T> extends SxProps {}
}

declare global {
    namespace JSX {
        // tslint:disable-next-line: no-empty-interface
        interface IntrinsicAttributes extends SxProps {}
    }
}

For whatever reason in my environment, the sx prop only works with Components but not with jsx elements like div. I got it to work by extending React's Attributes interface instead of extending DOMAttribute. React's IntrinsicAttributes already extends Attributes, so the following should simplify things

declare module 'react' {
    interface Attributes extends SxProps {}
}

This is what styled-components do for their typing of the css prop and I think it avoids having to mess around with the JSX namespace directly.

Thoughts?

@allanpope
Copy link
Contributor

Is it worth adding types to DefinitelyTyped for @theme-ui/color?

Since it's a wrapper around Polished I was able to get it working by adding this to my local definitions file.

declare module "@theme-ui/color" {
  export * from "polished"
}

@hasparus
Copy link
Member

Is it worth adding types to DefinitelyTyped for @theme-ui/color?

It is certainly not needed for savvy TypeScript users, @allanpope :)
However, adding a declaration to DT would make Automatic Type Acquisition (i.e. DX for JS users) work. I suppose they would be also easier to discover (without reading the source code).

@hasparus
Copy link
Member

hey @jonavila 👋

I took a quick look at it to figure out why it's done this way.

I think theme-ui JSX handling is inspired by what emotion does.
emotion-js/emotion/packages/core/types/index.d.ts#L82-L99
I hope it won't be rude if we ask the original author (@ifiokjr).

Merging sx (and css) into DOMAttributes seems superfluous at first.

image

We add this prop to the first parameter of React.createElement and it just won't be handled.
But if we look at DefinitelyTyped/DefinitelyTyped/blob/master/types/theme-ui/index.d.ts#L116-L121, we
can see

export const jsx: typeof React.createElement;

So this is why adding sx to DOMAttributes is needed.
The typings lie a bit — probably to reduce complexity (createElements signature is 36 lines in @types/react, for 100% jsx function typings would have about the same size).
JSX is pretty popular, so small inconsistency in createElement and jsx functions is hard to notice.

I think we could go two ways:

  1. for brevity we can add SxProps to React.Attributes as you mentioned
  2. for correctness we would have global.JSX.IntrinsicAttributes and properly typed jsx function

I think this is connected with #633 and a quick description how to tell TypeScript about your JSX pragma (regardless of framework) could be a nice addition to the README (some day?).

@allanpope
Copy link
Contributor

Is it worth adding types to DefinitelyTyped for @theme-ui/color?

It is certainly not needed for savvy TypeScript users, @allanpope :)
However, adding a declaration to DT would make Automatic Type Acquisition (i.e. DX for JS users) work. I suppose they would be also easier to discover (without reading the source code).

Good point on Automatic Type Acquisition @hasparus

I've made a pr for @theme-ui/color. Turns out a few of the Polished JS functions were renamed in Theme UI, so was worth doing.

@mxstbr
Copy link
Member

mxstbr commented Feb 13, 2020

👋 We are working on converting the entire theme-ui codebase to TypeScript and automatically generating the typedefs from the code to make TypeScript support a first-class citizen. Thanks to your excellent typedefs in DefinitelyTyped, this is turning out to be much simpler than I thought! 💯

We would love to have some more 👀 from y'all with way more TypeScript experience here: #662 (comment)

The plan is to ship the conversion of the @theme-ui/core package first so we can iron out any issues with the build process etc. I'll then write out a more detailed plan/todo list of all the packages that exist and need to be converted and would love to see y'all contribute ❤️

@mxstbr mxstbr mentioned this issue Feb 14, 2020
32 tasks
@mxstbr
Copy link
Member

mxstbr commented Feb 14, 2020

The initial spike #662 was merged! 🎉 We would love your help to convert the codebase to TypeScript, so I opened up a coordination issue: #668 Please chime in there if you have some time to spare to convert some packages 🙏

I am going to close this issue now. Thank so much to all of you for working on this since the initial release, we can't wait to ship first class TypeScript support now 💜

@danvim
Copy link

danvim commented Mar 5, 2020

FWIW The __css and __themeKey props are private and might be removed in future releases. They are not documented and should not be used

get and merge can be considered "public" since they are documented and will not be removed without a major/breaking release

@jxnblk Is it possible to add a __themeKey alternative? I believe it is a much needed feature as it has different specificity when it comes to variant. I am currently using both to accomplish some styles.

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

No branches or pull requests