@wordpress/data: Introduce useSelect custom hook. #15737

nerrad · 2019-05-20T12:44:20Z

Note: For latest wrap up comments on this pull see also this comment

Description

See #15473 for background and #15512 for initial experimental approach to the new useSelect. The initial iteration of this approach used some code provided by @epiqueras (see comment, but did require modification. In this approach to useSelect:

The signature is `useSelect( mapSelect: function, deps: array )`

mapSelect receives the registry.select function as the first argument, and registry as the second. This, follows roughly the signature of the mapSelectToProps signature withSelect currently exposes (so this preserves that api). The mapSelect does not receive the "ownProps" because in most cases of just hook usage, the callback provided will internally be working with props. I'm on the fence with this because it does put more burden on implementors to account for possible stale prop usage in their callbacks but this is mitigated for most cases by the fact useSelect always invokes the latest mapSelect provided on render (via usage of useRef).

deps should an array of values used for memoizing the provided mapSelect (similar in behaviour to react hooks with dependencies). Internally, the new withSelect wraps the internal component using useSelect with pure.

Other

useRegistry hook is exported and use exported context objects ( internally only - no need to export the entire context with the exposure of the hook).
I implemented the hook in a new withSelect export which eliminates a lot of code and allows us to measure behaviour across all of GB in one swoop.
I kept the old withSelect file for reference temporarily in this pull, but it will get removed if not used.

How has this been tested?

Currently I've just tested playing around with blocks in the editor and generally looking for breakage. Things to watch for will be:

e2e test failures (which is in part why I published this pull as opposed to making it a draft).
unit tests are NOT updated yet, so it's expected there will be some breakage there.

Types of changes

These are mostly internal changes so it's not expected to be a breaking change. The current signature for mapSelectToProps is preserved on implementations of withSelect so no effects there.

However, this does impact a lot of code (because of the withSelect implementation of the new hook). So the potential for regressions is significant here.

Next Steps:

Gather feedback, make changes
Address any issues (from feedback, e2e test failures etc)
Update unit tests
Update documentation (inline and README)
Update affected CHANGELOG.md in packages.
Remove WIP status and submit for full review.
Release?

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

nerrad · 2019-05-20T12:46:33Z

packages/data/README.md

-
-_Returns_
-
-   `Component`: Enhanced component with merged state data props.


This documentation will get replace once I add inline docs to the new withSelect. I just want to gather initial feedback on approach before doing so.

I guess this needs an update too.

nerrad · 2019-05-20T12:48:09Z

packages/data/src/components/use-select/index.js

+	useIsomorphicLayoutEffect( () => {
+		latestIsAsync.current = isAsync;
+		renderQueue.flush( queueContext );
+	}, [ isAsync ] );


Assuming the approach is sound, these could be extracted to internal only named hooks to make it a bit easier to follow in the main exported hook.

You can include this in the previous hook:

useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; if ( latestIsAsync.current !== isAsync ) { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); } isMounted.current = true; } );

hmm, ya I guess. I mostly liked the separation because the flush would only happen if isAsync changes (hence the depedency (so removes the need for the conditional check). So tomahtoes/tomatoes :)

Yeah, I was just thinking it could cause some issues with the order of effect clean-up/execution, but I think it's fine.

nerrad · 2019-05-20T12:51:14Z

packages/data/src/components/with-select/with-select-old.js

@@ -0,0 +1,208 @@
+/**


This file is just kept in this pull initially as a reference to compare the logic in the useSelect hook against what withSelect was doing. Once an approach is signed off on, then this can get removed.

youknowriad · 2019-05-20T13:30:30Z

packages/data/src/components/use-select/index.js

+ */
+// const useIsomorphicLayoutEffect = typeof window !== 'undefined' ?
+// 	useLayoutEffect : useEffect;
+const useIsomorphicLayoutEffect = typeof window !== 'undefined' ?


Do we really care about this? I mean even if we do (server-side usage), this feels like something that should be done across the whole codebase. I wonder if we should keep it separate and just use useLayoutEffect here.

ya I wondered the same thing. I also tried just using useEffect and the page doesn't load at all (which I guess is to be expected). Which makes me wonder if this would even work with server side rendering.

useEffect is a no-op in the server.

in a7b799f I just switched to useLayoutEffect only. Redux needs to do the isomorphic effect because as a library it's used in more environments but until GB supports server side rendering generally we shouldn't implement.

packages/data/src/components/use-select/index.js

youknowriad · 2019-05-20T13:33:38Z

packages/data/src/components/use-select/index.js

