Skip to content

Propose store-specific useSelect and useDispatch#26723

Closed
adamziel wants to merge 2 commits into
masterfrom
try/store-specific-useSelect
Closed

Propose store-specific useSelect and useDispatch#26723
adamziel wants to merge 2 commits into
masterfrom
try/store-specific-useSelect

Conversation

@adamziel
Copy link
Copy Markdown
Contributor

@adamziel adamziel commented Nov 5, 2020 •
edited
Loading

Description

🚧 Proof of Concept for discussion purposes! 🚧 some tests are expected to fail for now 🚧

This proposal is building on top of the ideas from #26692.

Just to reiterate:

  • useSelect scoped to an entire registry means a lot of unnecessary dirty-checking, the more granular we can get the faster things can get.
  • useDispatch already requires a store argument, why not make useSelect API consistent with that?

This PR proposes the following API change:

Before

const {
	canUserUseUnfilteredHTML,
	isReady,
	blocks,
	selectionStart,
	selectionEnd,
	reusableBlocks,
	hasUploadPermissions,
} = useSelect( ( select ) => {
	const {
		canUserUseUnfilteredHTML,
		__unstableIsEditorReady: isEditorReady,
		getEditorBlocks,
		getEditorSelectionStart,
		getEditorSelectionEnd,
	} = select( 'core/editor' );
	const { canUser } = select( 'core' );

	return {
		canUserUseUnfilteredHTML: canUserUseUnfilteredHTML(),
		isReady: isEditorReady(),
		blocks: getEditorBlocks(),
		selectionStart: getEditorSelectionStart(),
		selectionEnd: getEditorSelectionEnd(),
		reusableBlocks: select( 'core' ).getEntityRecords(
			'postType',
			'wp_block',
			{ per_page: -1 }
		),
		hasUploadPermissions: defaultTo( canUser( 'create', 'media' ), true ),
	};
} );

After

const {
	canUserUseUnfilteredHTML,
	isReady,
	blocks,
	selectionStart,
	selectionEnd,
} = useCoreEditorSelect(
	( {
		canUserUseUnfilteredHTML,
		__unstableIsEditorReady: isEditorReady,
		getEditorBlocks,
		getEditorSelectionStart,
		getEditorSelectionEnd,
	} ) => ( {
		canUserUseUnfilteredHTML: canUserUseUnfilteredHTML(),
		isReady: isEditorReady(),
		blocks: getEditorBlocks(),
		selectionStart: getEditorSelectionStart(),
		selectionEnd: getEditorSelectionEnd(),
	} )
);

const reusableBlocks = useCoreDataSelect( ( { getEntityRecords } ) =>
	getEntityRecords( 'postType', 'wp_block', { per_page: -1 } )
);
const hasUploadPermissions = useCoreDataSelect( ( { canUser } ) =>
	defaultTo( canUser( 'create', 'media' ), true )
);

That's still kind of repetitive so I thought, why not have a shorthand?

After, but using a shorthand notation

import { useSelect as useCoreEditorSelect } from '@wordpress/editor';
import { useSelect as useCoreDataSelect } from '@wordpress/core-data';

const [
	canUserUseUnfilteredHTML,
	isReady,
	blocks,
	selectionStart,
	selectionEnd,
] = useCoreEditorSelect( [
	'canUserUseUnfilteredHTML',
	'isEditorReady',
	'getEditorBlocks',
	'getEditorSelectionStart',
	'getEditorSelectionEnd',
] );

const reusableBlocks = useCoreDataSelect( ( { getEntityRecords } ) =>
	getEntityRecords( 'postType', 'wp_block', { per_page: -1 } )
);
const hasUploadPermissions = useCoreDataSelect( ( { canUser } ) =>
	defaultTo( canUser( 'create', 'media' ), true )
);

Also, to keep things consistent, useDispatch would also be package-specific like this:

import { useDispatch as useEditPostDispatch } from '@wordpress/edit-post';
const {
	__experimentalSetPreviewDeviceType: setPreviewDeviceType,
} = useEditPostDispatch();

Notes

At the moment, every module exports a member called useSelect. How about exporting a module-specific name such as useEditPostSelect? This would enable IDEs to automatically import these.

Shorthand notation accepts a list of strings. That is okay but potentially prone to typos:

  • Maybe we could make it accept a list of selectors imported from a module? In that case though one could pass an arbitrary selector and access potentially private state members.
  • Maybe export a list of selectors names from a module then? Much like the store name identifier @gziolo explored in Data: Use store instance as param for select and dispatch #26655 ; but then local variable names would often conflict with these imported identifiers.
  • So maybe just validate the list of received strings and throw a good error in case of a typo?

