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.

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.

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.

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.

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).

hmm...

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.

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

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

Nice!

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

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)

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?

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.