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

Closed
dcastil opened this issue May 2, 2024 · 18 comments
Closed

Add support for dynamic per-class prefix #412

dcastil opened this issue May 2, 2024 · 18 comments
Labels
context-v2 Related to tailwind-merge v2 feature Is new feature

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-/, '')

@unional
Copy link

unional commented Jun 8, 2024

Hi there, want to circle back to see have you decided on which path to take.

@dcastil
Copy link
Owner Author

dcastil commented Jun 13, 2024

Hey @unional, thanks for your patience. So far I couldn't figure out an API with the dynamic prefix that really fits. My problem is that the current idea is too imperative while the rest of the config is declarative.

With tailwind-merge having so many users I can only release APIs that are rock-solid, because changing them later is very painful. So I'm especially cautious here.

I think I'll go with exposing a low-level API from #385 so everyone can build their use-cases themselves. But that will require some time which I currently don't have unfortunately.

If you need the dynamic prefix right now, I recommend that you either fork the repo or copy and paste the tailwind-merge code into your codebase and modify it so that you can use a dynamic prefix. Maybe you'll figure out a great API along the way which we could introduce back to tailwind-merge as well.

@unional
Copy link

unional commented Jun 14, 2024

Sure, completely understand. By the way, for declarative approach, one way is accepting a regex. But in general that is not as good as just accepting a function.

@dcastil
Copy link
Owner Author

dcastil commented Jun 14, 2024

Yeah I also thought about it, but that still limits you to a more or less hardcoded value. It's possible to create RegExes dynamically, but that's also quite painful to do.

@unional
Copy link

unional commented Jun 21, 2024

I take a look at the code and I can see your hesitation.

It seems like the code is designed to populate the class/class group ahead of time. So adding this means the data structure types needs to change and also the resolution logic.

What's your "low-level context API" is about? I don't want to make changes that makes it impossible to merge back in the future.

@dcastil
Copy link
Owner Author

dcastil commented Jun 23, 2024

It seems like the code is designed to populate the class/class group ahead of time. So adding this means the data structure types needs to change and also the resolution logic.

Yeah, it would require some more work for sure.

What's your "low-level context API" is about? I don't want to make changes that makes it impossible to merge back in the future.

The problem it's trying to solve is described with some detail in #385. My idea is to create a function similar to createTailwindMerge or extendTailwindMerge that also allows you to hook into or even replace parts of the mergeClassList function (code).

That way you could implement more custom behaviors or add missing features yourself, like a dynamic prefix.

E.g. speaking of the dynamic prefixes, that could be done by modifying originalClassName in https://github.com/dcastil/tailwind-merge/blob/v2.3.0/src/lib/merge-classlist.ts#L22 to remove the prefix, storing the prefix together with the other stuff in https://github.com/dcastil/tailwind-merge/blob/v2.3.0/src/lib/merge-classlist.ts#L64-L70 and then later attaching the prefix to the className again in https://github.com/dcastil/tailwind-merge/blob/v2.3.0/src/lib/merge-classlist.ts#L96.

Also: The reason why I'm hesitant to add these hooks into createTailwindMerge itself and want to build a separate function for it is that those hooks will inevitably slow down the runtime. That is ok if you want to use these hooks, but would be a bummer for most tailwind-merge users who have to bear the runtime cost of a feature they don't need. twMerge might get called hundreds of times during the initial render of an app and the inner loop with the hooks being called can run thousands of times (once per every class in a string). So I'm very cautious with that part of the library.

@dcastil
Copy link
Owner Author

dcastil commented Jul 7, 2024

@unional I just released a new feature in v2.4.0 that allows you to use dynamic prefixes.

import { extendTailwindMerge } from 'tailwind-merge'

const twMerge = extendTailwindMerge({
    experimentalParseClassName({ className, parseClassName }) {
        const parsed = parseClassName(className)
        
        return {
            ...parsed,
            baseClassName: removeDynamicPrefix(parsed.baseClassName)
        }
    }
})

