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: update the resolvers to rely on controls instead of async generators #9958

Merged
merged 4 commits into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Sep 17, 2018

Follow up to #9507

This PR refactors the current Gutenberg resolvers to use controls instead of async generator.

Testing instructions

  • Unit and e2e tests pass.

Todo

  • Deprecate the async generators plugin

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

@youknowriad youknowriad self-assigned this Sep 17, 2018

@youknowriad youknowriad requested review from aduth and WordPress/gutenberg-core Sep 17, 2018

},
SELECT( { selectorName, args } ) {
return selectData( 'core' )[ selectorName ]( ...args );

This comment has been minimized.

@youknowriad

youknowriad Sep 17, 2018

Contributor

Ideally, this would rely on a given registry object as an argument in the control instead of calling the global singleton select which is not great especially because it circumvents match any applied RegistryProvider.

I'm planning to do this change in a separate PR

cc @aduth

@youknowriad

youknowriad Sep 17, 2018

Contributor

Ideally, this would rely on a given registry object as an argument in the control instead of calling the global singleton select which is not great especially because it circumvents match any applied RegistryProvider.

I'm planning to do this change in a separate PR

cc @aduth

@youknowriad youknowriad changed the title from Data Module: update the resolvers to rely on resolvers instead of async generators to Data Module: update the resolvers to rely on controls instead of async generators Sep 17, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 18, 2018

Contributor

This should be ready for a review

Contributor

youknowriad commented Sep 18, 2018

This should be ready for a review

@tofumatt

This seems to be part of a larger set of changes I've only been partially paying attention to, and while I find generator code less friendly to newbies over the async/await syntax I know it can buy us things too…

The code here seems as easier to follow as before, so looks good to me. I might be missing something though, if you wanted to wait for @aduth to have a look I wouldn't be offended 😅

Show outdated Hide outdated packages/core-data/src/entities.js Outdated
return {
type: 'SELECT',
selectorName,
args,

This comment has been minimized.

@tofumatt

tofumatt Sep 18, 2018

Member

It seems weird that this isn't a spread here but is above… does that mean this is doing: args: …args? Sometimes I get hazy with ES6 shorthands.

@tofumatt

tofumatt Sep 18, 2018

Member

It seems weird that this isn't a spread here but is above… does that mean this is doing: args: …args? Sometimes I get hazy with ES6 shorthands.

This comment has been minimized.

@youknowriad

youknowriad Sep 21, 2018

Contributor

In this particular case, it's better to keep it as a separate args property, to avoid ambiguity with the others keys (type, selectorName)

@youknowriad

youknowriad Sep 21, 2018

Contributor

In this particular case, it's better to keep it as a separate args property, to avoid ambiguity with the others keys (type, selectorName)

This comment has been minimized.

@tofumatt

tofumatt Sep 21, 2018

Member

It might be nice to add a comment saying as much, but that makes sense to me.

@tofumatt

tofumatt Sep 21, 2018

Member

It might be nice to add a comment saying as much, but that makes sense to me.

@youknowriad youknowriad merged commit a8bc550 into master Sep 21, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +35.19% compared to c28d2db
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/resolvers-use-controls branch Sep 21, 2018

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