DataViews: remove the AnyItem type #62856
Conversation
jsnajdr
added
[Type] Code Quality
| interface DataViewsProps< Item extends AnyItem > { | ||
| type ItemWithId = { id: string }; | ||
|
|
||
| type DataViewsProps< Item > = { |
There was a problem hiding this comment.
I'm not really familiar with any of this. Where is Item defined? Below in Item extends ItemWithId right? I thought an Item was an item in the data array? Those don't have a getItemId function?
There was a problem hiding this comment.
I guess I'm not getting this part
( Item extends ItemWithId
? { getItemId?: ( item: Item ) => string }
: { getItemId: ( item: Item ) => string } )There was a problem hiding this comment.
Item is a type parameter, it's completely generic. DataViews can work with any data type whatsoever, you can have a dataview of numbers. As long as you provide functions to extract IDs and field values from the item.
The Item extends ItemWithId part says that if the Item is an object that has an id field, the getItemId prop is optional, because we have a suitable default for it.
But if Item is not compatible with ItemWithId, then getItemId is mandatory. Because the default (item => item.id) is not going to work with such a type.
| isLoading?: boolean; | ||
| paginationInfo: { | ||
| totalItems: number; | ||
| totalPages: number; | ||
| }; | ||
| supportedLayouts: string[]; | ||
| onSelectionChange?: ( items: Item[] ) => void; | ||
| } | ||
| } & ( Item extends ItemWithId |
There was a problem hiding this comment.
Yes, with this the type system is Turing complete and you can build any program with it 😄
packages/dataviews/src/view-list.tsx
Outdated
| @@ -318,7 +316,7 @@ export default function ViewList< Item extends AnyItem >( | |||
| } = props; | |||
| const baseId = useInstanceId( ViewList, 'view-list' ); | |||
| const selectedItem = data?.findLast( ( item ) => | |||
| selection.includes( item.id ) | |||
| selection.includes( getItemId( item ) ) | |||
There was a problem hiding this comment.
I guess that's a bug fix.
There was a problem hiding this comment.
Yes. Another little suspicious thing is the optional chaining in data?.findLast. The types say that data is mandatory. If that's really true, the optional chaining is not needed. But that's a very minor issue compared with other problems.
|
@jsnajdr #62647 This PR is blocked because of this #62505 (comment) We need to find a good API/way to access/check permissions in dataviews while allowing global registration of actions... (Any suggestions here are welcome) Maybe, we can try to land the current PR without 62647 |
|
OK I can try to rebase it onto |
jsnajdr
force-pushed
the
try/remove-anyitem
branch
from
4897186 to
1b9df79
Compare
jsnajdr
changed the base branch from
update/allow-removing-permanently-delete-action
to
trunk
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
OK now this PR is independent from #62647 and can be merged directly into trunk. |
| header: string; | ||
| getValue: ( args: { item: Item } ) => any; | ||
| render: ( args: { item: Item } ) => ReactNode; | ||
| }; |
There was a problem hiding this comment.
I had to do this because the Required<Pick> construct doesn't work with the conditional type where getValue is defined twice.
There was a problem hiding this comment.
I think this is actually nicer and easier to understand
jsnajdr
force-pushed
the
try/remove-anyitem
branch
from
1b9df79 to
89ee50d
Compare
There was a problem hiding this comment.
Nice simplification here, thank you. I did leave one question I'd like to understand before this is merged.
Removes the
AnyItemtype from dataviews, as it turns out thatItemcan be completely generic without any constraints.Fixes several type errors that the removal revealed. Correct types for the
getItemIdandgetValuegetters that can be optional only for someItemsubtypes because the default getter is not 100% generic. Also uses thegetItemIdfunction instead of non-genericitem.idinViewList.Inspired by this discussion: #62647 (comment)
My plan is that we can finish and merge this PR after #62647 is landed.