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 global typescript theme definition #1453

Merged
merged 1 commit into from
Feb 15, 2021
Merged

support global typescript theme definition #1453

merged 1 commit into from
Feb 15, 2021

Conversation

k-yle
Copy link
Member

@k-yle k-yle commented Feb 14, 2021

This PR fixes a number of things that make react-jss's createUseStyles less convenient for TypeScript users.

The problems:

  1. Defining createUseStyles is tedious because you need to pass the theme as a type argument every time, which has to be imported from somewhere (createUseStyles<MyTheme>(theme => { ... }))
  2. If you supply a type argument, then you lose one of the most important type safety features - the typecheck of the class names:
    a
    This is because, if you supply a type argument, the union type ("myHeader" | "myFooter" in the screenshot) just becomes string.
    ­
    To fix this, you have to duplicate every class name, and now the code looks horrendous:
    createUseStyles<MyTheme, "myHeader" | "myFooter">(theme => ({ 
      myHeader: { margin: 0 },
      myFooter: { padding: 0 },
    }))
  3. For JavaScript users, or mixed TS/JS users, there is no way to get auto-complete (intellisense) for the theme. Because the createUseStyles<MyTheme>() syntax is not possible in JS files.

This PR fixes these 3 issues.

Anyone (TypeScript and JavaScript users) can create a file called global.d.ts anywhere in their project. It is not imported by anything; TypeScript and/or VSCode's TSServer finds it automatically. It could like this:

declare global {
  namespace Jss {
    export interface Theme {
      themeColour: string;
    }
  }
}

export {};

Now, when call createUseStyles() you don't need to provide any type arguments to get type definitions for the theme, and the class names (see screenshot above) are automatically inferred by typescript without defining them separately.

I have added tests to confirm that this works and that TypeScript produces errors if you make a typo in the classes object or the theme object.

Todo

  • Add test that verifies the modified behaviour
  • Add documentation if it changes public API

@k-yle k-yle requested a review from kof as a code owner February 14, 2021 00:42
@kof kof requested a review from moshest February 14, 2021 00:45
@moshest
Copy link
Member

moshest commented Feb 14, 2021

Most users will be lazy and will not create the Theme interface. How the library going to behave in this case?

Copy link
Member

@moshest moshest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@k-yle
Copy link
Member Author

k-yle commented Feb 14, 2021

hi @moshest, if users don't extend the JSS.Theme interface, it will just be an empty interface {}

this matches the old behaviour, so it won't be a breaking a change.

@kof kof closed this Feb 15, 2021
@kof kof reopened this Feb 15, 2021
@kof kof merged commit 56205c1 into cssinjs:master Feb 15, 2021
@kof
Copy link
Member

kof commented Feb 15, 2021

merged, thanks!

@kof
Copy link
Member

kof commented Feb 15, 2021

will be released with the next release, we aggregate a bit

@k-yle k-yle deleted the typescript-theme branch February 15, 2021 20:43
@k-yle k-yle mentioned this pull request Mar 9, 2021
2 tasks
@kof
Copy link
Member

kof commented Mar 14, 2021

Released in v10.6.0

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

Successfully merging this pull request may close these issues.

3 participants