-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
@wordpress/data: Introduce new custom useDispatch
react hook
#15896
Conversation
@youknowriad, @epiqueras, and @aduth I'd love to get your feedback on this before I go further (note not in it's final state with inline docs etc). |
this( registry.dispatch, registry )[ propName ]( ...args ); | ||
} | ||
|
||
const useDispatchWithMap = ( dispatchMap ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is just something we'd use in withDispatch
and not expose right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have another idea I'm going to try here too for performance. I think I could return a mutated ref object (which might help reduce renders).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya the idea I tried didn't help so not going with it.
|
||
const useDispatch = ( storeName ) => { | ||
const registry = useRegistry(); | ||
return useMemo( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if useMemo
is even needed here? Probably not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's more expensive? Calling dispatch
or the hook's overhead?
Can registry
change without dispatch
changing? If so, you could make line 12:
const dispatch = useRegistry().dispatch;
And just close over that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think I'm going to drop the use of useMemo
. Even if registry
changes it'll always expose dispatch
. So I'll probably do something like:
const useDispatch = ( storeName ) => {
const { dispatch } = useRegistry();
return storeName === void 0 ? dispatch : dispatch( storeName );
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So calling dispatch
on every render is fine (cheaper than the hook's overhead)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, registry.dispatch
is just an object of stores mapped to action creators, so there should be little to no overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 81ee6ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you benchmark both approaches? This will be used in so many places that slight performance differences can significantly affect the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the hook isn't used anywhere, so to use the performance tools we have right now I'd have to implement it in a few components that are tested with the performance tools (or there may be some other benchmarking approach I'd need to take). If you have some ideas on how I could effectively benchmark both approaches I'm all ears.
Otherwise, I think once the hook starts getting implemented throughout the codebase we can take a look at doing some benchmarking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timing dispatch(...)
vs. useMemo(...)
would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff @nerrad 🚀
I added a few suggestions and a fix for the performance regression you found.
packages/data/src/components/use-dispatch/use-dispatch-with-map.js
Outdated
Show resolved
Hide resolved
`Property ${ propName } returned from dispatchMap in useDispatchWithMap must be a function.` | ||
); | ||
} | ||
return proxyDispatch.bind( dispatchMap, propName, registry ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also just declare an arrow function here to avoid the this
complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ended up returning the dispatcher
variable instead (see 7d88dd1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to call mapDispatch
again when a dispatcher
is called, so the selected state is fresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so something like this instead?
return ( ...args ) => dispatchMap( registry.dispatch, registry )[ propName ]( ...args )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I had to implement useRef
to grab the latest dispatchMap (see 5856f20) Some failing e2e tests revealed that the above example I tried was not sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #15896 (comment)
packages/data/src/components/use-dispatch/use-dispatch-with-map.js
Outdated
Show resolved
Hide resolved
|
||
const useDispatch = ( storeName ) => { | ||
const registry = useRegistry(); | ||
return useMemo( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's more expensive? Calling dispatch
or the hook's overhead?
Can registry
change without dispatch
changing? If so, you could make line 12:
const dispatch = useRegistry().dispatch;
And just close over that instead.
Performance metrics from the latest commit 7d88dd1: Average time to load: 6410ms I retested master because it seemed the performance tests were running slower on my machine and there was a bit of a difference from the original post: Average time to load: 6831ms So it actually looks like things are running a tad bit faster now :) The variations in metrics are not that significant so I think we can roll with this approach? Any agreement here? If so, I'll get this pull finished up with:
|
@@ -26,7 +32,9 @@ const useDispatchWithMap = ( dispatchMap ) => { | |||
`Property ${ propName } returned from dispatchMap in useDispatchWithMap must be a function.` | |||
); | |||
} | |||
return dispatcher; | |||
return ( ...args ) => { | |||
return currentDispatchMap.current( registry.dispatch, registry )[ propName ]( ...args ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there still is a flaw with this though. dispatchMap
might be the same function, but return a different collection of props between rerenders. In which case, this will only invoke the props functions returned on the intiial render (unless the function changes).
Eg. a map that looks something like this:
const mapDispatch = ( dispatch ) => {
if ( ! props.foo ) {
return {};
}
return {
onClick: dispatch.('someStore').addFoo( props.foo )
};
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatchMap
is a dependency there so it will re-run and return a new object.
What we can do is accept a deps
argument and memoize dispatchMap
at the top just like in useSelect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would give it all the properties we want:
- The returned object with action dispatchers only changes when
dispatchMap
changes. dispatchMap
only changes if it hasdeps
and any of them change.
So for your example you would call it like this: useDispatchWithMap( mapDispatch, [ props.foo ] )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also don't need any refs this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind:
- We're not exposing
useDispatchWithMap
for the general api, it's only intended forwithDispatch
internals. withDispatch
can't pass any dependencies for the same reason it wouldn't work in thewithSelect
implementation ofuseSelect
because props shape changes.- My example is admittedly not clear because it would actually be something like this fed to
withDispatch
(and passed through here viawithDispatch
).
const SomeWrappedComponent = withDispatch( ( dispatch, { foo } ) => {
if ( ! foo ) {
return {};
}
return {
onClick: dispatch.('someStore').addFoo( foo )
};
} )( SomeComponent );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, here's the code that was failing in e2e tests which the useRef
implementation fixed:
gutenberg/packages/edit-post/src/components/keyboard-shortcut-help-modal/index.js
Lines 101 to 115 in 13e5851
export default compose( [ | |
withSelect( ( select ) => ( { | |
isModalActive: select( 'core/edit-post' ).isModalActive( MODAL_NAME ), | |
} ) ), | |
withDispatch( ( dispatch, { isModalActive } ) => { | |
const { | |
openModal, | |
closeModal, | |
} = dispatch( 'core/edit-post' ); | |
return { | |
toggleModal: () => isModalActive ? closeModal() : openModal( MODAL_NAME ), | |
}; | |
} ), | |
] )( KeyboardShortcutHelpModal ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, this should happen:
- Prop changes make the wrapped component rerender.
mapDispatch
is redefined:const mapDispatch = ( dispatch, registry ) => mapDispatchToProps( - The hook's
useMemo
clears and returns a new object:}, [ dispatchMap, registry ] );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya that's what I expected too. But both the e2e test and manual testing were failing for that behaviour (when using the keyboard shortcut for that modal when it was open, isModalActive
was always false - as if it was closed). It might be that this is exposing an issue somewhere else? (btw there were a couple other components failing too, this is just the easiest to point to).
sidenote: thanks for being responsive in this pull... I'm off for the night though 👋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I found the reason why.
componentDidMount() { |
That component binds the callbacks on mount and never updates them.
I guess we'll have to keep the ref then. But I think we should reference the ref instead of the variable everywhere for consistency. We should also use useLayoutEffect
instead of useEffect
to avoid action dispatchers called between renders and effects having an outdated callback.
Yeah it's getting pretty late here too. Good night!
The latest |
.current( registry.dispatch, registry )[ propName ]( ...args ); | ||
} | ||
); | ||
}, [ dispatchMap, registry ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means this is executed each time dispatchMap
changes and from the implementation of withDispatch
, the function passed is an inline one which means this will be regenerated on each call. This I believe defeats the purpose here.
Also, I don't mind if withDispatch
is not refactored to use a hook right now but I'm opposed to it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's fine because it's wrapped in pure
so that will only happen when props change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like someone else to verify my findings with performance tests but from the ones I've run the impact is pretty much nil either way (though I have seen more small improvements in successive runs than regressions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of the proxy dispatches is to avoid running the mapping function each time the props change to avoid rerendering sub-components (have always the same reference to the functions returned by withDispatch
across rerenders). This was one of the main differences with connect
.
How is this addressed if the the action creators are regenerated each time props change.
I'd still like someone else to verify my findings with performance tests but from the ones I've run the impact is pretty much nil either way
I think ideally, you should disable the "async" mode and do the performance tests to compare exactly the impact. The async mode hides some of the impact by only focusing on the selected block which is going to be rerendered anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. If we assume that the keys of the returned object won't change. We can do what I described here: #15896 (comment) and pass in []
as the deps in withDispatch
. That would achieve what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we assume that the keys of the returned object won't change. We can do what I described here: #15896 (comment) and pass in [] as the deps in withDispatch. That would achieve what we want.
As I mentioned in my reply to your previous comment, the problem here is what would we use as the value for deps from withDispatch
? We can't use Object.values( props )
because of the same issue encountered in useSelect
because the props object shape is not consistent.
I think ideally, you should disable the "async" mode and do the performance tests to compare exactly the impact. The async mode hides some of the impact by only focusing on the selected block which is going to be rerendered anyway.
Do you have some pointers on how I can do that for the performance tests? If you don't have anything in mind I can look into that (I'm assuming just temporarily modify the AsyncMode provider? for the build the tests are run on). Although I'm not sure that will give accurate results either because then we won't know if the performance hit is from the selects in the tests or the dispatch right?
With the uncertainty around impact here, I'm inclined to just do useDispatch
in this pull and leave withDispatch
as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some pointers on how I can do that for the performance tests? If you don't have anything in mind I can look into that (I'm assuming just temporarily modify the AsyncMode provider?
Pass false to this prop https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list/index.js#L212
Although I'm not sure that will give accurate results either because then we won't know if the performance hit is from the selects in the tests or the dispatch right?
yes, but I was thinking that if you compare this branch with master, the only changes are in withDispatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in my reply to your previous comment, the problem here is what would we use as the value for deps from withDispatch? We can't use Object.values( props ) because of the same issue encountered in useSelect because the props object shape is not consistent.
An empty array, it never needs to re-run because the callbacks reference the mutable ref.
In this case deps
would be: depencies that make Object.keys(mapDispatch(...))
return a different list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but I was thinking that if you compare this branch with master, the only changes are in withDispatch
Ahh right.
An empty array, it never needs to re-run because the callbacks reference the mutable ref. In this case deps would be: depencies that make Object.keys(mapDispatch(...)) return a different list.
I apologize for not quite getting it yet. Do you mean withDispatch
would call useDispatchWithMap
like this:
const dispatchProps = useDispatchWithMap( dispatchMap, [] );
And then the signature for the hook would be something like this:
const useDispatchWithMap( dispatchMap, deps ) {
dispatchMap = useCallback( dispatchMap, deps );
/** mostly existing logic **/
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I should have been more clear.
Yeah exactly, but the ref should always be updated from the passed in parameter (not the memoized variable).
Now that I think about it more. Replacing mapDispatch
with ...deps
in the useMemo
deps
would be cleaner and have the same effect.
ensures that any state is fresh when prop functions are called.
This more closely mirrors the original behaviour of withDispatch.
70f2bea
to
6946a6a
Compare
New
|
Branch | avg load | avg dom load | avg type | slowest type | fastest type |
---|---|---|---|---|---|
useDispatch - with async | 7246ms | 6638ms | 101ms | 240ms | 69ms |
master - with async | 6790ms | 6208ms | 98.92ms | 227ms | 56ms |
useDispatch - no async | 9190ms | 7878ms | 280.841ms | 509ms | 223ms |
master - no async | 9035ms | 7590ms | 293ms | 631ms | 219ms |
Conclusions:
master is faster than this branch for the "with async" tests for the average but oddly with no async (which more closely tests the changes in withDispatch
), this branch is faster! On the whole this suggests that there are some marginal gains with the new withDispatch
and some minor negative impact on overall performance.
useDispatch
with useMemo
and without useMemo
performance tests.
The purpose of these tests (as suggested here was to see whether memoizing the returned action creators (via useMemo
) was slower or faster than the currently implemented logic. For the test, I created a mock component implementing useDispatch
with a mock registry and used react-test-renderer
to mount and unmount the component 1000 times. I tested invoking useDispatch
both with a store name and without the store name argument. The results are below:
type | Avg time to render with store name | Avg time to render (no store name ) |
---|---|---|
no useMemo |
0.048ms | 0.03ms |
with useMemo |
0.049ms | 0.032ms |
Conclusions
It appears that the difference between the two is very marginal but there is an overhead cost (however minor) to implementing useMemo
so it appears we're good to leave the current logic as is.
Overall, from a performance perspective, I think there's nothing indicating that we need to backtrack on some of these changes. I'll continue on with covering all the changes in unit tests.
render() { | ||
return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />; | ||
} | ||
( WrappedComponent ) => pure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this pure
call is not really necessary (and I suspect the small decline of performance that you noticed is coming from it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good point, I'll try without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 6500117. The new 3 run average for the new withDispatch
after pure
being removed is:
Average time to load: 6590ms
Average time to DOM content load: 6041ms
Average time to type character: 99ms
Slowest time to type character: 234ms
Fastest time to type character: 71ms
So ya, it does look like there was some unnecessary overhead added by using pure
here.
*/ | ||
const useDispatch = ( storeName ) => { | ||
const { dispatch } = useRegistry(); | ||
return storeName === void 0 ? dispatch : dispatch( storeName ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First time I see === void 0
. I'd have done !! storeName
personally. Curious to learn more here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya !! storeName
could have been used here. Using void 0
is just an old habit I sometimes slip into when I want to check against the undefined
primitive value. I don't know if it's still the case with modern browsers, but it was possible for the undefined
global to be overwritten so it was just a protection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's still the case with modern browsers, but it was possible for the
undefined
global to be overwritten so it was just a protection.
It's not been possible to override in any browser which conforms to ES5 and newer (IE9+).
https://www.ecma-international.org/ecma-262/5.1/#sec-15.1.1.3
A future maintainer most certainly will struggle to under what this code is doing. Not a big issue, but in general we should feel fine to compare against undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Babel might even take care of it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel really old all of a sudden 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Babel might even take care of it too.
I was curious about this, and it doesn't seem like Babel does, but Uglify will convert it, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, maybe for the older targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely because void 0
is 3 fewer characters than undefined
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely because
void 0
is 3 fewer characters than undefined
Ha! That's my new reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ready to land. Great work.
* the first argument and the `registry` object | ||
* as the second argument. Should return an | ||
* object mapping props to functions. | ||
* @param {Array} deps An array of dependencies for the hook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What purpose does deps
serve if this is an internal hook, and the only place we use it, we always pass an empty array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Right now with this being an internal only implementation it's obviously an unneeded variable. There may be potential for this hook being exposed at some point as a public api (if use-case demonstrates need) in which case there is utility for the deps arg.
* } )( Button ); | ||
* | ||
* // Rendered in the application: | ||
* // | ||
* // <SaleButton>Start Sale!</SaleButton> | ||
* ``` | ||
* _Note:_ It is important that the `mapDispatchToProps` function always returns an object with the same keys. For example, it should not contain conditions under which a different value would be returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not foresee it being possible in the future for optimizations which leverage the fact we'd established an expectation that the prop names don't change? The previous implementation benefitted by the fact we could compute the proxy functions once for the entire lifecycle of a component. I'm not sure if or how it would make sense to reflect that in the new implementation, but changing the documentation here may risk eliminating this as an option to explore down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I removed this because I thought it was documentation I had added in iterating on this pull! I agree we should keep this in place. I'll add back in a separate pull (might not be till later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #15987
Description
This pull contains the work exposing a new useDispatch hook on the
@wordpress/data
package. As per discussions in #15473 and in #core-js slack meeting, the primary api exposed for the new hook is:storeName
is optional and if provided then the action creators registered on that store are returned. Otherwise if nostoreName
is provided, then theregistry.dispatch
function is returned.Also in this pull I've experimented with a
useDispatchWithMap
hook to replacewithDispatch
internals with (and we could maybe not expose this initially, but just implement inwithDispatch
only). I was interested in seeing:a. is it possible.
b. does it simplify logic
c. what impact does it have.
I have a working example in this pull and did some performance benchmarking with the following results:
5.8.0 Tag
Average time to load: 7834ms
Average time to DOM content load: 7073ms
Average time to type character: 86.05ms
Slowest time to type character: 246ms
Fastest time to type character: 60ms
Current Master (at time of writing this post)
Average time to load: 6161ms
Average time to DOM content load: 5698ms
Average time to type character: 79.06ms
Slowest time to type character: 164ms
Fastest time to type character: 57ms
This branch
Average time to load: 7730ms
Average time to DOM content load: 7199ms
Average time to type character: 100.29ms
Slowest time to type character: 209ms
Fastest time to type character: 68ms
With those results, I doubt we'll want to merge this with the
useDispatchWithMap
changes and instead just keepwithDispatch
as is. Still, I'm sure there's better ways of doing that hook...Things to do:
How has this been tested?
Screenshots
Types of changes
Checklist: