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.

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

Data: Add useSelector hook #21057

Closed
wants to merge 2 commits into from
Closed

Data: Add useSelector hook #21057

wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 20, 2020

Related Slack discussion (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1584734770269400

This pull request seeks to try an alternative function signature to useSelect.

Where what was previously expressed as...

function MyPlugin() {
    const currentPostType = useSelect( ( select ) => select( 'core/editor' ).getCurrentPostType() );
    // ...
}

...could instead be expressed as:

function MyPlugin() {
    const currentPostType = useSelector( 'core/editor' ).getCurrentPostType();
    // ...
}

In its current state, it's intended to be explored as a prototype. Some observations:

  • The refactored component is a +26 -72 revision.
    • Observation: Often less code to write (can be seen also by the snippets above)
  • Externalized selector functions are removed.
    • Observation: This can serve as a solution to the issues concerning variable shadowing of assignment of variables in a useSelect callback using same names as that of their parent scope.
    • Observation: However, these now return the selector function, not the result of that function
      • Bad: In current usage, some component implementations may need to either call the selector multiple times, or assign its result to a variable, where the latter would likely reintroduce the challenges around no-shadow.
      • Good: Calling the function inline in conditions can often be an optimization, and avoids calling the selector altogether in many cases:
  • Observation: A component will always render on any store change
    • Neutral: The selectors would be called regardless of whether their results are the same or not
    • Bad: We're not avoiding unneeded renders if selector results would be the same
    • Good: A future optimization for store specific subscriptions (previously @wordpress/data: Allow for subscribing listeners to specific stores. #15483, Data: withSelect: Subscribe only to reducer keys referenced in mapSelectToProps #12877) would be made trivial by this API. With this, renders could at least be limited to changes within the specific stores for which the component is concerned.
    • Good: We're at least benefitting from possibly not calling selectors at all (see above points about short-circuit evaluation and early returns)
    • Good: Unlike useSelect, we're never performing a shallow comparison.
    • Bad (Blocker?): If the render is allowed to cascade to its children, a top-level component using useSelector would not only needlessly re-render itself, but also all of its descendents... every time state changes.

Note: Strictly speaking, this could be implemented as an overloaded form of useSelect. I don't hold strong feelings about this, but implemented it as a separate useSelector for the proposal due to:

  • Easier to analyze the implementation in isolation (for purposes of prototyping and review)
  • Overloaded functions are hard to document, harder to implement, harder to type (as in "type-checking")
  • Possible performance overhead if each call to useSelect must make up-front decision on how to handle logic of its hook
  • Said "up-front decision" is a condition, and thus violates the first of the Rules of Hooks.
  • Semantically, it can make sense as two separate implementations: useSelect, as indicated by its name, provides a callback with the select function. By contrast, useSelector implies some intention to gain access to selectors specifically. More accurately, it could be useSelectors, since it returns an object of a store's selectors. To me, it feels more readable in the singular form.

@aduth aduth added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress [Package] Data /packages/data labels Mar 20, 2020
@github-actions
Copy link

Size Change: -16 B (0%)

Total Size: 856 kB

Filename Size Change
build/autop/index.js 2.58 kB +1 B
build/block-editor/index.js 100 kB -96 B (0%)
build/block-library/index.js 110 kB -1 B
build/blocks/index.js 57.5 kB +1 B
build/components/index.js 191 kB -6 B (0%)
build/compose/index.js 6.2 kB -5 B (0%)
build/core-data/index.js 10.6 kB +1 B
build/data/index.js 8.29 kB +94 B (1%)
build/edit-site/index.js 5.56 kB -1 B
build/edit-widgets/index.js 4.43 kB -4 B (0%)
build/editor/index.js 43.8 kB +3 B (0%)
build/element/index.js 4.44 kB +2 B (0%)
build/format-library/index.js 6.94 kB -7 B (0%)
build/keyboard-shortcuts/index.js 2.3 kB +1 B
build/nux/index.js 3.01 kB +1 B
build/redux-routine/index.js 2.84 kB +1 B
build/rich-text/index.js 14.4 kB -1 B
build/server-side-render/index.js 2.55 kB -1 B
build/url/index.js 4.01 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.24 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/style-rtl.css 7.41 kB 0 B
build/block-library/style.css 7.42 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.47 kB 0 B
build/edit-post/style.css 8.46 kB 0 B
build/edit-site/style-rtl.css 2.62 kB 0 B
build/edit-site/style.css 2.62 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member Author

aduth commented Mar 20, 2020

Tangentially, in thinking about this, I've been contemplating whether it'd actually be difficult to implement in useSelect as well:

Pseudo-code:

function useSelect( mapSelect ) {
	const stores = useRef( new Set );
	const registry = useRegistry();
	const [ mapOutput, setMapOutput ] = useState( () => mapSelect( ( storeKey ) => {
		stores.current.add( storeKey );
		return registry.select( storeKey );
	} );

	useLayoutEffect( () => {
		function onStoreChange() { /* ... */ }
		onStoreChange();
		registry.subscribe( Array.from( stores.current ), onStoreChange );
	} );

	return mapOutput;
}

@aduth
Copy link
Member Author

aduth commented Mar 21, 2020

I had started to consider another alternate take on this signature before its fatal flaw eventually revealed itself to me. The general idea was to use property descriptors (getters) as a flagging mechanism in the destructuring of a useSelect result, such that via the following code:

function MyComponent() {
    const { getCurrentPostType } = useSelector( 'core/editor' );

    // ...
}

...the mere act of destructuring the result of the hook would be enough to know that this component has a dependency on the result of getCurrentPostType (demo). I understand this to be very much alike to how hooks themselves work, and is also subject to many of the same restrictions ("Rules").

Once it can be known which selectors a component depends upon, it enables a few key optimizations:

  • We can invert the subscription. No longer does every component need to check see if there is a new value. Instead, this can be run once by the store when it changes, and because we know which selectors are in use and by which components, we know exactly when a component needs to render.
  • We only need to compute the result of a selector a single time per state change, regardless of how many components use it.
  • We could even lazily evaluate those results, and determine whether a component needs to update by iterating through its dependencies, seeing if a result has changed. At the very first moment a change is detected, the task can be aborted early, as the component is known to need to render. Any other selectors which hadn't yet been evaluated will be evaluated when (if!) they are called in the following logic of the component render.

There is a lot of similarity between this and...

  • Try: Data: Cache selector results by state via WeakMap #6458 - Try: Data: Cache selector results by state via WeakMap
    • Both seek to evaluate a selector result at most one time per state change
  • (I'm failing to find it now, but I recall reading at one point a blog post detailing a very similar approach of inverting the traditional Redux subscription to one where the store knows about the components and when to update them)

Now, the fatal flaw: Selector arguments. Just as I eventually discovered with #6458, this only works if a selector receives no arguments. If a selector is expected to receive arguments, the store can't know how to re-evaluate the result, particularly if that selector may only be called later in the component.

I'm still intrigued by the idea, and there may be some elements of this which could be leveraged into something more viable. Hence sharing for posterity's sake, despite not being feasible.

@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

This was intended as a prototype. I think the API interface is definitely more pleasant to work with than the current implementation. That said, there are some very concerning implications of this sort of change which need careful consideration and consensus.

It may be good to revisit or at least reference this in the future for prior art.

@aduth aduth closed this Apr 3, 2020
@aduth aduth deleted the try/use-select-store-key branch April 3, 2020 16:10
@aduth
Copy link
Member Author

aduth commented Apr 6, 2020

Tangentially, in thinking about this, I've been contemplating whether it'd actually be difficult to implement in useSelect as well:

[...]

Related prior art: #12877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Status] In Progress Tracking issues with work in progress [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant