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 for custom theme types via TypeScript module augmentation #5579

Merged

Conversation

jrolfs
Copy link
Contributor

@jrolfs jrolfs commented Feb 14, 2022

📝 Description

Here I'm following up on a previous idea I had mentioned in this PR. The TypeScript experience when distributing a component library based on Chakra still isn't great on account of the Chakra CLI step being required for consumers to the library to get the proper language server/editor experience. Not only is it an extra step, but there are multiple ways in which it can break preventing consumers of our library from getting accurate type definitions.

In this PR, I'm proposing a small change to the way that these custom types are defined that both allow the existing CLI functionality to continue working exactly as it had previously, but also providing an optional advanced feature that would allow custom type definitions to be shipped with component libraries that are based on Chakra that would not require end users to install and run the CLI. It also makes it possible to manually customize the type definitions, if desired.

⛳️ Current behavior (updates)

Defining custom theme typings relies on the @chakra-ui/cli package always running and overwriting the ThemeTypings interface in the end users' ./node_modules/ directory as documented here.

🚀 New behavior

This PR makes it possible to also customize the ThemeTypings interface using TypeScript's module augmentation and declaration merging features, much like custom theme types are implemented in Styled Components and Emotion.

In order to take advantage of this approach, the CustomThemeTypings interface needs to be "augmented" and at least implement the DefaultThemeTypings interface, otherwise it will fall back to the existing ThemeTypings interface that the CLI overwrites.

Example types/chakra.d.ts:

import "@chakra-ui/styled-system";

type DefaultSizes = 'small' | 'medium' | 'large';

declare module "@chakra-ui/styled-system" {
  export interface CustomThemeTypings {
    // Example custom `borders` tokens
    borders: 'none' | 'thin' | 'thick';
	// ...
	// Other custom tokens (everything in `DefaultThemeTypings` must be defined here)
	// ...
    components: {
      Button: {
        // Example custom component sizes and variants
        sizes: DefaultSizes;
        variants: 'solid' | 'outline' | 'wacky' | 'chill';
      };
      // ...
     }
  }
}

💣 Is this a breaking change (Yes/No):

No, see above.

📝 Additional Information

In it's current form, this PR simply makes this possible and adds some rudimentary inline documentation. I think it would be reasonable (barring maybe some feedback on how I named things) to merge this PR as is. With that said, I am very much ready to add documentation explaining the feature and I was even thinking we could combine this approach with the CLI with a new option to ouput a version of the definitions that leverages the above approach as opposed to modifying the definitions directly in ./node_modules.

In addition to making it possible to include custom type definitions with a component library that's based on Chakra, I think avoiding messing around in ./node_modules/ has some pretty clear benefits. Let me know what y'all think!

@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2022

🦋 Changeset detected

Latest commit: 3c3ca6b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@chakra-ui/styled-system Minor
@chakra-ui/system Patch
@chakra-ui/props-docs Patch
@chakra-ui/provider Patch
@chakra-ui/react Patch
@chakra-ui/skeleton Patch
@chakra-ui/test-utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Feb 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/chakra-ui/chakra-ui-storybook/3PbTC6znSpnVCyvXLJN8soW6RPFJ
✅ Preview: https://chakra-ui-storybook-git-fork-jrolfs-feature-th-287c30-chakra-ui.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 14, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3c3ca6b:

Sandbox Source
create-react-app-ts Configuration

@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 15, 2022

Oops, I forgot a changeset. I added one and updated the inline documentation to extend BaseThemeTypings. Do y'all have any feedback for me here? I'm happy to open a corresponding PR against the documentation site as well.

Thank you!

@TimKolberger
Copy link
Contributor

Love this PR! 💖

I'll review it over the weekend, because I have currently only access to my phone..

I am excited! 🎉

@TimKolberger
Copy link
Contributor

TimKolberger commented Feb 17, 2022

Hi @jrolfs,
I tried to test this feature in a new CRA project, with the built and linked chakra packages from this PR, but I failed.
The example app is available here: https://github.com/TimKolberger/chakra-5579 where I described my setup in the README and I appended it here.

I just got autocomplete from the default theme and not the augmented types (IDEA nor VSCode).

image

Can you kindly give me hint how to test this?

cd <chakra-repo>
# checkout PR branch and build project
cd <chakra-repo>/packages/react
yarn link
cd <chakra-repo>/packages/styled-system
yarn link

cd <this-repo>
yarn link "@chakra-ui/react"
yarn link "@chakra-ui/styled-system"

# try autocomplete in /src/App.tsx

@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 18, 2022

I just pulled down your repo and linked in my existing Chakra branch and it seems to be working in Code.

