[Fields] Migrate store and actions from editor package to fields package #65261

gigitux · 2024-09-12T07:32:54Z

What?

This PR moves actions and fields to @wordpress/fields.

Based on the different types, I created subfolder to collect all the actions that belong at the same type.

base-post: contains all the actions for the BasePost type.
common contains all the actions for the Post type (that it is an union type).
pattern contains all the actions for the Pattern type.

We should change the name of the files inside these folders, but I'm going to do in a dedicated PR.

@youknowriad suggested not to move the store at least for now in @wordpress/fields package given that stores are singleton and using them in "bundled packages" create a lot of issues.

Old description

According to this feedback, this PR initiates the process of moving the store and certain actions from the @wordpress/editor package to the @wordpress/fields package. To facilitate easier review and iteration, this work will be divided into multiple smaller PRs. This PR is the first in that series.

Question

This PR migrates all actions that do not depend on the @wordpress/editor package. How should we handle the actions that do depend on the @wordpress/editor package?

Some examples:

reset-post.tsx:

gutenberg/packages/editor/src/dataviews/actions/reset-post.tsx

Line 45 in 0e671a2

useDispatch( editorStore as StoreDescriptor )
duplicate-template-part.tsx:

gutenberg/packages/editor/src/dataviews/actions/duplicate-template-part.tsx

Lines 53 to 54 in 0e671a2

<CreateTemplatePartModalContents

blocks={ blocks }

Wouldn't it be better if @wordpress/fields only exposed the store, allowing actions to be registered based on the post type, while the actual registration occurs on the @wordpress/editor side?

I can imagine that for plugins that define new postType, the registration will happen on their JS code.

WDYT? @youknowriad @oandregal

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

gigitux · 2024-09-12T07:39:58Z

packages/editor/src/dataviews/store/private-actions.ts

The deleted code has been migrated to the @wordpress/fields package

gigitux · 2024-09-12T07:41:00Z

packages/fields/src/store/actions.ts

The commented code has not yet been migrated to the editor package. I will complete the migration in a subsequent pull request.

gigitux · 2024-09-12T07:42:11Z

packages/fields/src/types.ts

Copied-pasted from packages/editor/src/dataviews/types.ts

gigitux · 2024-09-12T07:42:45Z

packages/fields/src/wordpress-editor.d.ts

@@ -0,0 +1 @@
+declare module '@wordpress/editor';


This is necessary because @wordpress/editor package isn't a TypeScript package

…n-dataviews

youknowriad · 2024-09-12T10:45:12Z

This PR migrates all actions that do not depend on the @wordpress/editor package. How should we handle the actions that do depend on the @wordpress/editor package?

Looking at these actions, they don't really depend on the "editor" package, they just revertTemplate and resetTemplate actions that are basically just functions that are misplaced, these functions don't need the editor package at all.

Wouldn't it be better if @wordpress/fields only exposed the store, allowing actions to be registered based on the post type, while the actual registration occurs on the @wordpress/editor side?

I'm not sure I understand what you're suggestion: field and actions in the "fields" package, the store in the "fields" package but the registration elsewhere?

youknowriad · 2024-09-12T10:47:23Z

packages/editor/src/components/post-actions/actions.js

