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

Add support for dynamic per-class prefix #412

Open
dcastil opened this issue May 2, 2024 · 4 comments
Open

Add support for dynamic per-class prefix #412

dcastil opened this issue May 2, 2024 · 4 comments
Labels
context-v2 Related to tailwind-merge v2 feature request

Comments

@dcastil
Copy link
Owner

dcastil commented May 2, 2024

Comes from #405.

The idea is to resolve conflicts across multiple prefixes which aren't necessarily enumerable.

@dcastil dcastil added feature request context-v2 Related to tailwind-merge v2 labels May 2, 2024
@dcastil
Copy link
Owner Author

dcastil commented May 2, 2024

I see two paths to expose an API for this, not sure yet which one makes more sense. But I think I'm learning towards approach 2.

1. prefix config value as a function

const twMerge = createTailwindMerge({
    prefix: (className: string) => {
        // return className without prefix
    }
})

Pros:

  • Somewhat concise and easy to understand
  • Obvious place in the config

Cons:

  • Feels weird: Removing the prefix from a class should be something that happens internally within the library, looks a bit like a leaky abstraction
  • Easy to overlook that non-Tailwind classes might land in this function which could lead to mistakes. E.g. accidentally also supporting non-prefixed classes.

2. Expose low-level API that enables custom beahvior around prefixes

I could do #385 in a way that allows supporting dynamic prefixes.

Pros:

  • Could enable a lot of custom behavior around prefixes that I didn't think of
  • Exact prefix API can be chosen by user
  • Maybe it's better if user has to look into tailwind-merge internals more when handling prefixes to understand performance characteristics and prevent footguns

Cons:

  • More complicated for users

@unional
Copy link

unional commented May 8, 2024

I originally think the prefix function would look like this:

const twMerge = createTailwindMerge({
    prefix: (className: string): string | undefined => {
        // return the prefix, e.g. `tw-`, if the className contains prefix.
        // otherwise, return undefined
    }
})

Would that be better? This way the function is not removing the prefix. That is still handled by the library.

@dcastil

@dcastil
Copy link
Owner Author

dcastil commented May 18, 2024

Ah yes, that makes sense. Thanks!

I'm still worrying about how error-prone this pattern could be. It's not obvious what will happen if the returned prefix isn't in the actual className string.

Still needs a bit more thought. 🤔

@unional
Copy link

unional commented May 18, 2024

I'm still worrying about how error-prone this pattern could be. It's not obvious what will happen if the returned prefix isn't in the actual className string.

There is only three possible outcomes:

  1. drop silently
  2. emit warning in console.warn()
  3. throws error

Personally, my preference would be 1 > 3 > 2.

3 is the cleanest. I would do that in most case actually. Failing early is almost always the best option.

1 is pragmatic. It is least intrusive and follows JavaScript behavior.

// returns the string untouched when not matched
className.replace(/^prefix-/, '')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context-v2 Related to tailwind-merge v2 feature request
Projects
None yet
Development

No branches or pull requests

2 participants