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: Refactor media fetching to use the core data module #5707

Merged
merged 5 commits into from Mar 22, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Mar 20, 2018

Follow up to #5219

The idea is to leverage the resolver API to replace the usage of the withAPIData HoC by the core data module. In this PR, I'm adding support to media fetching to the core-data module.

Testing instructions

  • Use the Image/Gallery blocks
  • Set, remove, update the featured image
  • It should work like master.

@youknowriad youknowriad self-assigned this Mar 20, 2018

@youknowriad youknowriad requested a review from aduth Mar 20, 2018

} );
} );
it( 'yields with requested media', async () => {

This comment has been minimized.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I really like these tests. Nice work setting up the pattern @aduth

It also shows that we could push it further by avoiding the mocks entirely (using call effect) but granted that it's not that important at the moment.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I really like these tests. Nice work setting up the pattern @aduth

It also shows that we could push it further by avoiding the mocks entirely (using call effect) but granted that it's not that important at the moment.

* @param {Object} state State tree
* @param {number} id Media id
*/
export async function* getMedia( state, id ) {

This comment has been minimized.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I was wondering if passing the state to the resolvers is a good idea? Maybe it's useless, since we can still call select from inside the resolver if needed. Kept it as is for now, but let's see how this goes.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I was wondering if passing the state to the resolvers is a good idea? Maybe it's useless, since we can still call select from inside the resolver if needed. Kept it as is for now, but let's see how this goes.

This comment has been minimized.

@aduth

aduth Mar 20, 2018

Member

Indeed, I'm not too sure how much we want to promote the use of select, particularly in a context where we know we could call selectors via direct import and pass the state. Another reason was merely to align the signatures between selectors and resolvers (since their names already match).

See also: #5219 (comment)

@aduth

aduth Mar 20, 2018

Member

Indeed, I'm not too sure how much we want to promote the use of select, particularly in a context where we know we could call selectors via direct import and pass the state. Another reason was merely to align the signatures between selectors and resolvers (since their names already match).

See also: #5219 (comment)

*/
export async function* getMedia( state, id ) {
const media = await apiRequest( { path: `/wp/v2/media/${ id }` } );
yield receiveMedia( media );

This comment has been minimized.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I'm not tracking any "is requesting" state at the moment because it's not really needed for our current use-cases. But I was wondering if we should have a separate requests state tree to keep track in a generic way of these states regardless of the request data type, query, ...

Same here, I guess we'll see with use-cases while adding other resolvers.

@youknowriad

youknowriad Mar 20, 2018

Contributor

I'm not tracking any "is requesting" state at the moment because it's not really needed for our current use-cases. But I was wondering if we should have a separate requests state tree to keep track in a generic way of these states regardless of the request data type, query, ...

Same here, I guess we'll see with use-cases while adding other resolvers.

This comment has been minimized.

@aduth

aduth Mar 20, 2018

Member

But I was wondering if we should have a separate requests state tree to keep track in a generic way of these states regardless of the request data type, query, ...

The original implementation in ffb0760 had a standalone requested state, which was meant to be generic and may still be an option to explore. The idea of not having a dedicated state works particularly well when we adopt a pattern of a null value meaning that it's known to exist, but not yet having received the entities. Though this works only with collections. With singular entities, a null value may instead mean having received an empty entity (404?). I agree we'll have to see how it plays out.

Related: #5219 (comment)

@aduth

aduth Mar 20, 2018

Member

But I was wondering if we should have a separate requests state tree to keep track in a generic way of these states regardless of the request data type, query, ...

The original implementation in ffb0760 had a standalone requested state, which was meant to be generic and may still be an option to explore. The idea of not having a dedicated state works particularly well when we adopt a pattern of a null value meaning that it's known to exist, but not yet having received the entities. Though this works only with collections. With singular entities, a null value may instead mean having received an empty entity (404?). I agree we'll have to see how it plays out.

Related: #5219 (comment)

Show outdated Hide outdated blocks/library/gallery/gallery-image.js
Show outdated Hide outdated blocks/library/gallery/gallery-image.js
@@ -136,7 +136,7 @@ class ImageBlock extends Component {
}
getAvailableSizes() {
return get( this.props.image, [ 'data', 'media_details', 'sizes' ], {} );
return get( this.props.image, [ 'media_details', 'sizes' ], {} );

This comment has been minimized.

@aduth

aduth Mar 20, 2018

Member

Observing that we're still pretty bound to the REST API structure of an entity with the new core-data module. This has its upsides (easy state insertion, consistency), but also doesn't help decouple from the REST API.

@aduth

aduth Mar 20, 2018

Member

Observing that we're still pretty bound to the REST API structure of an entity with the new core-data module. This has its upsides (easy state insertion, consistency), but also doesn't help decouple from the REST API.

This comment has been minimized.

@youknowriad

youknowriad Mar 20, 2018

Contributor

Agreed, I was wondering if we should introduce "models" somehow

@youknowriad

youknowriad Mar 20, 2018

Contributor

Agreed, I was wondering if we should introduce "models" somehow

Show outdated Hide outdated blocks/library/image/block.js
</ResponsiveWrapper>
}
{ media && media.isLoading && <Spinner /> }
{ ! media && <Spinner /> }

This comment has been minimized.

@aduth

aduth Mar 20, 2018

Member

What if a media request returns 404 ? This is a concern with treating "empty" as equivalent to "request in progress".

@aduth

aduth Mar 20, 2018

Member

What if a media request returns 404 ? This is a concern with treating "empty" as equivalent to "request in progress".

This comment has been minimized.

@youknowriad

youknowriad Mar 21, 2018

Contributor

I think I'm fine with this right now, we can revisit once/if we introduce request state

@youknowriad

youknowriad Mar 21, 2018

Contributor

I think I'm fine with this right now, we can revisit once/if we introduce request state

Show outdated Hide outdated core-data/test/selectors.js
Show outdated Hide outdated core-data/reducer.js

Fixed, thanks @aduth

@youknowriad youknowriad merged commit 89205a9 into master Mar 22, 2018

2 checks passed

codecov/project 44.4% (+0.18%) compared to bf81a76
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/media-fetching branch Mar 22, 2018

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