cra

I'll dig in a bit more tomorrow and make sure I didn't forget something in the PR.

I was hoping to be able to just set up a CodeSandbox with the PR releases to test and demo this, but now that I look I don't see those happening on PRs anymore? I thought every PR got released on the PR dist-tag?

@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 18, 2022

Here's an example of the PR releases I was talking about: #5290 (comment)

@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 18, 2022

Aha, I forgot to export CustomThemeTypings from the index of @chakra-ui/styled-system! I amended my commit and pushed that up. You should be able to just pull down this PR again and rebuild with that project linked.

@TimKolberger
Copy link
Contributor

TimKolberger commented Feb 18, 2022

Awesome! I'll try it this evening.

Here's an example of the PR releases I was talking about: #5290 (comment)

The PR releases were only available for core members until I disabled them yesterday completely, because they had a bug where we released some packages unwillingly without an explicit release and without a pr dist tag 😬

Releasing unreviewed code in the chakra npm namespace raises some security concerns.

@TimKolberger
Copy link
Contributor

Thanks, that worked great! ✨

I tinkered a bit with the module augmentation and came up with this question:

The following module augmentation works in my test repository with the current version of chakra ui:

type DefaultSizes = 'small' | 'medium' | 'large';

declare module "@chakra-ui/styled-system" {
  export interface ThemeTypings {
    borders: 'none' | 'thin' | 'thick';
    components: {
      Button: {
        sizes: DefaultSizes;
        variants: 'wacky' | 'chill';
      };
    }
  }
}

Why do we need to introduce a new interface CustomThemeTypings?
If we augment ThemeTypings in our application codebase (or shipped via a "on-top-chakra component lib") those typings will have precedence over the typings generated by the CLI (in node_modules).
If the user wants to use the chakra cli because they are using extendTheme in their codebase, the changes would not be picked up, because still the augmented types have precedence.

In my current understanding the only way to override the augmented types from the "on-top-chakra component lib" for the user is to use augmented types again.

graph TD
    Chakra[chakra default ThemeTypings] -->|via CLI| CompLib[on-top-chakra component lib]
    CompLib -->|via TS Module Augmentation| App
    App -->|ONLY possible via TS Module Augmentation| App

You mentioned that we could add a feature to the CLI to generate the augmented type definitions:

I was even thinking we could combine this approach with the CLI with a new option to ouput a version of the definitions that leverages the above approach as opposed to modifying the definitions directly in ./node_modules.

And I think this is a great idea! And I think this would make this PR obsolete 🤔

Kindly let me know if I am missing something!

@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 19, 2022

Sorry, I guess I should taken notes back when I started trying to only leverage module augmentation to accomplish this. Going from your latest commit, it looks like component/theme props (size, variant) etc. do work, but I cannot get the types to work for token/regular style props:

When we don't import @chakra-ui/styled-system, these props (like margin) don't show up at all:
space--no-import

when we do import it, the default types take precedence:
space--with-import

Maybe we can modify the Token type? Although, I suspect it might require something similar to what I did with ThemeTypings.

@TimKolberger
Copy link
Contributor

...but I cannot get the types to work for token/regular style props [... without the changes in this PR]

True that! Thank you for laying our your thought process, I did learn a lot 💖

IMO this is good to go as is 🚀

This feature implicates a few follow up steps:

  • update the docs as you mentioned
    • kindly add a warning that the CLI generated types might not work if module augmentation is used
  • add a feature to the CLI to generate the augmented TS file

✨ Awesome work @jrolfs!

@TimKolberger TimKolberger merged commit b0da6e6 into chakra-ui:main Feb 20, 2022
@github-actions github-actions bot mentioned this pull request Feb 20, 2022
@jrolfs
Copy link
Contributor Author

jrolfs commented Feb 22, 2022

Hey, @TimKolberger thank you so much for testing this and for the feedback! I meant to at least acknowledge your concerns illustrated with that pretty diagram ;) — I think for this particular use case that further type customization at the application level won't really be a thing, but I do think it's important that we make that limitation clear...

kindly add a warning that the CLI generated types might not work if module augmentation is used

as, yes, once CustomThemeTypings is implemented, the default CLI output will no longer work. I'm hoping that explaining each use case in the documentation will make this clear, but I will make sure to call that out specifically. I hope to have PRs up for this stuff later this week :)

@TimKolberger
Copy link
Contributor

It was a pleasure collaborating with you on that topic and I am looking forward to further ideas and PRs 💖

Yes, the mentioned edge case is rare and we will never know what chakra users will experience in their codebase 😄 if we document it, maybe someone will save a few hours of debugging.

Cheers

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.

None yet

2 participants