Skip to content
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.

Sign up for GitHub

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

Remove select() hack from getBlockEditingMode() #51359

Closed
wants to merge 3 commits into from
Closed

Remove select() hack from getBlockEditingMode() #51359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a82a20b
Remove select() hack from getBlockEditingMode()
noisysocks Jun 9, 2023
41c2532
Update getBlockEditingMode tests
noisysocks Jun 9, 2023
e9855d2
Fix block-editor selector tests
noisysocks Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions packages/block-editor/src/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { createReduxStore, registerStore } from '@wordpress/data';
import { createReduxStore, register } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -35,11 +35,7 @@ export const store = createReduxStore( STORE_NAME, {
persist: [ 'preferences' ],
} );

// We will be able to use the `register` function once we switch
// the "preferences" persistence to use the new preferences package.
const registeredStore = registerStore( STORE_NAME, {
...storeConfig,
persist: [ 'preferences' ],
} );
unlock( registeredStore ).registerPrivateActions( privateActions );
unlock( registeredStore ).registerPrivateSelectors( privateSelectors );
unlock( store ).registerPrivateActions( privateActions );
unlock( store ).registerPrivateSelectors( privateSelectors );

register( store );
Copy link
Member

Choose a reason for hiding this comment

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

You need to keep the legacy registerStore here. Otherwise the persist plugin won't be active. It works by overriding the registerStore method of the registry and reading the persist option. If you call register, the plugin won't catch it.

This will be solved once we finish migration of the block-editor store to the preferences package. It's being worked on in #39632.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, bummer, I see.

I don't think we can remove this workaround yet then because if we do this:

export const store = createReduxStore( STORE_NAME, {
	...storeConfig,
	persist: [ 'preferences' ],
} );

unlock( store ).registerPrivateActions( privateActions );
unlock( store ).registerPrivateSelectors( privateSelectors );

registerStore( STORE_NAME, {
	...storeConfig,
	persist: [ 'preferences' ],
} );

Then the private selectors are not callable from public selectors as they are not bound to the store that's created by registerStore.

(When I try it I get a Uncaught TypeError: selector.registry is undefined error on load.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we will wait until #39632 is merged!

Copy link
Contributor

Choose a reason for hiding this comment

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

I did start looking at #39632 one again. I've rebased it and hope to push some further options this week.

Copy link
Member

Choose a reason for hiding this comment

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

Then the private selectors are not callable from public selectors as they are not bound to the store that's created by registerStore.

Oh, there are two gotchas at work here:

  1. Registering private selectors on the store object is a noop, because the store is not instantiated from that object! It's just a store descriptor object that's exported so that users can pass it to functions like select( store ) or dispatch( store ). The descriptor has an .instantiate method, but it's never called. The registerStore function creates a second store descriptor from STORE_NAME and storeConfig, and uses that internal one to actually instantiate the store. But that descriptor doesn't have any private selectors registered.
  2. To get the private selectors pre-bound to the registry, they need to be registered after createReduxStore and before the .instantiate function is called, but registerStore calls them both within one statement:

    gutenberg/packages/data/src/registry.js

    Line 310 in 4d0f303

    createReduxStore( storeName, options ).instantiate( registry )

65 changes: 30 additions & 35 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import createSelector from 'rememo';
/**
* WordPress dependencies
*/
import { select } from '@wordpress/data';
import { store as blocksStore } from '@wordpress/blocks';
import { createRegistrySelector } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -72,40 +72,35 @@ export function getLastInsertedBlocksClientIds( state ) {
* @return {BlockEditingMode} The block editing mode. One of `'disabled'`,
* `'contentOnly'`, or `'default'`.
*/
export const getBlockEditingMode = createSelector(
( state, clientId = '' ) => {
if ( state.blockEditingModes.has( clientId ) ) {
return state.blockEditingModes.get( clientId );
}
if ( ! clientId ) {
return 'default';
}
const rootClientId = getBlockRootClientId( state, clientId );
const templateLock = getTemplateLock( state, rootClientId );
if ( templateLock === 'contentOnly' ) {
const name = getBlockName( state, clientId );
// TODO: Terrible hack! We're calling the global select() function
// here instead of using createRegistrySelector(). The problem with
// using createRegistrySelector() is that then the public
// block-editor selectors (e.g. canInsertBlockTypeUnmemoized) can't
// call this private block-editor selector due to a bug in
// @wordpress/data. See
// https://github.com/WordPress/gutenberg/pull/50985.
const isContent =
select( blocksStore ).__experimentalHasContentRoleAttribute(
name
);
return isContent ? 'contentOnly' : 'disabled';
}
const parentMode = getBlockEditingMode( state, rootClientId );
return parentMode === 'contentOnly' ? 'default' : parentMode;
},
( state ) => [
state.blockEditingModes,
state.blocks.parents,
state.settings.templateLock,
state.blockListSettings,
]
export const getBlockEditingMode = createRegistrySelector( ( select ) =>
createSelector(
Copy link
Member

Choose a reason for hiding this comment

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

This memoization won't work because createSelector is called again and again on each call of getBlockEditingMode, creating a new instance with its own memoization cache each time.

createRegistrySelector is like:

createRegistrySelector = ( selectorFactory ) => ( ...args ) => selectorFactory( registry.select )( ...args );

I don't know how to solve this, maybe we'll need to patch createRegistrySelector to call the "factory" function only once.

( state, clientId = '' ) => {
if ( state.blockEditingModes.has( clientId ) ) {
return state.blockEditingModes.get( clientId );
}
if ( ! clientId ) {
return 'default';
}
const rootClientId = getBlockRootClientId( state, clientId );
const templateLock = getTemplateLock( state, rootClientId );
if ( templateLock === 'contentOnly' ) {
const name = getBlockName( state, clientId );
const isContent =
select( blocksStore ).__experimentalHasContentRoleAttribute(
name
);
return isContent ? 'contentOnly' : 'disabled';
}
const parentMode = getBlockEditingMode( state, rootClientId );
return parentMode === 'contentOnly' ? 'default' : parentMode;
},
( state ) => [
state.blockEditingModes,
state.blocks.parents,
state.settings.templateLock,
state.blockListSettings,
]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if memoizing this selector is even a good idea. This particular implementation won't catch changes in select( blockStore ).hasContentRoleAttribute( name ). It's very unlikely that a block's registration properties would change, but still.

The selector doesn't do any very complex calculation, and the return value is a string, which doesn't need an unique identity. Let's just remove the createSelector memoization.

)
);

/**
Expand Down
13 changes: 5 additions & 8 deletions packages/block-editor/src/store/test/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* WordPress dependencies
*/
import { select } from '@wordpress/data';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -126,9 +121,11 @@ describe( 'private selectors', () => {
const __experimentalHasContentRoleAttribute = jest.fn(
() => false
);
select.mockReturnValue( {
__experimentalHasContentRoleAttribute,
} );
getBlockEditingMode.registry = {
select: () => ( {
__experimentalHasContentRoleAttribute,
} ),
};

it( 'should return default by default', () => {
expect(
Expand Down
7 changes: 7 additions & 0 deletions packages/block-editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ const {
wasBlockJustInserted,
__experimentalGetGlobalBlocksByName,
} = selectors;
import { getBlockEditingMode } from '../private-selectors';

getBlockEditingMode.registry = {
select: () => ( {
__experimentalHasContentRoleAttribute: jest.fn( () => false ),
} ),
};

describe( 'selectors', () => {
let cachedSelectors;
Expand Down