@@ -14,17 +15,33 @@ export function usePostActions( { postType, onActionPerformed, context } ) {
 	const { defaultActions } = useSelect(
 		( select ) => {
 			const { getEntityActions } = unlock( select( editorStore ) );
+			const { getEntityActions: getEntityActionsFromFieldsPackage } =
+				select( fieldsStore );


One thing that I just thought about is that "stores" are singletons and using them in "bundled packages" create a lot of issues. For instance if a third-party decides to use the package as well, we'll end up with two stores that don't talk to each other.

This makes me thing that maybe a first step here is to extract the fields and the actions but to leave the store and registration in the "editor" package for now.

gigitux · 2024-09-12T11:11:41Z

Looking at these actions, they don't really depend on the "editor" package, they just revertTemplate and resetTemplate actions that are basically just functions that are misplaced, these functions don't need the editor package at all.

Oh ok! Gotcha! Also, in the case of the duplicateTemplatePart, we are using CreateTemplatePartModalContents that it is defined in the editor package.

Sorry for not being clear. I'm going to rephrase/improving my suggestion having new context after you comment.

I'm suggesting to (on long term):

fields, actions (that don't depend on editor package) and store in the fields package.
keep remaining actions in the @wordpress/editor package, registering them via the registerEntityAction function.

This makes me thing that maybe a first step here is to extract the fields and the actions but to leave the store and registration in the "editor" package for now.

👍 It makes sense! I will update the PR!

youknowriad · 2024-09-12T11:46:49Z

fields, actions (that don't depend on editor package) and store in the fields package.

I don't think there should be fields in Gutenberg that depend on "editor". It's probably just because they were developed there... The CreateTemplate modal can be moved to the "fields" package.

youknowriad · 2024-09-12T13:00:50Z

packages/editor/src/dataviews/store/private-actions.ts

+	duplicatePattern,
+	reorderPage,
+	exportPattern,
+	permanentlyDeletePost,


If we're going to export actions and fields from the same package, maybe we should consider suffixing these with Action?

Yeah, my idea is to have a structure like this Action.

gutenberg/packages/fields/src/actions/base-post/duplicate-post.tsx

Lines 30 to 31 in 3616964

const duplicatePost: Action< BasePost > = {

id: 'duplicate-post',

For instance, the duplicatePost action will be duplicateBasePostAction.

I'm a bit confused about why this exports actions. How you all envision the wordpress/fields package? If it's about fields, it should only export fields (see suggestion). If it's about dumping every WordPress-related thing we have in DataViews, wouldn't it be best to use a name that communicates that better?

I think that the @wordpress/fields will expose all the components/actions necessary for WordPress Core - we discussed this here.

youknowriad · 2024-09-12T13:01:24Z

packages/fields/README.md

-Nothing to document.
+### duplicatePattern
+
+Undocumented declaration.


Would be cool to add some placeholder JS Docs for these to avoid the "Undocumented declaration". We can definitely do that in a separate PR though.

After wrapped up the migration, I will:

add JSDocs

update file name and action name (as I described here)

youknowriad · 2024-09-12T13:02:07Z

packages/fields/src/actions/base-post/index.ts

@@ -0,0 +1,5 @@
+export { default as viewPost } from './view-post';


Do we need the "base-post" folder?

I don't have a strong opinion, but I explained why I structured it in this way in the PR description! Happy to revisit it!

I like to start flat personally in general but I don't mind if you prefer it this way.

github-actions · 2024-09-12T13:29:26Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

gigitux · 2024-09-12T14:09:16Z

I fixed the CI steps regarding mobile version with ae0efeb and f7f1d4d.

This PR is ready to be merged! Thanks for the review! 🙇

Migrate store and actions from editor package to fields package

67ff043

gigitux force-pushed the add/action-dataviews branch from 85d4fc0 to 67ff043 Compare September 12, 2024 07:34

gigitux added 2 commits September 12, 2024 09:37

fix documentation

a71e726

fix import

0141d8c

gigitux commented Sep 12, 2024

View reviewed changes

packages/fields/src/types.ts Outdated

Copy link

Contributor Author

gigitux Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied-pasted from packages/editor/src/dataviews/types.ts

gigitux commented Sep 12, 2024

View reviewed changes

gigitux added 3 commits September 12, 2024 09:54

remove duplicate code

f21a974

Merge branch 'trunk' of github.com:WordPress/gutenberg into add/actio…

7063ee8

…n-dataviews

update package-lock

644dab1

youknowriad reviewed Sep 12, 2024

View reviewed changes

refactor PR

3616964

youknowriad reviewed Sep 12, 2024

View reviewed changes

youknowriad added [Type] Code Quality Issues or PRs that relate to code quality [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Sep 12, 2024

youknowriad approved these changes Sep 12, 2024

View reviewed changes

fix mobile build

f7f1d4d

gigitux marked this pull request as ready for review September 12, 2024 13:29

update package.json

ae0efeb

youknowriad merged commit d329bfb into WordPress:trunk Sep 12, 2024
59 checks passed

github-actions bot added this to the Gutenberg 19.3 milestone Sep 12, 2024

gigitux mentioned this pull request Sep 12, 2024

Migrate store/actions from editor package to fields package #65289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fields] Migrate store and actions from editor package to fields package #65261

[Fields] Migrate store and actions from editor package to fields package #65261

gigitux commented Sep 12, 2024 •

edited

Loading

gigitux Sep 12, 2024

gigitux Sep 12, 2024 •

edited

Loading

gigitux Sep 12, 2024

gigitux Sep 12, 2024

youknowriad commented Sep 12, 2024

youknowriad Sep 12, 2024

gigitux commented Sep 12, 2024 •

edited

Loading

youknowriad commented Sep 12, 2024

youknowriad Sep 12, 2024

gigitux Sep 12, 2024

oandregal Sep 12, 2024

gigitux Sep 16, 2024

youknowriad Sep 12, 2024

gigitux Sep 12, 2024

youknowriad Sep 12, 2024

gigitux Sep 12, 2024

youknowriad Sep 12, 2024

github-actions bot commented Sep 12, 2024

gigitux commented Sep 12, 2024

	const duplicatePost: Action< BasePost > = {
	id: 'duplicate-post',

		@@ -0,0 +1,5 @@
		export { default as viewPost } from './view-post';

	<CreateTemplatePartModalContents
	blocks={ blocks }

[Fields] Migrate store and actions from editor package to fields package #65261

[Fields] Migrate store and actions from editor package to fields package #65261

Conversation

gigitux commented Sep 12, 2024 • edited Loading

What?

Old description

Question

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Choose a reason for hiding this comment

gigitux Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Sep 12, 2024

Choose a reason for hiding this comment

gigitux commented Sep 12, 2024 • edited Loading

youknowriad commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2024

gigitux commented Sep 12, 2024

gigitux commented Sep 12, 2024 •

edited

Loading

gigitux Sep 12, 2024 •

edited

Loading

gigitux commented Sep 12, 2024 •

edited

Loading