Code quality: Replace hardcoded store keys with store definitions

[gziolo]

Description

With #26655 it's possible to use store definitions as param for select/useSelect/withSelect and dispatch/useDispatch/withDispatch API method calls exposed from @wordpress/data. It resolves the issue with the need to use unscoped import statements for packages that define stores used as explained in #8981 (comment):

To be clear, all packages that use a given datastore should import it in the entry point by convention. It's something that could be improved because if it's missed, it might indeed cause errors. At the moment, there is no simple way to validate it happens. Example:

gutenberg/packages/edit-post/src/index.js

Lines 4 to 10 in 87fa5f5

	import '@wordpress/core-data';
	import '@wordpress/block-editor';
	import '@wordpress/editor';
	import '@wordpress/keyboard-shortcuts';
	import '@wordpress/reusable-blocks';
	import '@wordpress/viewport';
	import '@wordpress/notices';

The next step is to use newly exposed store definitions from the corresponding packages similar to how it was applied to the store exposef from @wordpress/viewport in #26655. Example:

diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js
index 56ad63c601e1..f35fcf9d14bc 100644
--- a/packages/interface/src/components/complementary-area/index.js
+++ b/packages/interface/src/components/complementary-area/index.js
@@ -11,6 +11,7 @@ import { useDispatch, useSelect } from '@wordpress/data';
 import { __ } from '@wordpress/i18n';
 import { check, starEmpty, starFilled } from '@wordpress/icons';
 import { useEffect, useRef } from '@wordpress/element';
