chore: migrate from @radix-ui/* to unified radix-ui package#2033
chore: migrate from @radix-ui/* to unified radix-ui package#2033unrevised6419 wants to merge 1 commit into
Conversation
|
@unrevised6419 is attempting to deploy a commit to the Strapi Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 17048da The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
404ba6b to
534604a
Compare
Replace individual @radix-ui/* dependencies with the unified `radix-ui` package (1.6.0) in the design-system and primitives workspaces. - Components import from `radix-ui`, internal utilities from `radix-ui/internal` - Inline `clamp` (was @radix-ui/number); keep @radix-ui/react-use-previous - Adapt to bundled newer primitive versions: React.ComponentPropsWithoutRef, useControllableState v1.2 (required defaultProp), DropdownMenuItemProps rename - Pin @types/react to 18.3.3 via root resolutions (radix install nests React 19 types) - Cast around @radix-ui/react-primitive v2's global CSSProperties augmentation in handleResponsiveValues Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
534604a to
17048da
Compare
|
Adzouz
left a comment
There was a problem hiding this comment.
Hi @unrevised6419, thanks a lot for the PR, your efficiency and many contributions for Strapi, it really means a lot for us 🙌
About this one, it's a bigger change than just merging all packages into one since we're also bumping the version (moving from ~2 years old packages to the latest ones) we have to run non-regression tests to be sure that the impacted components are still working as before.
In the meantime, here's a few comments that would need to be addressed while we're testing on our side 🙏
| locale?: string; | ||
| onOpenChange?(open: boolean): void; | ||
| // TODO: This might need to be `value: string | undefined` | ||
| onValueChange?(value: string): void; |
There was a problem hiding this comment.
This TODO can be resolved in this PR — the answer is yes, the type should be onValueChange?(value: string | undefined): void.
Evidence that undefined is genuinely delivered at runtime (pre-existing behavior, also on main):
- The internal context already types it that way (
onValueChange(value: string | undefined), line 80). - The Escape-clear paths call
context.onValueChange(undefined)(lines 547 and 679), which flows throughsetValue→useControllableState.onChange→ the consumer'sonValueChange.
So the public string type has been lying since before this PR; the new v1.2 generics just forced the cast that exposes it. Widening the prop type here:
- lets you delete the
as ((value: string | undefined) => void) | undefinedcast on line 218, - automatically propagates to the design-system
Combobox(itsonChangeis typed asComboboxPrimitive.RootProps['onValueChange']), - won't break consumers' typechecks — method-style signatures are bivariant, so existing
(value: string) => voidhandlers still assign.
Suggest doing it now rather than merging a known-incorrect type with a TODO.
There was a problem hiding this comment.
This could be a type "breaking change". I will need first to do some checks in Strapi monorepo to see how is used there.
| defaultFilterValue?: string; | ||
| filterValue?: string; | ||
| // TODO: This might need to be `value: string | undefined` | ||
| onFilterValueChange?(value: string): void; |
There was a problem hiding this comment.
This TODO can also be resolved now. Unlike onValueChange, none of the 8 internal context.onFilterValueChange(...) call sites ever pass undefined — they all pass strings. So strictly, string is accurate today.
That said, the state is declared useControllableState<string | undefined> and the context types it (value: string | undefined) => void (line 95), so I'd still widen the public type to string | undefined for consistency — it lets you drop the cast on line 230 and matches the actual state domain (filterValue can legitimately be undefined, see the context.filterValue === undefined checks). Either way, the TODO should not ship.
| const [textValue, setTextValue] = useControllableState({ | ||
| prop: textValueProp, | ||
| defaultProp: allowCustomValue && !defaultTextValue ? valueProp : defaultTextValue, | ||
| defaultProp: (allowCustomValue && !defaultTextValue ? valueProp : defaultTextValue) as string, |
There was a problem hiding this comment.
This as string cast is unsound: valueProp and defaultTextValue are both optional, so the whole expression can be undefined — and undefined is a meaningful state here (placeholder rendering checks context.textValue === undefined, e.g. the data-placeholder logic).
Suggest typing it explicitly like the sibling states instead of casting the value:
const [textValue, setTextValue] = useControllableState<string | undefined>({
prop: textValueProp,
defaultProp: allowCustomValue && !defaultTextValue ? valueProp : defaultTextValue,
onChange: onTextValueChange as ((value: string | undefined) => void) | undefined,
caller: COMBOBOX_NAME,
});(And if onTextValueChange gets the same string | undefined widening as the other two callbacks, that cast disappears too.)
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "@strapi/design-system": patch | |||
There was a problem hiding this comment.
The import rewrite is mechanical, but radix-ui@1.6.0 bundles the current primitive versions while the individual packages were pinned to ~2023 versions. It's ~2 years of Radix behavior changes landing at once — focus handling, pointer-events on dismissable layers, positioning — i.e. a real runtime upgrade for every overlay/menu/dialog in consumers, not just a dependency rename.
The description should also say this explicitly: Radix primitives are upgraded from the 1.0.x era to the versions bundled in radix-ui@1.6.0, and the radix install nests React 19 types — consumers on React 18 may want an @types/react resolution like the one added to this repo's root package.json.
| useComposedRefs, | ||
| useControllableState, | ||
| useLayoutEffect, | ||
| } from 'radix-ui/internal'; |
There was a problem hiding this comment.
Worth a note (here and in Combobox.tsx): radix-ui/internal is Radix's explicitly-unstable surface — no semver guarantees, unlike the individually-versioned public packages these imports replace. Since this package is a fork of Radix Select anyway the dependency on internals is somewhat unavoidable, and the exact 1.6.0 pin protects us. But I'd add a short comment at the import site saying bumps of radix-ui must re-verify the radix-ui/internal imports, so a future "routine" dependency bump doesn't silently hit a moved/removed internal API.
| import { FocusGuards, FocusScope as FocusScopePrimitive } from 'radix-ui/internal'; | ||
|
|
||
| const FocusScope = FocusScopePrimitive.FocusScope; | ||
| const useFocusGuards = FocusGuards.useFocusGuards; |
There was a problem hiding this comment.
Nit: these const re-exports are declared in the middle of the import block (imports continue below on line 23). Valid JS since imports hoist, and oxlint doesn't flag it, but it reads oddly. Suggest either moving the aliases below the last import, or destructuring in place where possible. Same pattern in Combobox.tsx, Select.tsx, Collection.tsx, and DesignSystemProvider.tsx.



What
Replace individual
@radix-ui/*dependencies with the unifiedradix-uipackage (1.6.0) in thedesign-systemandprimitivesworkspaces.Changes
radix-ui, internal utilities fromradix-ui/internalclamp(was@radix-ui/number) andusePrevious(was@radix-ui/react-use-previous) — neither is shipped by the unifiedradix-uipackageReact.ComponentPropsWithoutRef,useControllableStatev1.2 (requireddefaultProp),DropdownMenuItemPropsrename@types/reactto 18.3.3 via root resolutions (radix install nests React 19 types)@radix-ui/react-primitivev2's globalCSSPropertiesaugmentation inhandleResponsiveValuesWhy
Easier upgrade path. With individually-versioned
@radix-ui/*packages, nothing guarantees that the versions you install work together — in theory they can drift out of sync. The unifiedradix-uipackage updates every primitive at once as a single, mutually-compatible set, so upgrades are one bump instead of a coordination exercise across dozens of packages. Single dependency, single React tree, fewer version-skew bugs.Precursor to a possible Base UI migration. In the current shadcn/ui ecosystem, Radix is no longer the default primitive library, and there's a real risk it gets left behind over time — shadcn himself has signalled the shift toward Base UI. Consolidating onto the unified package now is a natural stepping stone toward Base UI if we choose to move later — Base UI ships codemods for Radix and is close to 1:1 API parity, so the eventual switch would be low-friction.
🤖 Generated with Claude Code