Make Tailwind optional#16
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps package to 1.2.8, makes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils.ts (1)
15-16: Add regression tests for bothcnexecution paths.This change introduces two runtime modes (
twMergepresent vs absent), but current tests only cover basic concatenation. Add explicit tests for both modes so this optional-peer behavior stays stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 15 - 16, Tests are missing coverage for both execution paths of the cn utility (with and without the optional twMerge behavior); add two unit tests: one that constructs inputs for cn when twMerge is undefined/absent and asserts it returns the expected concatenated class string (verifying clsx logic), and another that supplies a mock twMerge implementation to cn and asserts that the returned value is the result of twMerge(classes) (verifying the branch where twMerge is used); reference the cn function, the twMerge parameter, and clsx to locate the implementation and ensure both branches (twMerge ? twMerge(classes) : classes) are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 50-52: Replace the nonstandard peerDependenciesOptional entry by
adding "tailwind-merge" to peerDependencies and marking it optional via
peerDependenciesMeta: move the dependency from peerDependenciesOptional into the
peerDependencies object and add a peerDependenciesMeta object with
"tailwind-merge": { "optional": true }; remove the peerDependenciesOptional key
entirely and ensure package.json remains valid JSON.
In `@src/utils.ts`:
- Around line 5-8: Replace the static CommonJS require usage with a dynamic
import to make tailwind-merge an optional runtime dependency: in the block where
you currently call require('tailwind-merge') and assign twMerge, change the
logic to use import('tailwind-merge').catch(() => undefined) and then, if the
module exists, set twMerge = module.twMerge (or module?.default?.twMerge) so
bundlers like Vite/Rollup/Webpack won't try to resolve the dependency at build
time; update any code that relies on twMerge to handle it being undefined.
---
Nitpick comments:
In `@src/utils.ts`:
- Around line 15-16: Tests are missing coverage for both execution paths of the
cn utility (with and without the optional twMerge behavior); add two unit tests:
one that constructs inputs for cn when twMerge is undefined/absent and asserts
it returns the expected concatenated class string (verifying clsx logic), and
another that supplies a mock twMerge implementation to cn and asserts that the
returned value is the result of twMerge(classes) (verifying the branch where
twMerge is used); reference the cn function, the twMerge parameter, and clsx to
locate the implementation and ensure both branches (twMerge ? twMerge(classes) :
classes) are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d2f0a87-3f91-4fd9-a843-7bffe344c952
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
package.jsonplayground/tsconfig.jsonsrc/utils.tstsconfig.jsonvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 68-81: The dependency versions in package.json are incompatible:
`@vitejs/plugin-react` (^6.0.1) requires Vite 8+, and `@vitest/coverage-istanbul`
(^4.1.5) expects Vitest 4.x while your vitest is ^2.1.9; fix by aligning
versions—either downgrade `@vitejs/plugin-react` to a 5.x release that supports
vite@^5.4.21 or upgrade vite to a compatible 8.x/9.x range to match
`@vitejs/plugin-react`@^6.0.1, and likewise either downgrade
`@vitest/coverage-istanbul` to a 2.x-compatible release or upgrade vitest to
^4.1.5 to match `@vitest/coverage-istanbul`@^4.1.5; update the package.json
entries (the "vite", "@vitejs/plugin-react", "vitest", and
"@vitest/coverage-istanbul" fields), then reinstall (npm/yarn/pnpm) and run the
test/build to confirm compatibility.
In `@src/utils.ts`:
- Around line 6-17: Remove the top-level side-effect import and instead add an
exported async initializer (e.g., initTailwindMerge) that performs the dynamic
import('tailwind-merge') and sets an internal twMerge and a boolean flag (e.g.,
isTwMergeInitialized); modify cn(...) to check that flag and either throw a
clear error if initTailwindMerge wasn't awaited (forcing callers to initialize
before first render) or use a documented deterministic fallback merge function,
and ensure you reference the existing symbols twMerge, cn, and clsx when making
the changes so callers can find and await initTailwindMerge before using cn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c247b052-3ee5-4a4e-bb2c-82660f5de902
📒 Files selected for processing (3)
biome.jsonpackage.jsonsrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- biome.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils.ts`:
- Around line 23-27: The cn() function currently throws if isTwMergeInitialized
is false, breaking existing synchronous call paths; change this to preserve a
synchronous, deterministic API by removing the throw in cn() and instead
performing a safe fallback or on-demand synchronous initialization: either
synchronously call a non-async initializer (or a synchronous wrapper around
initTailwindMerge()) or default to a deterministic merge behavior when
isTwMergeInitialized is false. Locate the check using isTwMergeInitialized
inside cn() and replace the throw with logic that ensures cn() can run
synchronously (e.g., call a sync initTailwindMerge() helper or use a safe
default merge) so callers need not await initTailwindMerge().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+23
to
+27
| if (!isTwMergeInitialized) { | ||
| throw new Error( | ||
| 'initTailwindMerge() must be awaited before using cn(). Call await initTailwindMerge() during initialization.', | ||
| ) | ||
| } |
There was a problem hiding this comment.
cn() now throws on existing synchronous call paths (runtime-breaking change).
At Line 23, cn() throws unless initTailwindMerge() was awaited first. Current usage patterns call cn() directly (e.g., src/utils.test.ts:1-9, and component consumers via src/components/Drawer.tsx / src/index.ts exports), so this introduces immediate runtime failures and a breaking API shift.
Proposed fix (keep API synchronous + deterministic)
let twMerge: ((input: string) => string) | undefined
let isTwMergeInitialized = false
+let didUseCn = false
export async function initTailwindMerge(): Promise<void> {
- if (isTwMergeInitialized) {
+ if (isTwMergeInitialized || didUseCn) {
return
}
try {
const module = await import('tailwind-merge')
- twMerge = module?.twMerge || module?.default?.twMerge
+ if (!didUseCn) {
+ twMerge = module?.twMerge || module?.default?.twMerge
+ }
} catch {
// tailwind-merge is not available, use fallback
- twMerge = undefined
+ if (!didUseCn) {
+ twMerge = undefined
+ }
}
isTwMergeInitialized = true
}
export function cn(...inputs: ClassValue[]) {
- if (!isTwMergeInitialized) {
- throw new Error(
- 'initTailwindMerge() must be awaited before using cn(). Call await initTailwindMerge() during initialization.',
- )
- }
+ didUseCn = true
const classes = clsx(inputs)
return twMerge ? twMerge(classes) : classes
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!isTwMergeInitialized) { | |
| throw new Error( | |
| 'initTailwindMerge() must be awaited before using cn(). Call await initTailwindMerge() during initialization.', | |
| ) | |
| } | |
| let twMerge: ((input: string) => string) | undefined | |
| let isTwMergeInitialized = false | |
| let didUseCn = false | |
| export async function initTailwindMerge(): Promise<void> { | |
| if (isTwMergeInitialized || didUseCn) { | |
| return | |
| } | |
| try { | |
| const module = await import('tailwind-merge') | |
| if (!didUseCn) { | |
| twMerge = module?.twMerge || module?.default?.twMerge | |
| } | |
| } catch { | |
| // tailwind-merge is not available, use fallback | |
| if (!didUseCn) { | |
| twMerge = undefined | |
| } | |
| } | |
| isTwMergeInitialized = true | |
| } | |
| export function cn(...inputs: ClassValue[]) { | |
| didUseCn = true | |
| const classes = clsx(inputs) | |
| return twMerge ? twMerge(classes) : classes | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.ts` around lines 23 - 27, The cn() function currently throws if
isTwMergeInitialized is false, breaking existing synchronous call paths; change
this to preserve a synchronous, deterministic API by removing the throw in cn()
and instead performing a safe fallback or on-demand synchronous initialization:
either synchronously call a non-async initializer (or a synchronous wrapper
around initTailwindMerge()) or default to a deterministic merge behavior when
isTwMergeInitialized is false. Locate the check using isTwMergeInitialized
inside cn() and replace the throw with logic that ensures cn() can run
synchronously (e.g., call a sync initTailwindMerge() helper or use a safe
default merge) so callers need not await initTailwindMerge().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
tailwind-mergemoved to an optional peer dependency while retained for developmentBehavior
Tests / Dev tooling