-
Notifications
You must be signed in to change notification settings - Fork 430
fix(ui): Improve structural CSS warning message format, warn for appearance usage on components #7628
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
Open
brkalow
wants to merge
5
commits into
main
Choose a base branch
from
bryce/warn-customization-without-pinning-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+285
−25
Open
fix(ui): Improve structural CSS warning message format, warn for appearance usage on components #7628
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
50eb8dc
fix(ui): Improve structural CSS warning message format
brkalow bd9d3fa
improve warning message
brkalow 6f24e0a
feat(ui): Warn about structural CSS on component-level appearance
brkalow 3dd0d17
chore: Add empty changeset
brkalow 9f7563c
chore(ui): Bump bundlewatch limits for ui.browser.js and ui.legacy.br…
brkalow File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| --- | ||
| --- |
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
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
113 changes: 113 additions & 0 deletions
113
packages/ui/src/hooks/__tests__/useWarnAboutCustomizationWithoutPinning.test.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { ClerkInstanceContext } from '@clerk/shared/react'; | ||
| import { renderHook } from '@testing-library/react'; | ||
| import React from 'react'; | ||
| import { beforeEach, describe, expect, test, vi } from 'vitest'; | ||
|
|
||
| import { OptionsContext } from '../../contexts/OptionsContext'; | ||
| import { useWarnAboutCustomizationWithoutPinning } from '../useWarnAboutCustomizationWithoutPinning'; | ||
|
|
||
| // Mock the warning function | ||
| vi.mock('../../utils/warnAboutCustomizationWithoutPinning', () => ({ | ||
| warnAboutComponentAppearance: vi.fn(), | ||
| })); | ||
|
|
||
| import { warnAboutComponentAppearance } from '../../utils/warnAboutCustomizationWithoutPinning'; | ||
|
|
||
| const mockWarnAboutComponentAppearance = vi.mocked(warnAboutComponentAppearance); | ||
|
|
||
| // Helper to create a wrapper with contexts | ||
| function createWrapper({ | ||
| clerkInstanceType = 'development', | ||
| hasClerkContext = true, | ||
| uiPinned = false, | ||
| }: { | ||
| clerkInstanceType?: 'development' | 'production'; | ||
| hasClerkContext?: boolean; | ||
| uiPinned?: boolean; | ||
| } = {}) { | ||
| return function Wrapper({ children }: { children: React.ReactNode }) { | ||
| const clerkValue = hasClerkContext | ||
| ? { | ||
| value: { | ||
| instanceType: clerkInstanceType, | ||
| } as any, | ||
| } | ||
| : undefined; | ||
|
|
||
| const optionsValue = uiPinned ? { ui: { version: '1.0.0' } } : {}; | ||
|
|
||
| return ( | ||
| <ClerkInstanceContext.Provider value={clerkValue}> | ||
| <OptionsContext.Provider value={optionsValue as any}>{children}</OptionsContext.Provider> | ||
| </ClerkInstanceContext.Provider> | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| describe('useWarnAboutCustomizationWithoutPinning', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| // Mock requestIdleCallback since it may not be available in test environment | ||
| vi.stubGlobal('requestIdleCallback', (cb: () => void) => { | ||
| cb(); | ||
| return 1; | ||
| }); | ||
| vi.stubGlobal('cancelIdleCallback', vi.fn()); | ||
| }); | ||
|
|
||
| describe('in development mode', () => { | ||
| test('calls warnAboutComponentAppearance when component mounts with appearance', () => { | ||
| const appearance = { | ||
| elements: { card: { '& > div': { color: 'red' } } }, | ||
| }; | ||
|
|
||
| renderHook(() => useWarnAboutCustomizationWithoutPinning(appearance), { | ||
| wrapper: createWrapper({ clerkInstanceType: 'development' }), | ||
| }); | ||
|
|
||
| expect(mockWarnAboutComponentAppearance).toHaveBeenCalledTimes(1); | ||
| expect(mockWarnAboutComponentAppearance).toHaveBeenCalledWith(appearance, false); | ||
| }); | ||
|
|
||
| test('passes uiPinned=true when options.ui is set', () => { | ||
| const appearance = { | ||
| elements: { card: { '& > div': { color: 'red' } } }, | ||
| }; | ||
|
|
||
| renderHook(() => useWarnAboutCustomizationWithoutPinning(appearance), { | ||
| wrapper: createWrapper({ clerkInstanceType: 'development', uiPinned: true }), | ||
| }); | ||
|
|
||
| expect(mockWarnAboutComponentAppearance).toHaveBeenCalledTimes(1); | ||
| expect(mockWarnAboutComponentAppearance).toHaveBeenCalledWith(appearance, true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('in production mode', () => { | ||
| test('does not call warnAboutComponentAppearance', () => { | ||
| const appearance = { | ||
| elements: { card: { '& > div': { color: 'red' } } }, | ||
| }; | ||
|
|
||
| renderHook(() => useWarnAboutCustomizationWithoutPinning(appearance), { | ||
| wrapper: createWrapper({ clerkInstanceType: 'production' }), | ||
| }); | ||
|
|
||
| expect(mockWarnAboutComponentAppearance).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('without ClerkProvider context', () => { | ||
| test('does not call warnAboutComponentAppearance (graceful degradation for tests)', () => { | ||
| const appearance = { | ||
| elements: { card: { '& > div': { color: 'red' } } }, | ||
| }; | ||
|
|
||
| renderHook(() => useWarnAboutCustomizationWithoutPinning(appearance), { | ||
| wrapper: createWrapper({ hasClerkContext: false }), | ||
| }); | ||
|
|
||
| expect(mockWarnAboutComponentAppearance).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
57 changes: 57 additions & 0 deletions
57
packages/ui/src/hooks/useWarnAboutCustomizationWithoutPinning.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { ClerkInstanceContext, OptionsContext as SharedOptionsContext } from '@clerk/shared/react'; | ||
| import { useContext, useEffect } from 'react'; | ||
|
|
||
| import { OptionsContext } from '../contexts/OptionsContext'; | ||
| import type { Appearance } from '../internal/appearance'; | ||
| import { warnAboutComponentAppearance } from '../utils/warnAboutCustomizationWithoutPinning'; | ||
|
|
||
| /** | ||
| * Hook that checks component-level appearance for structural CSS patterns | ||
| * and warns if found (when version is not pinned). | ||
| * | ||
| * This is called when individual components mount with their own appearance, | ||
| * to catch structural CSS that wasn't passed through ClerkProvider. | ||
| * | ||
| * Only runs in development mode. | ||
| * | ||
| * Note: This hook is safe to use outside of ClerkProvider context (e.g., in tests) | ||
| * - it will simply not perform any checks in that case. | ||
| */ | ||
| export function useWarnAboutCustomizationWithoutPinning(appearance: Appearance | undefined): void { | ||
| // Access contexts directly to handle cases where they might not be available (e.g., in tests) | ||
| const clerkCtx = useContext(ClerkInstanceContext); | ||
| // Try our local OptionsContext first, then fall back to shared (if any) | ||
| const localOptions = useContext(OptionsContext); | ||
| const sharedOptions = useContext(SharedOptionsContext); | ||
| const options = localOptions ?? sharedOptions; | ||
|
|
||
| // Cast to any to access `ui` property which exists on IsomorphicClerkOptions but not ClerkOptions | ||
| // This matches the pattern used in warnAboutCustomizationWithoutPinning | ||
| const uiPinned = !!(options as any)?.ui; | ||
|
|
||
| useEffect(() => { | ||
| // Skip if clerk context is not available (e.g., in tests) | ||
| if (!clerkCtx?.value) { | ||
| return; | ||
| } | ||
|
|
||
| // Only check in development mode | ||
| if (clerkCtx.value.instanceType !== 'development') { | ||
| return; | ||
| } | ||
|
|
||
| // Defer warning check to avoid blocking component mount | ||
| const scheduleWarningCheck = | ||
| typeof requestIdleCallback === 'function' ? requestIdleCallback : (cb: () => void) => setTimeout(cb, 0); | ||
|
|
||
| const handle = scheduleWarningCheck(() => { | ||
| warnAboutComponentAppearance(appearance, uiPinned); | ||
| }); | ||
|
|
||
| return () => { | ||
| if (typeof cancelIdleCallback === 'function' && typeof handle === 'number') { | ||
| cancelIdleCallback(handle); | ||
| } | ||
| }; | ||
| }, [clerkCtx?.value, appearance, uiPinned]); | ||
| } | ||
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
requestIdleCallbackisn't available, we fall back tosetTimeoutbut the cleanup still callscancelIdleCallback. Probably fine since the 0ms timeout fires immediately anyway, just wanted to flag it