-
Notifications
You must be signed in to change notification settings - Fork 4.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
DataViews Extensibility: Allow unregistering the reorder-page action #64199
Conversation
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. |
@@ -0,0 +1,25 @@ | |||
/** |
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.
This PR is an opportunity to create a dedicated folder/file for the post type fields. If the fields declaration grows and becomes too big we can separate each field in its own file.
@@ -18,7 +18,9 @@ export function normalizeFields< Item >( | |||
|
|||
const getValue = | |||
field.getValue || | |||
( ( { item }: { item: ItemRecord } ) => item[ field.id ] ); | |||
( ( { item }: { item: ItemRecord } ) => | |||
// @ts-ignore |
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.
This is needed because I had to change the "ItemRecord" type from Record to Object. For me it should be possible to access object properties using a string key like here, so maybe there's another typescript friendly way to do it.
If I don't update the type of ItemRecord
than the orderField
definition becomes invalid requiring me to have an explicit getValue
instead.
cc @jsnajdr in case you have thoughts.
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.
Interesting, for some reason PostWithPageAttributesSupport
type is not assignable to Record<string, unknown>
.
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.
This can be fixed by making PostWithPageAttributesSupport
a type
instead of interface
. Because interfaces cannot be indexed by any string. Try this:
interface IPostWithPageAttributesSupport {
menu_order: number;
}
type TPostWithPageAttributesSupport = {
menu_order: number;
}
type IExt = IPostWithPageAttributesSupport extends Record<string,unknown> ? 'yes' : 'no';
type TExt = TPostWithPageAttributesSupport extends Record<string,unknown> ? 'yes' : 'no';
The IExt
type is going to be no
and TExt
will be yes
.
I experimented with making ItemRecord
a bit smarter, to index on specific property instead of all strings:
type ItemRecord<Name extends string> = { [name in Name]: unknown };
type Field<Item, Name extends string = string> = { id: Name } & ( Item extends ItemRecord<Name> ? ... : ... );
const orderField: Field< PostWithPageAttributesSupport, 'menu_order' > = ...;
but this created an avalanche of other problems I was unable to fix.
FYI @sirreal as this is another interesting TS puzzle 🙂
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.
To be honest I wonder which is better. Keep that ts-ignore and support interfaces or force users of DataViews to use "type"
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.
It's very strange behavior indeed:
interface IPostWithPageAttributesSupport {
menu_order: number;
// [K: string]: unknown;
}
type TPostWithPageAttributesSupport = {
menu_order: number;
}
// Depends on the string index in the interface.
type IExt = IPostWithPageAttributesSupport extends Record<string,unknown> ? 'yes' : 'no';
// "yes"
type TExt = TPostWithPageAttributesSupport extends Record<string,unknown> ? 'yes' : 'no';
declare var i: IPostWithPageAttributesSupport;
declare var t: TPostWithPageAttributesSupport;
// This is an error if the interface does not have the string index.
i['foo'];
// This is an error… but the type extends Record<string,unknown>???
t['foo'];
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.
// This is an error… but the type extends Record<string,unknown>??? t['foo'];
Interesting that this can be fixed with:
(t as Record<string, unknown>)['foo'];
but the i['foo']
construct cannot be fixed because then TS complains that the two types don't sufficiently overlap for as
to work.
Also you can assign t
to a variable:
const rt: Record<string,unknown> = t;
but you cannot do that with i
.
So, the fact that A extends B
apparently means only that A is assignable to B, but not that A can be directly used as B without any cast.
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.
All these type issues are caused by trying to be smart about the Field.getValue
method. We try to figure out automatically whether it must be supplied by the caller, or whether it's optional and can be defaulted to:
getValue = ( { item } ) => item[ field.id ];
This works only for certain types of item
, namely those that have a field called field.id
. Or for those that can be at least safely used with the []
operator. Then the value will be undefined
, but at least it won't crash, like e.g. ( null )[ 'foo' ]
would do.
If we stopped being smart about this, and made getValue
always optional, then we could get rid of this complexity. Even at the cost of allowing a runtime crash when the item
type is not indexable. But I'd say that in practice this is very unlikely.
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'm implementing the suggestion from the previous comment in #64367.
Size Change: +374 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
f9f94b4
to
4082646
Compare
4082646
to
a5b6f30
Compare
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.
Besides the types thing that is being discussed, this looks good. Thank you!
694e2ea
to
94d1d2b
Compare
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
Related #61084
Similar to #62647
What?
In #62052 an API to register and unregister dataviews actions has been implemented. But in order to allow third-party developers to be able to unregister these actions, we need to be using the same actions in Core to register the core actions. The current PR explore the possibility to use the API to register one action: "reorder page".
Testing Instructions
1- Open the pages dataviews.
2- You should be able to see the "reorder" action in the actions dropdown menu
3- you can try to use the action.