+import { store as viewportStore } from '@wordpress/viewport';
 
 /**
  * Internal dependencies
@@ -104,10 +105,8 @@ function ComplementaryArea( {
 				isActive: _activeArea === identifier,
 				isPinned: isItemPinned( scope, identifier ),
 				activeArea: _activeArea,
-				isSmall: select( 'core/viewport' ).isViewportMatch(
-					'< medium'
-				),
-				isLarge: select( 'core/viewport' ).isViewportMatch( 'large' ),
+				isSmall: select( viewportStore ).isViewportMatch( '< medium' ),
+				isLarge: select( viewportStore ).isViewportMatch( 'large' ),
 			};
 		},
 		[ identifier, scope ]

The goal is to make it simpler to maintain dependencies between packages to avoid bugs when consuming from npm. The other benefit is that we will prevent cyclic dependencies in the early stages of development.

Tasks

This is the list of store names that should be replaced with their newly introduced store definitions:

• core
• core/annotation (Annotations: Replace store name string with exposed store definition #28156 @rafaelgalani)
• core/block-directory (Block Directory: Replace store name string with exposed store definition #27178 @rafaelgalani)
• core/block-editor
• core/blocks (Blocks: Replace store name string with exposed store definition #27336 @rafaelgalani )
• core/data
• core/edit-navigation (Edit Navigation: Replace store name string with exposed store definition #27182, Edit Navigation: Replace remaining store name string with constant #27281 @rafaelgalani)
• core/edit-post (Edit Post: Replace store name string with exposed store definition #27414 @rafaelgalani)
• core/edit-site (Site Editor: Replace core/edit-site store name with store object #28695 @david-szabo97)
• core/edit-widgets (Edit Widgets: Replace store name string with exposed store definition #28044 @rafaelgalani)
• core/editor
• core/interface (Interface: Replace store name string with exposed store definition #28041 @rafaelgalani)
• core/keyboard-shortcuts (Keyboard Shortcuts: Replace store name string with exposed store definition #27355 @bosconian-dynamics)
• core/notices (Code Quality: Use store definition instead of string for notices packages #27548 @gziolo)
• core/reusable-blocks (Reusable Blocks: Change from store name provided as a string to store definition #27094 @grzim)
• core/rich-text (Rich Text: Replace store name string with exposed store definition #27820 @rafaelgalani)
• core/viewport (@gziolo)

[rafaelgallani]

Hello! Are the unchecked items (except the one in #27094) available for taking? I would like to replace some of these.
Initially, the core/block-directory one if it's not taken.

[gziolo]

Yes, those unchecked and without a PR referenced are up for grabs 👍

core/block-directory seems like a good one to start, it might not have an external usage outside of its package though 😃

[rafaelgallani]

Taking core/edit-navigation 😸

[rafaelgallani]

@gziolo I was going to pick some more items, but I just saw that #27226 is proposing a change to the stores names. Should I wait for the PR to be merged or can I continue refactoring?

[gziolo]

@gziolo I was going to pick some more items, but I just saw that #27226 is proposing a change to the stores names. Should I wait for the PR to be merged or can I continue refactoring?

There is no need to wait. We can always rebase with master branch and update #27226 if we decide to change exported names. It will be a quick operation in IDE to replace store as with an empty string 😄

[rafaelgallani]

Great! I just asked to make sure I wouldn't be complicating things... 😸
I'll work on it then. 👍🏻

[bosconian-dynamics]

I would love to pick up one of these as my inaugural Gutenberg issue, using the other PRs so far as a model for the work. core/keyboard-shortcuts looks to me like it might be relatively straightforward?

[rafaelgallani]

Can I take core/blocks? It looks a little bit more fun than the previous ones I took since other packages depend on it 😄

[gziolo]

@rafaelgalani, whatever works for you best. I added your name next to core/blocks on the list.

[gziolo]

I opened #27368 that removes bare import statements for packages that use now store definitions rather than hardcoded strings.

[rafaelgallani]

Taking core/edit-post 😸😸😸

[david-szabo97]

Should we maybe land the ESLint rule first at the warning level to make the process simpler?

Oh, that sounds like a fantastic idea! We can also warn people working on new PRs to avoid using strings.

[gziolo]

@aristath, what's the current status after your recent work in this area? I see a few more warnings, are there any issues discovered? We should promote the warning to error as the last step.

[aristath]

There are just a handful of them remaining, these are the ones I couldn't address (*a couple of them were reverted because they were causing an issue with an e2e test)

gutenberg/packages/block-library/src/categories/edit.js

Line 30 in 4c3300c

const { isResolving } = select( 'core/data' );

gutenberg/packages/block-library/src/embed/edit.js

Line 67 in 4c3300c

const { invalidateResolution } = useDispatch( 'core/data' );

gutenberg/packages/edit-post/src/components/header/fullscreen-mode-close/index.js

Line 27 in 4c3300c

const { isResolving } = select( 'core/data' );

gutenberg/packages/edit-post/src/store/actions.js

Lines 499 to 500 in 4c3300c

	const isWelcomeGuideActive = yield controls.select(
	'core/edit-post',

gutenberg/packages/editor/src/components/post-publish-panel/prepublish.js

Line 36 in 4c3300c

const { isResolving } = select( 'core/data' );

gutenberg/packages/server-side-render/src/index.js

Line 19 in 4c3300c

const coreEditorSelect = select( 'core/editor' );

Once we address these 6 we should promote this to an error 🎉

[gziolo]

I started #32537 which tries to tackle the remaining issues.

[gziolo]

We did it 🎉

Amazing team effort everyone involved. Major props for the achievement.

There is even an ESLint rule @wordpress/data-no-store-string-literals that will prevent usage of hardcoded store names in the future.

There are still several places where we have hardcoded store names, but they require some architectural changes because the stores used in the code are present conditionally depending on the execution context.

There is a group of items to look at raised for the code used with native mobile apps in #27751.

In the web part of the universe, we still have two places that need more work:

Discovered in #32801:

gutenberg/packages/block-library/src/calendar/edit.js

Lines 51 to 55 in ee69822

	// FIXME: @wordpress/block-library should not depend on @wordpress/editor.
	// Blocks can be loaded into a *non-post* block editor.
	// eslint-disable-next-line @wordpress/data-no-store-string-literals
	const editorSelectors = select( 'core/editor' );
	if ( editorSelectors ) {

Discovered in #32537

gutenberg/packages/server-side-render/src/index.js

Lines 19 to 23 in 58aab25

	// FIXME: @wordpress/server-side-render should not depend on @wordpress/editor.
	// It is used by blocks that can be loaded into a *non-post* block editor.
	// eslint-disable-next-line @wordpress/data-no-store-string-literals
	const coreEditorSelect = select( 'core/editor' );
	if ( coreEditorSelect ) {

The related follow-up issue is #32842.