function removeDynamicPrefix(className: string) {
    // remove dynamic prefix here
}

More on that in #444.

@dcastil
Copy link
Owner Author

dcastil commented Jul 31, 2024

@unional I'm considering this issue resolved due to the experimentalParseClassName API. Let me know if I should reopen.

@dcastil dcastil closed this as completed Jul 31, 2024
@unional
Copy link

unional commented Aug 5, 2024

Thanks, I'm going to use it this week. Will let you know if I discover anything.

@unional
Copy link

unional commented Aug 5, 2024

Hi, I just tried it out.

Instead of "remove dynamic prefix", I actually need to convert it to return the baseClassName with the same prefix in order for it to work correctly:

const knownPrefixes = ['tw-', 'app1-']
const regex = new RegExp(`^(${knownPrefixes.join('|')})(.*)`)
createTailwindMerge(() => {
  const defaultConfig = getDefaultConfig()
  return {
    ...defaultConfig,
    prefix,
    experimentalParseClassName: ({ className, parseClassName }) => {
      const parsed = parseClassName(className)
      const match = parsed.baseClassName.match(regex)

      return {
        ...parsed,
        baseClassName: match ? `${prefix}${match[2]}` : parsed.baseClassName
      }
    }
  }
})

@glittersnack
Copy link

Thanks @unional! This is very helpful.

I ran into an issue with negative values and updated this script with the following

const normalizedPrefix = "ui-";
const knownPrefixes = [normalizedPrefix, "web-"];
const regex = new RegExp(`^-?(${knownPrefixes.join("|")})(.*)`);

const twMerge = extendTailwindMerge({
  prefix: normalizedPrefix,
  experimentalParseClassName: ({ className, parseClassName }) => {
    const parsed = parseClassName(className);
    const match = parsed.baseClassName.match(regex);
    const isNegative = match ? parsed.baseClassName.startsWith("-") : false;
    const prefix = `${isNegative ? "-" : ""}${normalizedPrefix}`;

    return {
      ...parsed,
      // We need to normalize all known prefix'd classes for the parser to work correctly.
      baseClassName: match ? `${prefix}${match[2]}` : parsed.baseClassName,
    };
  },
));

@unional
Copy link

unional commented Aug 29, 2024

Awesome. I give it a try, it seems like only adjusting the regex with -? is sufficient:

	it('merges negative class with positive class', () => {
		const p2 = '-twds-p-2'
		expect(twMerge('twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 -twds-p-2')
	})

	it('merges positive class with negative class', () => {
		const p2 = 'twds-p-2'
		expect(twMerge('-twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 twds-p-2')
	})

	it('merges negative class with negative class', () => {
		const p2 = '-twds-p-2'
		expect(twMerge('-twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 -twds-p-2')
	})

Did I miss some case?

@unional
Copy link

unional commented Aug 29, 2024

Also note that technically tailwind supports 2 negative syntax: -tw-z-5 and tw--z-5.

For me, I will only support the first syntax. If you want to support the second one, you can further adjust the regex.

@glittersnack
Copy link

Awesome. I give it a try, it seems like only adjusting the regex with -? is sufficient:

	it('merges negative class with positive class', () => {
		const p2 = '-twds-p-2'
		expect(twMerge('twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 -twds-p-2')
	})

	it('merges positive class with negative class', () => {
		const p2 = 'twds-p-2'
		expect(twMerge('-twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 twds-p-2')
	})

	it('merges negative class with negative class', () => {
		const p2 = '-twds-p-2'
		expect(twMerge('-twds-p-1 twds-bg-green-100', p2)).toEqual('twds-bg-green-100 -twds-p-2')
	})

Did I miss some case?

I don't think so! I was trying to solve a problem I ran into in my own project and tried to reconstruct the return value I was given.

@dcastil dcastil added feature Is new feature and removed feature request labels Oct 20, 2024
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 Is new feature
Projects
None yet
Development

No branches or pull requests

3 participants