chore: Upgrade to React 18 #1636
Conversation
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
…canvas-kit into upgrade-to-react-18
RayRedGoose
requested review from
mannycarrera4,
NicholasBoll and
alanbsmith
and removed request for
mannycarrera4 and
NicholasBoll
RayRedGoose
added
dependencies
| return ( | ||
| <Flex | ||
| as={Element} |
There was a problem hiding this comment.
Kind of a sneaky change to a real polymorphic component.
| export interface ActionBarListProps<T = unknown> | ||
| extends Partial<ExtractProps<typeof HStack, never>> { | ||
| export interface ActionBarListProps<T = any> | ||
| extends Omit<Partial<ExtractProps<typeof HStack, never>>, 'children'> { |
There was a problem hiding this comment.
Why is omitting children necessary now and not before? HStack always had children as part of the interface. Wouldn't that clash before?
| @@ -30,7 +30,7 @@ export interface ActionBarListProps<T = unknown> | |||
| * {(item) => <ActionBar.Item key={item.id} name={item.name}>{item.text}</ActionBar.Item>} | |||
| * </ActionBar.List> | |||
| */ | |||
| children: ((item: T) => React.ReactNode) | React.ReactNode; | |||
| children: ((item: T, index: number) => React.ReactNode) | React.ReactNode; | |||
There was a problem hiding this comment.
I wonder if this shouldn't use ListBoxProps instead.
| @@ -84,15 +84,15 @@ describe('Popper', () => { | |||
|
|
|||
| // force PopperJS to run | |||
| // eslint-disable-next-line compat/compat | |||
| await act(() => new Promise(requestAnimationFrame)); | |||
| await act(() => new Promise<any>(requestAnimationFrame)); | |||
There was a problem hiding this comment.
Why is this any needed? It should extract the type from requestAnimationFrame, but we don't actually care about the return type.
There was a problem hiding this comment.
ReactDOM test utils changed the type of the promise that could be returned to act and forced it to be void even though the promise return type is ignored. requestAnimationFrame takes a callback that expects a number and they 2 come into conflict.
The alternative is:
await act(() => new Promise(cb => requestAnimationFrame(() => cb())));But that seems messier
| @@ -17,7 +17,8 @@ import { | |||
| import {useTabsModel} from './useTabsModel'; | |||
|
|
|||
| // Use `Partial` here to make `spacing` optional | |||
| export interface TabListProps<T = unknown> extends Partial<ExtractProps<typeof Stack, never>> { | |||
| export interface TabListProps<T = any> | |||
| extends Omit<Partial<ExtractProps<typeof Stack, never>>, 'children'> { | |||
There was a problem hiding this comment.
I wonder if ExtractProps should remove children... It is easier to add children than to take it away. I don't like using Omit as the type becomes much messier. All the Extract* utility types avoid using it and instead return clean interfaces
NicholasBoll
added
automerge
and removed
ready for review
|
Note, I'm removing this text from the PR description as a correction.
|
Summary
reactandreact-domto v18@types/reactand@types/react-domto v18Resolved: #1527
Checklist