Shorthand notation returns an array ordered in the same way as input. That could contribute to some off-by-1 indexing errors down the road. Now I think about it more, it could easily return an object keyed by selector names like this:

const {
	canUserUseUnfilteredHTML,
	isReady,
	getEditorBlocks: blocks,
	getEditorSelectionStart: selectionStart,
	getEditorSelectionEnd: selectionEnd,
} = useCoreEditorSelect( [
	'canUserUseUnfilteredHTML',
	'isEditorReady',
	'getEditorBlocks',
	'getEditorSelectionStart',
	'getEditorSelectionEnd',
] );

@adamziel adamziel added [Package] Data /packages/data [Package] Core data /packages/core-data labels Nov 5, 2020
@adamziel adamziel self-assigned this Nov 5, 2020
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 5, 2020

Size Change: +1.08 kB (0%)

Total Size: 1.21 MB

Filename Size Change
build/a11y/index.js 1.14 kB -1 B
build/annotations/index.js 3.82 kB +44 B (1%)
build/block-directory/index.js 8.74 kB +18 B (0%)
build/block-editor/index.js 131 kB +24 B (0%)
build/block-library/index.js 146 kB -5 B (0%)
build/blocks/index.js 48.1 kB +21 B (0%)
build/components/index.js 172 kB -14 B (0%)
build/compose/index.js 9.82 kB +8 B (0%)
build/core-data/index.js 12.6 kB +43 B (0%)
build/data/index.js 9.11 kB +337 B (3%)
build/date/index.js 31.9 kB +1 B
build/edit-navigation/index.js 11.2 kB +55 B (0%)
build/edit-post/index.js 306 kB +85 B (0%)
build/edit-site/index.js 22.5 kB +72 B (0%)
build/edit-widgets/index.js 26.4 kB +83 B (0%)
build/editor/index.js 43.1 kB +41 B (0%)
build/format-library/index.js 7.7 kB +1 B
build/keyboard-shortcuts/index.js 2.57 kB +50 B (1%)
build/notices/index.js 1.84 kB +54 B (2%)
build/nux/index.js 3.47 kB +49 B (1%)
build/reusable-blocks/index.js 3.09 kB +38 B (1%)
build/rich-text/index.js 13.3 kB +38 B (0%)
build/url/index.js 4.06 kB -2 B (0%)
build/viewport/index.js 1.88 kB +43 B (2%)
ℹ️ View Unchanged
Filename Size Change
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 9.02 kB 0 B
build/block-library/editor.css 9.02 kB 0 B
build/block-library/style-rtl.css 7.9 kB 0 B
build/block-library/style.css 7.89 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.2 kB 0 B
build/data-controls/index.js 772 B 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@adamziel adamziel mentioned this pull request Nov 5, 2020
Merged
6 tasks
@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Nov 5, 2020

Probably just a micro-optimization that may not be worth pursuing at this point but I just had this idea so I'll drop it here: A declarative shorthand notation would make it possible keep track of all the nullary selectors that are currently listened to. This means we could recompute them only once on every store change instead of once per useSelect.

@adamziel adamziel mentioned this pull request Nov 5, 2020
Merged
@adamziel adamziel marked this pull request as draft November 5, 2020 14:00
@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Nov 5, 2020 •
edited
Loading

I just turned this back into a draft, let's see if we can get alignment around #26692 first

@adamziel
Copy link
Copy Markdown
Contributor Author

adamziel commented Nov 9, 2020

Closing for now, see #26692

@adamziel adamziel closed this Nov 9, 2020
@youknowriad youknowriad deleted the try/store-specific-useSelect branch November 9, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@draganescu draganescu Awaiting requested review from draganescu

@youknowriad youknowriad Awaiting requested review from youknowriad

@noisysocks noisysocks Awaiting requested review from noisysocks

@talldan talldan Awaiting requested review from talldan

@gziolo gziolo Awaiting requested review from gziolo

@sirreal sirreal Awaiting requested review from sirreal

@ellatrix ellatrix Awaiting requested review from ellatrix

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@atimmer atimmer Awaiting requested review from atimmer

@cameronvoell cameronvoell Awaiting requested review from cameronvoell

@etoledom etoledom Awaiting requested review from etoledom

@guarani guarani Awaiting requested review from guarani

@kevin940726 kevin940726 Awaiting requested review from kevin940726

@nerrad nerrad Awaiting requested review from nerrad

Assignees

@adamziel adamziel

Labels

[Package] Core data /packages/core-data [Package] Data /packages/data

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@adamziel