Make types of available keys more strict and configurable through generics

[kachkaev]

Edit from @dcastil:

Closes #293

---

A couple of weeks ago I happily configured font sizes inside extendTailwindMerge:

As it turned out, fontSize was not supported by the theme object which was not obvious from the start. Discovering this was a bit painful and took our team some energy and time. After searching for similar issues with fontSize, I found a few folks falling into the same trap: #274, #250, etc.

This PR slightly improves the DX by flagging the trap I have fallen into:

Stricter typings will hopefully make it easier for other teams to configure tailwind-merge faster and with fewer errors. They also enable autocompletion:

For those who are here from search, the right config for fontSize in my case was this:

const mergeTailwindClasses = extendTailwindMerge({
  theme: {
    borderRadius: figmaSpacingUnits,
    borderWidth: figmaSpacingUnits,
    spacing: figmaSpacingUnits,
  },
  classGroups: {
    'font-size': [{ text: ['tiny', 'okay-ish', 'enormous'] }], // names are fake, avoid these if you can 😂
  },
});

PS: Great lib @dcastil 💯

PPS: Apologies for unexpected reviews – I shared this PR with colleagues and they thought it was against our repo 😅

[dcastil]

Hey @kachkaev! 👋

Thanks for the PR (and the reviews haha 😁).

The partial support for theme collections is really a pain in tailwind-merge, sorry for the energy you had to spend on this.

However, narrowing the type of possible keys in the config would be a breaking change, I'm afraid we can't merge this as-is.

It is currently possible to expand the available theme keys yourself and use those in the config which is important in the context of plugins. It's not so well documented at the moment, but you'll find the info here: https://github.com/dcastil/tailwind-merge/blob/v1.14.0/docs/api-reference.md#fromtheme

E.g. you can define your font-size scale like this:

import { extendTailwindMerge, fromTheme } from 'tailwind-merge'

const mergeTailwindClasses = extendTailwindMerge({
  theme: {
    // …rest of definition
    fontSize: ['tiny', 'okay-ish', 'enormous']
  },
  classGroups: {
    'font-size': [{ text: [fromTheme('fontSize')] }],
  },
});

It's a really tricky problem:

• I can't remove the theme keys that exist because otherwise adding a custom value to the spacing scale would require changes in a lot of places. Generally I added theme keys for any scale that is used in more than a single classGroup.
• I can't add all the theme keys to tailwind-merge since that would disproportionally increase the size of the library. I don't remember so well but I think I tripled the size when I tested adding all of them.
• I can't make TypeScript more strict because users need to be able to add their own theme keys

I think the only option available at the moment is to improve the documentation and make it easier to understand how to configure it (less reading, get to results quicker).

[kachkaev]

Hi @dcastil, thanks for sharing the details of this problem, it’s quite deep indeed! I can see the flaws in the proposed solution now.

The docs of tailwind-merge are great already, the problem with them (like with any docs) is that they are out of sight when a mistake is being made. The trap with fontSize was to do with me not having expected feedback in the IDE when I was adding font sizes. When working on the first PR, I configured everything but font sizes and I checked the docs back then. The list of theme fields looked long so I remembered it as ‘everything is supported’. When working on a follow-up PR (font sizes alone), I just relied on this inaccurate knowledge, did not get any feedback in CI, did not notice a clash between text-xs and text-red-500 and chucked the PR in. It turned out that our custom text-xs just matched the standard font size, which fooled me together with the lack of squiggly lines in the editor.

With the potential use of fromTheme, WDYT of this facade?

type CustomThemeKey = 'fontSize' | 'somethingElse';

const mergeTailwindClasses = extendTailwindMerge<ExtraThemeKey>({
  theme: {
    // …rest of definition
    fontSize: ['tiny', 'okay-ish', 'enormous'], // works
    somethingElse: ['foo', 'bar'], // works
    oops: ['hello'], // typescript error
  },
  classGroups: {
    'font-size': [{ text: [fromTheme<CustomThemeKey>('fontSize')] }],
  },
});

It will be a breaking change indeed, but only in the TS land. I have seen other libraries releasing tighter typings in minor versions, so that’s a potential option too. For those who do want to upgrade but have no time to configure new typings, all they’d need to do is:

- const mergeTailwindClasses = extendTailwindMerge({
+ const mergeTailwindClasses = extendTailwindMerge<string>({

Making string a default type would probably defeat the purpose of the change. People will keep falling into a trap with fontSize and other unsupported theme objects. However, if you are hesitant about TS breaking changes in a minor release, another option is:

1. Introduce CustomThemeKey as a generic type arg, default it to string; release semver-minor
2. Change string to an empty set, release semver-major

WDYT?

[dcastil]

With the potential use of fromTheme, WDYT of this facade?

Oh I never considered to use generics. This is a really nice idea! This could also be useful to make the classGroup keys more strict to prevent accidental use of non-existent class groups and to make finding class group keys easier. E.g. the API could look like this:

extendTailwindMerge<ThemeKeys, ClassGroupKeys>(…)

// This would enable the same behavior as currently
extendTailwindMerge<string, string>(…)

It will be a breaking change indeed, but only in the TS land.

Lots of tailwind-merge users are using TypeScript and it would break a lot of build pipelines when a breaking change in types only would be introduced. I think this would be unexpected to most of those people in a minor version change, so I'd only ship this in a major version.

But I'm already looking at a potential v2 release (you can check the list of changes I want to make in the v2 milestone). Introducing the generic defaulting to string right now and making it more strict in v2 seems like a good option to me.

Would you want to do the non-breaking change in this PR? Then you could solve this problem in your team right away and I could solve this for everyone else in v2. This would also give me some time to learn whether there is a way to make the API even better.

[dcastil]

@kachkaev I'm going to use this PR to implement #293 on top of the changes you already made. Let me know if you object. 😁

[kachkaev]

Great, thanks! Feel free to close this PR if needed 👍

[dcastil]

This turned out to be quite the change with a bit of more involved TypeScripting. I hope it'll all work as expected. 😅

[dcastil]

Adding size info since the bot for it didn't run here:

size-limit report 📦

Path	Size
dist/bundle-mjs.mjs	6.70 kB (-0.15% 🔽)
dist/bundle-cjs.js	6.82 KB (-0.15% 🔽)
dist/es5/bundle-mjs.mjs	6.99 KB (-0.57% 🔽)
dist/es5/bundle-cjs.js	7.17 KB (-0.55% 🔽)

[github-actions]

This was addressed in release v2.0.0.