+	}, [ registry, nextProps ] );
+
+	useIsomorphicLayoutEffect( () => {
+		const unsubscribe = registry.subscribe( () => {


I expect this will break the re-rendering order, because the children will subcribe before the parents. Right?
I think the solution might be to subscribe synchronously at the render level (but only manage to do it once).

Yeah, see https://react-redux.js.org/next/api/hooks#stale-props-and-zombie-children

hmm so the subscription callback would likely have to be a ref maybe?

This basically means for me that subscriptions should happen at the root level of the hook and not inside a useEffect.

Something like

const registry = useRegistry(); const isAsync = useAsyncMode(); const previousRegistry = usePrevious( registry ); const previousIsAsync = usePrevious( isAsync ); if ( registry !== previousRegistry || isAsync !== previousIsAsync ) { // Trigger subscription }

https://gist.github.com/markerikson/faac6ae4aca7b82a058e13216a7888ec

reduxjs/react-redux#1179

Do you think you'd be able to provide an example (codepen or something) where the React Async rendering would break this. Asking because that would be a good way to try and test alternative implementations addressing this issue.

That would take a while to set up. The entire React API relies on render being idempotent. Note that the current implementation of withSelect also has this issue as it has side effects in the constructor.

From the React docs:

Conceptually, React does work in two phases:

The render phase determines what changes need to be made to e.g. the DOM. During this phase, React calls render and then compares the result to the previous render.

The commit phase is when React applies any changes. (In the case of React DOM, this is when React inserts, updates, and removes DOM nodes.) React also calls lifecycles like componentDidMount and componentDidUpdate during this phase.

The commit phase is usually very fast, but rendering can be slow. For this reason, the upcoming async mode (which is not enabled by default yet) breaks the rendering work into pieces, pausing and resuming the work to avoid blocking the browser. This means that React may invoke render phase lifecycles more than once before committing, or it may invoke them without committing at all (because of an error or a higher priority interruption).

Render phase lifecycles include the following class component methods:

constructor

componentWillMount

componentWillReceiveProps

componentWillUpdate

getDerivedStateFromProps

shouldComponentUpdate

render

setState updater functions (the first argument)

Because the above methods might be called more than once, it’s important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems, including memory leaks and invalid application state. Unfortunately, it can be difficult to detect these problems as they can often be non-deterministic.

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following methods:

Class component constructor method

The render method

setState updater functions (the first argument)

The static getDerivedStateFromProps lifecycle

Note:
This only applies to development mode. Lifecycles will not be double-invoked in production mode.

For example, consider the following code:

class TopLevelRoute extends React.Component { constructor(props) { super(props); SharedApplicationState.recordEvent('ExampleComponent'); } }

At first glance, this code might not seem problematic. But if SharedApplicationState.recordEvent is not idempotent, then instantiating this component multiple times could lead to invalid application state. This sort of subtle bug might not manifest during development, or it might do so inconsistently and so be overlooked.

By intentionally double-invoking methods like the component constructor, strict mode makes patterns like this easier to spot.

Thanks for sharing. I think we probably have a lot of components that suffer from these issues. Makes me think we don't really support async mode. Not to say that we shouldn't but if feels like something we should invest in at a more global level.

From what I understand, say we have a global counter, we could increment (side effect) from within hooks. If the render function increments this counter, we'll still be certain that children components will pick a higher-number from this counter than their parent as even if react could double invoke parents render function ... it will still call the render of the children after the parents or is this a wrong assumption (assuming initial rendering)?

Here is the execution order:

render parent render childA render childB useLayoutEffect cleanup childA useLayoutEffect cleanup childB useLayoutEffect cleanup parent useLayoutEffect effect childA useLayoutEffect effect childB useLayoutEffect effect parent useEffect cleanup childA useEffect effect childA useEffect cleanup childB useEffect effect childB useEffect cleanup parent useEffect effect parent

You can't run side effects in a parent before running them in its children, because side effects only happen after the first render. This is true without hooks as well. The only alternative I see would be to stagger-render (pause render at each level of the tree to run side effects), but performance would suffer, and you would still have issues when children are rendered conditionally/dynamically.

epiqueras · 2019-05-20T14:25:00Z

packages/data/src/components/use-select/index.js

+	 */
+	useIsomorphicLayoutEffect( () => {
+		onStoreChange();
+	}, [ registry, nextProps ] );


onStoreChange can be the dependency here.

It seems that this means this hooks makes the "nextProps" (the dependencies of the callback) mandatory. I'd think that we should also support a naive version where the mapping is attempted on each render if nextProps is undefined.

It seems that this means this hooks makes the "nextProps" (the dependencies of the callback) mandatory.

undefined === undefined, so it will only run when the registry changes.

I almost think it should be mandatory. This allows useSelect to effectively "know" the dependencies. I'm thinking then it's possible for implementations of just the hook to describe what dependencies the provided mapSelect callback has.

Thus the new withSelect hoc is provided all the props coming in from the parent as dependencies (simply because there's no way to know what mappers will be provided by the components composed with withSelect).

I almost think it should be mandatory.

I agree with that but conceptually speaking, it behaves differently than the useEffect dependencies and could mislead people.

useEffect( something ) updates on each render, I expect useSelect( something ) to update on each render as well.

nextProps is problematic, because it's an object so you put the burden on the user to memoize it.

onStoreChange doesn't need to be memoized. It can just read refs.

nextProps should be an array that gets passed as the dependency array to the useIsomorphicLayoutEffect of this chat thread.

You also don't want to run this during the first render, so this should come before the effect that sets isMounted, and it should check for it to be true.

There was another comment attached to an outdated code snippet about assuming nextProps is an array and spreading it here (thus being more consistent with dependency shape with useEffect). I think that's probably what we should do here and have withSelect do the necessary implementation to follow that signature expectation for useSelect.

Yeah that's better, but it still means the component re-renders twice. First when nextProps change and then after the selector is run.

Take a look at this approach: https://github.com/reduxjs/react-redux/blob/v7-hooks-alpha/src/hooks/useSelector.js

Here:
https://github.com/reduxjs/react-redux/blob/0e41eaeebf5d8d123daaa50a91ee1c219c4830de/src/hooks/useSelector.js#L61-L64
You would also check if nextProps have changed.

This actually removed the need for nextProps.

I updated my gist to implement all the behavior you want. Look at how we can implement withSelect now.

https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630

epiqueras · 2019-05-20T14:29:49Z

packages/data/src/components/use-select/index.js

+			unsubscribe();
+			renderQueue.flush( queueContext );
+		};
+	}, [ registry ] );


onStoreChange is also a dependency here.

Hmm, there is risk of that causing more subscription re-renders though right?

I don't think onStoreChange should be memoized. It can be a ref if you also need to use it in the other effect.

See #15737 (comment)

see changes in 69f3b80

epiqueras · 2019-05-20T14:32:04Z

packages/data/src/components/with-select/with-select-new.js

@@ -0,0 +1,25 @@
+/**


youknowriad · 2019-05-20T14:52:18Z

packages/data/src/components/use-select/index.js

+				setMapOutput( newMapOutput );
+			}
+		}
+	}, [ registry, nextProps ] );


I feel this should be ...nextProps and not nextProps

epiqueras · 2019-05-20T18:02:14Z

@youknowriad @nerrad

I gave this some more thought now that we want the callback to run again when nextProps changes.

It makes more sense for the callback to be called again when it changes. This was needed regardless, because the callback might have new closures every time it changes and the component wouldn't reflect that until the next store update. Now, users can just memoize their callback if they don't want it to run on every render, using a traditional hook dependency array. This is what I did in my implementation of withSelect, passing Object.values( ownProps ) as the dependencies to useCallback. This makes the code simpler and gives developers more freedom in their optimization logic.

See the updated gist: https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630.

I also added the queue context and flushing that @nerrad added in this PR. You can copy and paste this into this PR if you don't see any issues with it.

nerrad

Thanks again @epiqueras for the code example. It's looking more and more like the redux useSelect implementation :)

This appears to work in rough testing and I mostly implemented the code from your gist with an exception wrt to the memoized mapSelect callback in the withSelect hoc (see my comments).

nerrad · 2019-05-20T22:44:05Z

packages/data/src/components/use-select/index.js

+ * Fallback to useEffect for server rendered components because currently React
+ * throws a warning when using useLayoutEffect in that environment.
+ */
+const useIsomorphicLayoutEffect =


I re-implemented this. I realize that GB as a whole isn't really supporting server rendering but with this being a separate package that could be used in a server rendering environment, this might be a valid thing to do (and follows a pattern that Redux is using for their hooks)

nerrad · 2019-05-20T22:48:19Z

packages/data/src/components/with-select/with-select-new.js

+				ownProps,
+				registry
+			),
+			[ ownProps ]


Note, I could not do Object.values( ownProps ) here because of this error:

Warning: The final argument passed to useCallback changed size between renders. The order and size of this array must remain constant.

I experimented with generating a consistent array of values from the prop keys in the same order over multiple renders (and filling missing values with undefined) but this caused the same error (because it looks like the number of props can increase between re-renders as well). This almost smells like a problem with implementation in some component in the editor tree but I'm uncertain what to do here. On the surface just passing in the prop object seems to be sufficient for this case.

ownProps changes on every render so this is like not memoizing it and running the callback every time.

I think we can just wrap the inner component in a React.memo.

Updated: https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630#file-with-select-js

ownProps changes on every render so this is like not memoizing it and running the callback every time.

I couldn't implement React.memo because it resulted in invalid blocks on existing content. I'm not sure of the internal workings of it but it's a no go based on results. So I implemented pure which should work similarly.

I'm not seeing much difference in behaviour between the two when I do a rough comparison using the react dev profiler tool:

However pure does seem marginally better so probably good to use it.

memo doesn't implement shouldComponentUpdate like pure does, it just adds a special flag for the renderer to handle.

Both have the same functionality, but internally React has different rules for when to ignore a shouldComponentUpdate or a memo.

It sounds like these blocks are relying on some indeterministic and undocumented behavior.

The blocks throwing the invalidation issue were basic blocks included with GB (paragraph block for instance).

Could there be a state/props mutation somewhere in there?

nerrad · 2019-05-20T23:03:00Z

@youknowriad there's still some failing e2e tests in here, it's unclear to me what the issue is with the tests (because of my lack of familiarity with them) but I picked a few of the tests that I can replicate in testing myself (such as re-enabling nux tips) and I'm not reproducing the e2e fails.

nerrad · 2019-05-20T23:13:19Z

For reference, here are the failing e2e test:

Failed Test	Could I reproduce?	Notes
FAIL packages/e2e-tests/specs/plugins/container-blocks.test.js / InnerBlocks Template Sync › Ensures blocks without locking are kept intact even if they do not match the template	n/a	I didn't try
FAIL packages/e2e-tests/specs/links.test.js / Links › should contain a label when it should open in a new tab	No
FAIL packages/e2e-tests/specs/nux.test.js / New User Experience (NUX) › should enable tips when the "Enable tips" option is toggled on	No

youknowriad · 2019-05-21T07:52:10Z

packages/data/src/components/use-select/index.js

+	useIsomorphicLayoutEffect( () => {
+		const onStoreChange = () => {
+			if ( isMounted.current ) {
+				const newMapOutput = latestMapSelect.current( registry.select, registry );


So So if I'm reading this properly, the mapping function is never called when the mapping function changes or when the props used in the mapping function changes. This feels like a bug to me, what if a prop changes and the store wasn't updated in the meantime.

I guess what I'm saying here is I'm still not sure why we dropped the dependencies array from the arguments.

So if I'm reading this properly, the mapping function is never called when the mapping function changes

I think it is:

if ( latestMapSelect.current !== mapSelect ) {

latestMapSelect gets set to the current mapping function on ever render and the next render compares that value with the previous. If they differ, then the function is invoked again to get the mapOutput.

I guess what I'm saying here is I'm still not sure why we dropped the dependencies array from the arguments.

The dependency is now on the mapSelect callback. This gives more control to developers to optimize on their mapSelect function.

I think there's two approaches we could go here:

useSelect requires a dependency array and uses that to optimize rendering.

Disadvantages:

withSelect will be harder to optimize for because ownProps fluctuates too much so we can't just do Object.values( ownProps )

Easier for developers to shoot themselves in the foot because their dependency array doesn't correctly all the dependencies in their mapSelect callback (granted that might be somewhat resolveable via an ES lit rule).

Advantages:

enforces optimal approaches for performance.

makes the dependencies more clear and obvious.

Developers do their own memoization of the mapSelect callback.

Disadvantages:

performance responsibility is shifted to developers, so they can still shoot themselves in the foot wrt performance (granted, something being non performant via not working are too different things :) )

Advantages:

More flexibility for developers with regards to how they optimize things.

withSelect is a bit more easier to optimize (although there's still some difficulties as noted in the comments for my latest commit).

I have a strong preference for having a dependencies argument in the signature, mimic useEffect, and absorb the optimizations in the framework personally.

Will we need more fine-tuning? Maybe, but it seems like this could be a useSelectAdvanced similar to how there's a connectAdvanced in react-redux

I think the connector's job is to update the component when it has to update. Memoizing a selector for performance is a separate concern.

Keep in mind that a lot of the time, specially for computationally expensive selectors, they will already be defined elsewhere without closures over props, removing the need for inline hook callback memoization. This codebase is a good example of that.

I also like the fact that the public API remains simple and developers have one less thing to learn. They already have to learn how to use primitive memoization hooks, so we can build on that.

Let's look at a few examples:

// Callback never changes. // Performance is good. useSelect( importedSelector ); // Callback changes and re-runs on every render. // `.getItem` is memoized. // Performance is good. useSelect( ( select ) => select( 'store' ).getItem() ); // Callback changes and re-runs on every render. // `.getItem` is memoized with `prop`. // Performance is good. useSelect( ( select ) => select( 'store' ).getItem( prop ) ); // Callback changes and re-runs on every render. // `.getItem` is not memoized. // Performance suffers. // We memoize ourselves. useSelect( useCallback( ( select ) => select( 'store' ).getItem( prop ), [ prop ] ) ); // Callback changes and re-runs on every render. // `.getItem` is not memoized. // Performance suffers. // We delegate memoization to the implementation. useSelect( ( select ) => select( 'store' ).getItem( prop ), [ prop ] );

See where the two approaches differ in the last two calls. My main issue with having the dependencies array is that the user has to learn a new interpretation of it. Does it memoize the callback everywhere? Only for renders? Does it always use the latest one in store updates? Does it update the memoized value after that?

It very quickly makes it much harder to think about and learn. Developers already know how useCallback works and it's much easier to say: This hook calls your callback every time it changes and when the store updates, and then returns its output. If your callback is expensive, you can memoize it, because it's only called when it changes or when the store updates.

I think the connector's job is to update the component when it has to update. Memoizing a selector for performance is a separate concern.

The same thing could have been said about useEffect hook. Why the React team decided to implement the dependencies there and not rely on useCallback to ensure it runs the effect only when the callback changes?

I understand that a "pure" implementation is better for an advanced developper that understands how to override the default behavior and build on top of it but let's take a look at the common use-case from a user's perspective:

const { block } = useSelect( ( select ) => { return { block: select( 'core/editor' ).getBlock() // call other selectors }; } );

In our previous attempts to improve the performance of the editor, the fact that the selectors were memoized didn't prove to be enough to address the editor performance issues once we load a lot of blocks. The problem was that a lot of selectors were being run (memoized or not) on each subscribe. So for me, the more we avoid running selectors entirely, the better is.

I understand that I'm basing this on previous metrics, that might not transpose 1-1 to the current use-cases. Ideally, we'd perform performance tests and compare.

My main issue with having the dependencies array is that the user has to learn a new interpretation of it

That's my main issue with the dependencies-less implementation as well. Users need to learn how to memoize things by them selves (using other hooks) while the dependencies array is a common pattern already used in useEffect.

Ultimately, I think we should take a path, mark it as experimental and evaluate it with real usage.

In thinking about this more. It seems that the behavior of both approaches is the same if we omit passing any second argument and if we don't check for the callback refrence (always run the callback on render and rely on memoization), that suggests that we could potentially adopt a first version without any attempt to memoize (based on callback or based on dependencies) as a common ground and adapt using concrete measures.

The functionality of useEffect is completely different. It's not just about closures. There you might want to run the effect more or less times for other reasons. That's why they didn't rely on useCallback.

This is really just about:

useSelect( mapSelect, [ dep ] )

vs.

useSelect( useCallback( mapSelect, [ dep ] ) )

The first one does have less code and I guess it's fine if we document it well enough. Something like:

This hook calls your callback every time it changes and when the store updates, and then returns its output. If your callback is expensive, you can pass a dependency array as the second parameter to memoize it, because we will only call it when it changes or when the store updates.

I updated the gist: https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630.

nerrad · 2019-05-21T12:26:27Z

Looks like the e2e tests all pass on the latest push to this branch 🎉 so as far as I can tell here's some remaining points to iron out:

require dependencies or not for the signature.
potential for zombie/stale props in children (see discussion beginning here. Although this appears to be more of a global issue with GB and something likely handled in a separate pull? (if I understand correctly, the current released implementation of withSelect suffers from this problem too?)

Anything else?

youknowriad · 2019-05-21T12:34:54Z

if I understand correctly, the current released implementation of withSelect suffers from this problem too?)

The current implementation of withSelect don't suffer from the zombie/stale props issue but it's broken in React Async mode. My proposal on that thread have the same characteristics: it fixes the zombie/stale props issue but break React Async mode. At the moment, it's very hard for me to find a solution that solves both at the same time 100%. So our options are:

Since React Async mode is broken more globally in Gutenberg, ship my "broken" fix :) to avoid introducing bugs (removing blocks in some situations can cause these errors).
Try the approach used by react-redux to try/catch selector calls. The main uncertainty for me here is whether this is a legit approach for us because with @wordpress/data calling select means calling a selector (which is pure) but also calling a resolver some times which is "unpure". In their document they suggest that the approach don't work when the selector is not pure which makes me wonder if it would work for us.

epiqueras · 2019-05-21T14:49:19Z

@nerrad

require dependencies or not for the signature.

See discussions and changes here: #15737 (comment).

@youknowriad

The current implementation of withSelect don't suffer from the zombie/stale props issue but it's broken in React Async mode.

It does suffer from it. It subscribes in constructors. These are called top-down parent to children, but not when children are rendered dynamically. I.e. lists, conditionals, switches, code-splitting, etc.

The only way to solve this is for subscriptions to have a sense of location in the tree. This can be done by wrapping every subscribed component in a new context provider that overrides the subscription for its entire sub-tree. This is what connect does in React Redux. The subscription class has a tree of subscriptions: https://github.com/reduxjs/react-redux/blob/v7-hooks-alpha/src/utils/Subscription.js. This can be added to @wordpress/data in a separate pull request.

I do think the try/catch does help, albeit in a duct tape way. You're basically giving the callback a second chance. If there was an error caused by stale props, the second call will have the correct props. If the component is a zombie, it won't be called again. The only thing it doesn't solve is when stale props don't cause an error. In those cases the component will be outdated until the mapSelect callback changes or the store is updated again. Most of the time, the former will happen immediately so users will never see the outdated state.

@nerrad I've updated the gist to handle this: https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630

youknowriad · 2019-05-21T15:08:51Z

t does suffer from it. It subscribes in constructors. These are called top-down parent to children, but not when children are rendered dynamically. I.e. lists, conditionals, switches, code-splitting, etc.

That's a good point 👍 I guess the fact that we didn't notice might indicate that it's harder to trigger but you're right.

youknowriad · 2019-05-21T15:11:14Z

@epiqueras Do you think the "try/catch" approach could work for us even if our selectors are not entirely pure? this was suggested as something that would break the approach in the document you shared.

If that's the case, I'd be happy to move forward with that proposal.

epiqueras · 2019-05-21T15:32:29Z

@youknowriad

@epiqueras Do you think the "try/catch" approach could work for us even if our selectors are not entirely pure? this was suggested as something that would break the approach in the document you shared.

If that's the case, I'd be happy to move forward with that proposal.

A selector's corresponding resolvers will either get triggered in the first or a second call to mapSelect, depending on if and where it throws, or twice if the selector takes stale props (that it has not used before) as arguments. This looks fine to me.

I think they were more worried about a side effect that depended on props running with stale props and then somehow affecting the next render. This is not the case for how resolvers work in @worpress/data. The resolver/side effect would just be triggered again with the correct props in the second call.

nerrad · 2019-05-21T15:39:07Z

Thanks again @epiqueras for the explanation and the code examples. I'll try and get another commit up sometime later today.

nerrad · 2019-05-22T00:00:02Z

In 585795c I've implemented the try/catch approach as outlined by @epiqueras which is also similar to what Redux is doing in their useSelect hook. Note, I still am unable to use memo because of an error thrown on existing content. The error thrown is:

'The "edit" property must be a valid function.'

Which is found in the block api:

gutenberg/packages/blocks/src/api/registration.js

Lines 115 to 119 in 5c34f2f

    
           if ( 'edit' in settings && ! isFunction( settings.edit ) ) { 
        
           	console.error( 
        
           		'The "edit" property must be a valid function.' 
        
           	); 
        
           	return;

This error happens for both core blocks (in this case core/paragraph) and custom blocks (I'm testing with a block I've written too because it uses withSelect).

For now I'm using pure instead of React.memo.

nerrad · 2019-05-22T00:58:19Z

Looks like all e2e tests pass on the latest build here as well so if this approach looks like what we want to go with in the initial iteration I'll get to work on updating unit tests and adding unit test coverage for this along with doing up the necessary CHANGELOG.md updates etc.

nerrad · 2019-05-22T01:21:06Z

woops looks like I missed pushing a commit (see c8c2ca8). @epiqueras this is a bit different than your gist because your gist had some bugs.

nerrad · 2019-05-22T01:45:14Z

I still don't think any of the last pushes are sufficient as I think there's still scenarios where the incoming select hasn't changed but the dependencies has and the previous mapOutput will still be returned. So it's unclear to me yet how the deps arg affects things. I think what needs to happen is that we'll need to use a useRef for dependencies as well and shallowly compare previous with current along with the shallow compare of the mapSelect.

nerrad · 2019-05-25T19:24:20Z

Alright with the latest commits this is ready for final review. Please note the following changes though:

A `withSelect` test asserted dispatched actions on mount (both in the component constructor and the `componentDidMount` lifecycle method).

A couple points about this test update:

dispatching in the constructor seems to be a "no-no" for react (breaks React Suspense and React Async Mode), so should we be asserting that works?
I think supporting dispatches in componentDidMount is acceptable (need confirmation here).

In order to retain previous behaviour, I needed to make this change in the useSelect hook:

gutenberg/packages/data/src/components/use-select/index.js

Lines 143 to 149 in cf2e824

    
           // catch any possible state changes during mount before the subscription 
        
           // could be set. 
        
           if ( latestIsAsync.current ) { 
        
           	renderQueue.add( queueContext, onStoreChange ); 
        
           } else { 
        
           	onStoreChange(); 
        
           }

The changes effectively mean that the mapSelect callback is run twice on initial mount. However, the extra call does not happen on re-renders unless the registry changes or the component was unmounted and remounted.

I think we kinda need to keep these changes, otherwise any dispatches done in the componentDidMount lifecycle method will not get picked up. However, it should be noted that this was primarily added as a potential back-compat issue with including this in withSelect.

The performance overhead from the additional calls can be mitigated by memoized mapSelect callbacks either from the passed in dependencies on useSelect or external for those using withSelect. Regardless, as noted via the performance measurements section in this comment, the impact is neglible and not any worse than current behaviour.

The test for parent subscriptions running before children is no longer true.

Basically, with the useSelect implementation, child subscriptions do fire before the parent. I think this was to be expected based on previous conversations in this pull. It is accounted for with the early run of the mapSelect callback on re-renders.

It should be pointed out that although there is an increase in the number of times mapSelect callbacks are invoked (as demonstrated by the tests), the expected number of renders is not affected (matching expectations from the the tests before changes).

Performance

I used the performance tests available on #14506 for measuring impact and as @youknowriad already pointed out, there's not much difference:

Master

Average time to load: 21075ms
Average time to DOM content load: 20194ms
Average time to type character: 152.36ms
Slowest time to type character: 284ms
Fastest time to type character: 93ms

This branch

Average time to load: 20970ms
Average time to DOM content load: 20017ms
Average time to type character: 140.53ms
Slowest time to type character: 315ms
Fastest time to type character: 84ms

So as you can see there's a slight improvement but its negligible. I expect that once we implement useSelect directly in components (as opposed to wrapping with withSelect) there could be some more impact on performance (especially in cases where dependencies are correctly used).

youknowriad

Awesome work on this PR @nerrad I think we can ship it and iterate on it as we expand usage. useDispatch? :)

nerrad · 2019-05-27T13:40:45Z

useDispatch? :)

Yup I plan on working on that over the course of this week 👍

aduth

Nice work on this!

Do you know if this would close #13879 ?

aduth · 2019-05-21T12:53:16Z

packages/data/src/index.js

@@ -9,11 +9,14 @@ import combineReducers from 'turbo-combine-reducers';
 import defaultRegistry from './default-registry';
 import * as plugins from './plugins';

-export { default as withSelect } from './components/with-select';
+export { withSelect } from './components/with-select';


Was there a reason this was changed from a default to a named export?

I think you're commenting on an early iteration of this file (I was temporarily leaving the old withSelect in place for a reference point).

I think you're commenting on an early iteration of this file (I was temporarily leaving the old withSelect in place for a reference point).

I must have had that stored from an earlier, unsubmitted review 🤔 I didn't write it today.

aduth · 2019-05-28T19:28:12Z

packages/data/src/components/use-select/index.js

+import useAsyncMode from '../async-mode-provider/use-async-mode';
+
+/**
+ * Favor useLayoutEffect to ensure the store subscription callback always has


I really admire the emphasis in documentation in the changes of this pull request. 👍

aduth · 2019-05-28T19:29:33Z

packages/data/src/components/use-select/index.js

+ * In general, this custom React hook follows the
+ * [rules of hooks](https://reactjs.org/docs/hooks-rules.html).
+ *
+ * @param {Function} _mapSelect  Function called on every state change. The


Minor: Considering that this is enshrined in the public-facing documentation, I think we could have optimized for this to be the mapSelect, either choosing _mapSelect or (preferably, if one exists) a better name for the internal reference.

I guess it depends on your preference for or against "modifying" the argument, but since the arguments aren't const, you could always re-define:

export default function useSelect( mapSelect, deps ) { mapSelect = useCallback( mapSelect, deps );

aduth · 2019-05-28T19:31:41Z

packages/data/src/components/use-select/index.js

+	const registry = useRegistry();
+	const isAsync = useAsyncMode();
+	const queueContext = useMemo( () => ( { queue: true } ), [ registry ] );
+	const [ , forceRender ] = useReducer( ( s ) => s + 1, 0 );


Who will be the first to breach 9007199254740991 (Number.MAX_SAFE_INTEGEER) useSelect renders in a page session? 😆

lol if that happens, there will be other problems likely ;)

nerrad · 2019-05-28T20:19:52Z

Do you know if this would close #13879 ?

Ya it's possible it will because the subscription no longer gets set on construct (and is only set on effect - and unsubscribes on either a registry change or unmount). Worth testing to confirm but very likely will fix.

aduth · 2019-12-17T22:08:38Z

packages/data/src/components/use-select/index.js

+		} );
+
+		return () => {
+			isMounted.current = false;


I don't know that it ends up being an issue, but this callback won't only be called for unmounting, it will also be called on a change in registry (and, if #19205 lands, in change in isAsync). I guess it would depend on the order of how onStoreChange or a subsequent call to the previous useIsomorphicLayoutEffect which sets isMounted.current back to true. My worry could be that onStoreChange wouldn't run the selector if it considers the component unmounted. I guess for the purpose of the selector running as unique to the registry (or isAsync in #19205), we don't want the invalidated change callback, and we can feel confident that either renderQueue.add or onStoreChange would be called for the next registry? In which case, it's more an issue of a confusing name.

I guess for the purpose of the selector running as unique to the registry (or isAsync in #19205), we don't want the invalidated change callback, and we can feel confident that either renderQueue.add or onStoreChange would be called for the next registry?

Exactly

In which case, it's more an issue of a confusing name.

I am not sure what else to call it. isCleaningUp, isUnsubscribing?

In which case, it's more an issue of a confusing name.

I am not sure what else to call it. isCleaningUp, isUnsubscribing?

Yeah, either of those would work pretty well I think (flipping the boolean value).

That boolean covers both scenarios^^

zanona · 2020-03-10T14:57:33Z

Hey folks, is there a recipe to use useSelect on class components or isn't this really possible, and we should still use a withSelect wrapper for those? Thanks.

aduth · 2020-03-10T15:06:49Z

@zanona Hooks cannot be used in class components. This is a limitation imposed by React, and not something we have control over. In my experience, most any component can be ported to or expressed exclusively as a function component with hooks, so it's worth considering to migrate or write new components this way. Otherwise, withSelect will remain available as an option to use with class components.

nerrad · 2020-03-10T15:09:09Z

A workaround for using react hooks with class components if they are super complex and hard to convert immediately to a function component is to implement the hook in a higher order component that wraps your class component.

However, in this specific case, that's already handled for you because withSelect implements useSelect internally.

nerrad requested review from aduth and youknowriad as code owners May 20, 2019 12:44

nerrad commented May 20, 2019

View reviewed changes

youknowriad reviewed May 20, 2019

View reviewed changes

epiqueras reviewed May 20, 2019

View reviewed changes

packages/data/src/components/with-select/with-select-new.js Outdated

@@ -0,0 +1,25 @@

/**

Copy link

Contributor

epiqueras May 20, 2019

Choose a reason for hiding this comment

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

Nice!

youknowriad reviewed May 20, 2019

View reviewed changes

nerrad commented May 20, 2019

View reviewed changes

youknowriad reviewed May 21, 2019

View reviewed changes

nerrad mentioned this pull request May 21, 2019

@wordpress/data: Export entire registry context #15445

Closed

6 tasks

This was referenced May 22, 2019

@wordpress/data: Create useSelect hook. #15512

Closed

Refactor: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) #15444

Merged

nerrad force-pushed the FET/useSelect-take2 branch from 585795c to c8c2ca8 Compare May 22, 2019 01:19

nerrad requested a review from talldan as a code owner May 25, 2019 18:10

nerrad added 2 commits May 25, 2019 15:10

code cleanup and add documentation

c65f45f

update changelog

cf2e824

nerrad changed the title ~~@wordpress/data: useSelect experiment 2 (WIP)~~ @wordpress/data: useSelect experiment 2 May 25, 2019

nerrad changed the title ~~@wordpress/data: useSelect experiment 2~~ @wordpress/data: Introduce useSelect custom hook. May 25, 2019

nerrad requested a review from youknowriad May 25, 2019 19:26

nerrad self-assigned this May 25, 2019

nerrad added [Package] Data /packages/data [Type] Enhancement A suggestion for improvement. Framework Issues related to broader framework topics, especially as it relates to javascript labels May 25, 2019

youknowriad approved these changes May 27, 2019

View reviewed changes

nerrad merged commit ce78cd8 into master May 27, 2019

nerrad deleted the FET/useSelect-take2 branch May 27, 2019 13:58

aduth mentioned this pull request May 28, 2019

Data: Documentation: Document undocumented declarations #15176

Merged

aduth reviewed May 28, 2019

View reviewed changes

nerrad mentioned this pull request May 29, 2019

Serializer: Fix Invariant error related to hooks #15873

Closed

aduth mentioned this pull request Jun 4, 2019

Data: Implement useSelect hooks corollary to withSelect #15473

Closed

youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

aduth mentioned this pull request Jun 10, 2019

Data: withSelect: Subscription callbacks linger after renderToString #13879

Closed

aduth mentioned this pull request Dec 9, 2019

Data: Provide dependencies from withSelect to useSelect #19007

Closed

aduth reviewed Dec 17, 2019

View reviewed changes

nerrad mentioned this pull request Sep 17, 2021

Exceptions thrown in mapSelect function of useSelect hook should be reported in Sentry wordpress-mobile/gutenberg-mobile#3979

Open


		_Returns_

		- `Component`: Enhanced component with merged state data props.

@wordpress/data: Introduce useSelect custom hook. #15737

@wordpress/data: Introduce useSelect custom hook. #15737

Conversation

nerrad commented May 20, 2019 • edited Loading

Description

The signature is useSelect( mapSelect: function, deps: array )

Other

How has this been tested?

Types of changes

Next Steps:

Checklist:

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

Choose a reason for hiding this comment

nerrad May 20, 2019 • edited Loading

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

youknowriad May 20, 2019 • edited Loading

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

youknowriad May 20, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented May 20, 2019

nerrad left a 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

nerrad May 21, 2019 • edited Loading

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

nerrad commented May 20, 2019

nerrad commented May 20, 2019

Choose a reason for hiding this comment

nerrad May 21, 2019 • edited Loading

Choose a reason for hiding this comment

useSelect requires a dependency array and uses that to optimize rendering.

Developers do their own memoization of the mapSelect callback.

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

nerrad commented May 21, 2019

youknowriad commented May 21, 2019

epiqueras commented May 21, 2019

youknowriad commented May 21, 2019

youknowriad commented May 21, 2019

epiqueras commented May 21, 2019 • edited Loading

nerrad commented May 20, 2019 •

edited

Loading

The signature is `useSelect( mapSelect: function, deps: array )`

nerrad May 20, 2019 •

edited

Loading

youknowriad May 20, 2019 •

edited

Loading

youknowriad May 20, 2019 •

edited

Loading

nerrad May 21, 2019 •

edited

Loading

nerrad May 21, 2019 •

edited

Loading

`useSelect` requires a dependency array and uses that to optimize rendering.

Developers do their own memoization of the `mapSelect` callback.

epiqueras commented May 21, 2019 •

edited

Loading

nerrad commented May 22, 2019 •

edited

Loading

nerrad commented May 25, 2019 •

edited

Loading

A `withSelect` test asserted dispatched actions on mount (both in the component constructor and the `componentDidMount` lifecycle method).

nerrad May 28, 2019 •

edited

Loading