-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[TypeScript] Make types more strict in ra-core, part II #9743
Conversation
ListControllerResult { | ||
children: ReactElement; | ||
} | ||
export type ReferenceManyFieldViewProps = Omit< |
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.
breaking change - this is now a type instead of an interface
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.
I don't see a case where this type could be used, so I'll skip it for the upgrade guide.
showFilter: null, | ||
total: null, | ||
}); | ||
export const ListContext = createContext<ListControllerResult | null>(null); |
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.
Having a default value would force choosing between the pending/error/success case, which doesn't make sense e.g. when using <Datagrid>
outside of a ListContext
but with a data
prop.
On the other hand, using a null
default value should mean that useListContext
returns ListControllerResult | null
, but for some reason the typeScript inference sees the return value as ListControllerResult
, even without return type annotation.
This is convenient, because the alternative is to turn the return type of useListContext
as Partial< ListControllerResult>
, which loses in the process the union between loading/error/success types. Or we could have two hooks:
- one that throws if there is no context, but always returns a
ListControllerResult
- the other that doesn't throw but returns a
Partial< ListControllerResult>
, forcing a lot more runtime checks.
Having two hooks seems too complex, so let's keep the code as is for now.
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.
There aren't many components that actually need the prop fallback: Datagrid, SingleFieldList, and SimpleList. I ended up making a second hook especially for these ones. This allows me to throw an error whenever a useXXXController
hook is invoked outside of a XXXContext
, and ensure that the types are correct when reading the context with these hooks.
@@ -24,7 +19,7 @@ export const BulkActionsToolbar = (props: BulkActionsToolbarProps) => { | |||
className, | |||
...rest | |||
} = props; | |||
const { selectedIds = [], onUnselectItems } = useListContext(props); | |||
const { selectedIds = [], onUnselectItems } = useListContext(); |
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.
Note that a standalone <Datagrid>
with bulkActionToolbars
not set to false
will now fail loudly, wit han exception from useListContext
. But it's better than before, because <Datagrid>
only used to pass the selectedIds
to the BulkActionToolbar
, which didn't let the user select or unselect rows. I modified the doc to make it obvious that if you want bulk actions with a standalone Datagrid, you'll have to provide a context using useList
.
Switching to RFR |
@@ -37,7 +37,7 @@ export const BulkDeleteWithConfirmButton = ( | |||
...rest | |||
} = props; | |||
const { meta: mutationMeta, ...otherMutationOptions } = mutationOptions; | |||
const { selectedIds, onUnselectItems } = useListContext(props); | |||
const { selectedIds, onUnselectItems } = useListContext(); |
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.
Shouldn't it call useListContextWithProps
? It seems we loose the ability to override through props as before. Same for all the other UI components
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.
you're right, but I don't see any use case for overriding the selectedIds on this one.
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.
💪
With strictNullChecks turned on, this PR reduces TS compilation errors on ra-core from 180 to 60 (-67%)
Refs #9622
Follows #9644, #9741
Page Contexts Are Now Types Instead of Interfaces
The return type of page controllers is now a type. If you were using an interface extending one of:
ListControllerResult
,InfiniteListControllerResult
,EditControllerResult
,ShowControllerResult
, orCreateControllerResult
,you'll have to change it to a type:
TypeScript: Stronger Types For Page Contexts
The return type of page context hooks is now smarter. This concerns the following hooks:
useListContext
,useEditContext
,useShowContext
, anduseCreateContext
Depending on the fetch status of the data, the type of the
data
,error
, andisPending
properties will be more precise:{ data: undefined, error: undefined, isPending: true }
{ data: <Data>, error: undefined, isPending: false }
{ data: undefined, error: <Error>, isPending: false }
{ data: <Data>, error: <Error>, isPending: false }
This means that TypeScript may complain if you use the
data
property without checking if it's defined first. You'll have to update your code to handle the different states:const MyCustomList = () => { const { data, error, isPending } = useListContext(); if (isPending) return <Loading />; + if (error) return <Error />; return ( <ul> {data.map(record => ( <li key={record.id}>{record.name}</li> ))} </ul> ); };
Besides, these hooks will now throw an error when called outside of a page context. This means that you can't use them in a custom component that is not a child of a
<List>
,<ListBase>
,<Edit>
,<EditBase>
,<Show>
,<ShowBase>
,<Create>
, or<CreateBase>
component.