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: Support controls in resolvers #9507

Merged
merged 15 commits into from Sep 17, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Aug 31, 2018

This PR tweaks the data module to allow extended usage of the controls API in resolvers. To achieve that it does:

  • Refactors the way resolvers are registered and fulfilled in the data module
  • It exposes a fulfill function in the registry essentially to allow plugins to override the fulfillment implementation.
  • Refactors the redux-routine middleware to allow the possibility to know when the execution of the routine ends
  • Extend the fulfill function in the controls plugin to support resolvers in the controls plugin.

Testing instructions

  • Check that the current resolvers work properly (we don't use the controls plugin yet in Gutenberg)

@aduth aduth self-requested a review Aug 31, 2018

Show resolved Hide resolved packages/redux-routine/README.md
Show outdated Hide outdated packages/redux-routine/README.md Outdated

@youknowriad youknowriad changed the title from WIP: Support controls in resolvers to Data Module: Support controls in resolvers Sep 3, 2018

@aduth

fulfill is fine, rungen is fine, promises returned from dispatch is fine.

I'm not really digging our mixed consideration of resolvers. Is a resolver one of the proper concepts of the registry? If so, we're losing optimizations by wrapping selectors in registerResolvers. If not, we need to stop referencing resolvers in the plugins, in registerSelectors, and in the registry fulfill.

* Create a co-routine runtime.
*
* @param {Object} controls Object of control handlers.
* @param {function} dispatch Unhandled action dispatch.

This comment has been minimized.

@aduth

aduth Sep 4, 2018

Member

Trying to consider whether this should or shouldn't be Function vs. function. I think our basis is partially on typeof, so lowercase is likely correct.

Maybe add to

gutenberg/eslint/config.js

Lines 139 to 152 in ef25165

preferType: {
array: 'Array',
bool: 'boolean',
Boolean: 'boolean',
float: 'number',
Float: 'number',
int: 'number',
integer: 'number',
Integer: 'number',
Number: 'number',
object: 'Object',
String: 'string',
Void: 'void',
},

@aduth

aduth Sep 4, 2018

Member

Trying to consider whether this should or shouldn't be Function vs. function. I think our basis is partially on typeof, so lowercase is likely correct.

Maybe add to

gutenberg/eslint/config.js

Lines 139 to 152 in ef25165

preferType: {
array: 'Array',
bool: 'boolean',
Boolean: 'boolean',
float: 'number',
Float: 'number',
int: 'number',
integer: 'number',
Integer: 'number',
Number: 'number',
object: 'Object',
String: 'string',
Void: 'void',
},

This comment has been minimized.

@aduth

aduth Sep 12, 2018

Member

We should settle what this should be at some point. My previous comment is nonsense because typeof {} === 'object' (lowercase), yet we still use Object (uppercase) to describe the type.

Either to be discussed in Core JS or at least search through prior discussion to find what basis we had settled on here.

I'm thinking "Function" is correct though.

@aduth

aduth Sep 12, 2018

Member

We should settle what this should be at some point. My previous comment is nonsense because typeof {} === 'object' (lowercase), yet we still use Object (uppercase) to describe the type.

Either to be discussed in Core JS or at least search through prior discussion to find what basis we had settled on here.

I'm thinking "Function" is correct though.

Show outdated Hide outdated packages/redux-routine/src/index.js Outdated
Show outdated Hide outdated packages/data/src/plugins/controls/index.js Outdated
Show outdated Hide outdated packages/data/src/registry.js Outdated
Show outdated Hide outdated packages/data/src/registry.js Outdated
Show outdated Hide outdated packages/data/src/middlewares/async-generator.js Outdated
Show outdated Hide outdated packages/data/src/middlewares/async-generator.js Outdated
Show outdated Hide outdated packages/data/src/plugins/async-generator/index.js Outdated
Show outdated Hide outdated packages/data/src/plugins/async-generator/index.js Outdated
@@ -347,6 +273,7 @@ export function createRegistry( storeConfigs = {} ) {
}
let registry = {
namespaces,

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

I recall in early iterations of plugins implementation I needed to consider in withPlugins whether the result of mapValues should be left alone if not a function. Here, namespaces is being added and is unique from the others in that it is an object, not a function. It cannot therefore be apply'd.

@aduth

aduth Sep 6, 2018

Member

I recall in early iterations of plugins implementation I needed to consider in withPlugins whether the result of mapValues should be left alone if not a function. Here, namespaces is being added and is unique from the others in that it is an object, not a function. It cannot therefore be apply'd.

@@ -0,0 +1,3 @@
import { use, plugins } from '../../../../packages/data/src';
use( plugins.asyncGenerator );

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

What thing is depending on async generators in the test? Should they be, or should those tests be part of the suite for the asyncGenerator plugin instead?

@aduth

aduth Sep 6, 2018

Member

What thing is depending on async generators in the test? Should they be, or should those tests be part of the suite for the asyncGenerator plugin instead?

This comment has been minimized.

@youknowriad

youknowriad Sep 7, 2018

Contributor

This is needed for some effects tests. We have some effects triggering async generator resolvers that won't work without this.

@youknowriad

youknowriad Sep 7, 2018

Contributor

This is needed for some effects tests. We have some effects triggering async generator resolvers that won't work without this.

This comment has been minimized.

@youknowriad

youknowriad Sep 7, 2018

Contributor

This is something that won't be needed in the future if we rewrite those effects as generators with controls as we can directly feed the generator with mocked results in the tests without running the controls.

@youknowriad

youknowriad Sep 7, 2018

Contributor

This is something that won't be needed in the future if we rewrite those effects as generators with controls as we can directly feed the generator with mocked results in the tests without running the controls.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 7, 2018

Contributor

Ok, this PR now is in a much better shape. I think it's getting ready to 🚢

I wanted to deprecated the async-generator middleware/plugin but thought I would first wait for us to refactor the current async generators we have in Gutenberg to use controls and do that right after.

Contributor

youknowriad commented Sep 7, 2018

Ok, this PR now is in a much better shape. I think it's getting ready to 🚢

I wanted to deprecated the async-generator middleware/plugin but thought I would first wait for us to refactor the current async generators we have in Gutenberg to use controls and do that right after.

Show outdated Hide outdated packages/data/src/registry.js Outdated
Show outdated Hide outdated packages/data/src/plugins/controls/index.js Outdated
Show outdated Hide outdated packages/data/src/plugins/async-generator/index.js Outdated
Show outdated Hide outdated packages/data/src/registry.js Outdated

I addressed the feedback, let me know if you think the middleware should be moved elsewhere.

@aduth

aduth approved these changes Sep 12, 2018

Consider __experimental comment I left at #9507 (comment)

Otherwise LGTM 👍 Thanks for putting up with me 😄

Show outdated Hide outdated packages/data/src/plugins/async-generator/index.js Outdated
Show outdated Hide outdated packages/data/src/registry.js Outdated
Show outdated Hide outdated packages/redux-routine/CHANGELOG.md Outdated
* Create a co-routine runtime.
*
* @param {Object} controls Object of control handlers.
* @param {function} dispatch Unhandled action dispatch.

This comment has been minimized.

@aduth

aduth Sep 12, 2018

Member

We should settle what this should be at some point. My previous comment is nonsense because typeof {} === 'object' (lowercase), yet we still use Object (uppercase) to describe the type.

Either to be discussed in Core JS or at least search through prior discussion to find what basis we had settled on here.

I'm thinking "Function" is correct though.

@aduth

aduth Sep 12, 2018

Member

We should settle what this should be at some point. My previous comment is nonsense because typeof {} === 'object' (lowercase), yet we still use Object (uppercase) to describe the type.

Either to be discussed in Core JS or at least search through prior discussion to find what basis we had settled on here.

I'm thinking "Function" is correct though.

@youknowriad youknowriad added this to the 4.0 milestone Sep 14, 2018

@youknowriad youknowriad merged commit cbee072 into master Sep 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/redux-routine branch Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment