-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data Module: Adding support for sync generators to allow async action creators #7546
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6ef5716
Data: Move registry into own file
aduth 7cf782e
Data Module: Add default registry and createRegistry function
youknowriad 6294cb7
Adding support for custom registries in withSelect/withDispatch
youknowriad b9cd86e
Simplify registry initialization
youknowriad cc95059
update the tests to use react-test-renderer
youknowriad e4d1927
Reorganize the components of the data module
youknowriad 27902d7
Data Module: omit extra props introduced by HigherOrderComponents in …
youknowriad 6eb675c
Data Module: Add sync generators support for actions and resolvers
youknowriad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createContext } from '@wordpress/element'; | ||
|
||
const { Consumer, Provider } = createContext( null ); | ||
|
||
export const RegistryConsumer = Consumer; | ||
|
||
export default Provider; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { mapValues } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
Component, | ||
compose, | ||
createElement, | ||
createHigherOrderComponent, | ||
pure, | ||
} from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import defaultRegistry from '../../default-registry'; | ||
import { RegistryConsumer } from '../registry-provider'; | ||
|
||
/** | ||
* Higher-order component used to add dispatch props using registered action | ||
* creators. | ||
* | ||
* @param {Object} mapDispatchToProps Object of prop names where value is a | ||
* dispatch-bound action creator, or a | ||
* function to be called with with the | ||
* component's props and returning an | ||
* action creator. | ||
* | ||
* @return {Component} Enhanced component with merged dispatcher props. | ||
*/ | ||
const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent( | ||
compose( [ | ||
pure, | ||
( WrappedComponent ) => { | ||
class ComponentWithDispatch extends Component { | ||
constructor( props ) { | ||
super( ...arguments ); | ||
|
||
this.proxyProps = {}; | ||
this.setProxyProps( props ); | ||
} | ||
|
||
componentDidUpdate() { | ||
this.setProxyProps( this.props ); | ||
} | ||
|
||
proxyDispatch( propName, ...args ) { | ||
// Original dispatcher is a pre-bound (dispatching) action creator. | ||
const dispatch = this.props.registry ? this.props.registry.dispatch : defaultRegistry.dispatch; | ||
mapDispatchToProps( dispatch, this.props.ownProps )[ propName ]( ...args ); | ||
} | ||
|
||
setProxyProps( props ) { | ||
// Assign as instance property so that in reconciling subsequent | ||
// renders, the assigned prop values are referentially equal. | ||
const dispatch = props.registry ? props.registry.dispatch : defaultRegistry.dispatch; | ||
const propsToDispatchers = mapDispatchToProps( dispatch, props.ownProps ); | ||
this.proxyProps = mapValues( propsToDispatchers, ( dispatcher, propName ) => { | ||
// Prebind with prop name so we have reference to the original | ||
// dispatcher to invoke. Track between re-renders to avoid | ||
// creating new function references every render. | ||
if ( this.proxyProps.hasOwnProperty( propName ) ) { | ||
return this.proxyProps[ propName ]; | ||
} | ||
|
||
return this.proxyDispatch.bind( this, propName ); | ||
} ); | ||
} | ||
|
||
render() { | ||
return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />; | ||
} | ||
} | ||
|
||
return ( ownProps ) => ( | ||
<RegistryConsumer> | ||
{ ( registry ) => ( | ||
<ComponentWithDispatch | ||
ownProps={ ownProps } | ||
registry={ registry } | ||
/> | ||
) } | ||
</RegistryConsumer> | ||
); | ||
}, | ||
] ), | ||
'withDispatch' | ||
); | ||
|
||
export default withDispatch; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import TestRenderer from 'react-test-renderer'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createElement } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import withDispatch from '../'; | ||
import { createRegistry } from '../../../registry'; | ||
import RegistryProvider from '../../registry-provider'; | ||
|
||
describe( 'withDispatch', () => { | ||
let registry; | ||
beforeEach( () => { | ||
registry = createRegistry(); | ||
} ); | ||
|
||
it( 'passes the relevant data to the component', () => { | ||
const store = registry.registerStore( 'counter', { | ||
reducer: ( state = 0, action ) => { | ||
if ( action.type === 'increment' ) { | ||
return state + action.count; | ||
} | ||
return state; | ||
}, | ||
actions: { | ||
increment: ( count = 1 ) => ( { type: 'increment', count } ), | ||
}, | ||
} ); | ||
|
||
const Component = withDispatch( ( _dispatch, ownProps ) => { | ||
const { count } = ownProps; | ||
|
||
return { | ||
increment: () => _dispatch( 'counter' ).increment( count ), | ||
}; | ||
} )( ( props ) => <button onClick={ props.increment } /> ); | ||
|
||
const testRenderer = TestRenderer.create( | ||
<RegistryProvider value={ registry }> | ||
<Component count={ 0 } /> | ||
</RegistryProvider> | ||
); | ||
const testInstance = testRenderer.root; | ||
|
||
const incrementBeforeSetProps = testInstance.findByType( 'button' ).props.onClick; | ||
|
||
// Verify that dispatch respects props at the time of being invoked by | ||
// changing props after the initial mount. | ||
testRenderer.update( | ||
<RegistryProvider value={ registry }> | ||
<Component count={ 2 } /> | ||
</RegistryProvider> | ||
); | ||
|
||
// Function value reference should not have changed in props update. | ||
expect( testInstance.findByType( 'button' ).props.onClick ).toBe( incrementBeforeSetProps ); | ||
|
||
incrementBeforeSetProps(); | ||
|
||
expect( store.getState() ).toBe( 2 ); | ||
} ); | ||
} ); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
So I'm a bit confused about our intended usage here. In this case, the promise returned by
apiRequest
does not return an action object, yet we'reyield
ing it the same as we would for any other action. I suppose as implemented, it's up to the runtime to determine whether it "looks" like an action to decide whether it should be dispatched? Not always clear at a glance whether the thing being yielded is an action or not.I guess in my mind, I had imagined something more like:
i.e. always yielding a proper action to which we can attach specific behaviors (through some API to add controls to rungen dynamically). I think this could also help rein in control to prevent abuse of promises by limiting the control flows exposed.
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 think that's the goal, I won't consider these "actions" but "effects". I didn't want to introduce new effects on the initial implementation.
Also, I tend to think we shouldn't couple the data module to API requests. I'd prefer something more generic:
yield { type: 'CALL', function: apiRequest, args: { path: '/wp/v2/categories?per_page=-1' } }
I think it's possible to keep the data module REST API agnostic and still allow you to do what you propose, but this requires having the ability to provide custom controls to the rungen runtime when calling
registerStore
, not sure how I feel about it. I tend to prefer starting smallThere 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 know this isn't a democracy, but I agree 😆
Keeping the module API-agnostic means testing/mocking is easier and it's easy to allow people to use their own functions to intercept/modify certain calls if wanted. Win/win in my mind.
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.
We've already promoted resolvers as being the swappable layer of abstraction, so do we really have to go the next step further and also make its yielded actions generic?
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 suppose not, but I find there's a lot of flexibility in this approach.
If I wanted to test the generic approach, it would be easier, for instance.
I suppose I don't see the harm in it but I see benefits.
I'm not sure what the "abuse of promises" would be as mentioned earlier, but I'm generally in favour of us giving ourselves lots of flexibility. I know flexibility is another word for footgun, but I guess I don't see what the danger is 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.
This many times.
I've often pointed to the
effects.js
file as a prime example of flexibility gone wrong, as the file has been a failure in almost every regard: Lack of self-contained documented functions, non-obvious behaviors, callback hell, side effects which aren't side effects, implicit behaviors by reaching into state.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 what it's worth though, the proposed, saga-style pattern is more idiomatic and in my experience using something similar, never bit us or made us write confusing/dangerous code on the Add-ons project.
The effects are definitely a bit too out there. I don't think what's being proposed is as flexible or dangerous, and I think perhaps some intense code review around the first set of patches could lead to us creating strong idioms we could document and stick with.
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.
holy cannoli.
and oh how nasty this is without strong static types,
without the compiler helping make sense of it.
just had to toss that out there 😉
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 talking about this with @youknowriad today, I came to realize I misinterpreted the point here. I never meant to suggest that the data module should have awareness of how to make API interactions, but rather that it should provide the tools to allow an individual store to define its own continuation procedures for specific action types, so if the core-data store decided it needed a way to make API calls, it could define this through some API exposed on the data module (the generalized "controls" structure).
Rough idea:
Unsure if it needs to be middleware-esque, or just return the
Promise
:Then what we have here becomes:
It's a small difference, but intentionally restricts what's possible in the action creators alone to avoid the footgun effect of allowing unrestricted promises, encouraging instead a common set of well-defined continuation conditions. Further, it's more consistent in that the only thing that's yielded are action objects.