Skip to content
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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions blocks/library/gallery/gallery-image.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import classnames from 'classnames';
* WordPress Dependencies
*/
import { Component } from '@wordpress/element';
import { IconButton, withAPIData, Spinner } from '@wordpress/components';
import { IconButton, Spinner } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { keycodes } from '@wordpress/utils';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -75,10 +76,10 @@ class GalleryImage extends Component {
}

componentWillReceiveProps( { isSelected, image } ) {
if ( image && image.data && ! this.props.url ) {
if ( image && ! this.props.url ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will image be falsey?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the API loads the image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't see the object fallback

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this pull request, but why do we access this.props.url here? Should this not be from the nextProps argument?

this.props.setAttributes( {
url: image.data.source_url,
alt: image.data.alt_text,
url: image.source_url,
alt: image.alt_text,
} );
}

Expand Down Expand Up @@ -147,6 +148,6 @@ class GalleryImage extends Component {
}
}

export default withAPIData( ( { id } ) => ( {
image: id ? `/wp/v2/media/${ id }` : {},
export default withSelect( ( select, { id } ) => ( {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This works well for an in-place replacement, but in my own experience I want to start going out of my way to be more verbose with how we use withSelect, to set a good precedent especially when scaling a component to select many things from the same store (example), and to avoid both (a) in-place ownProps destructuring (destructuring obscures what is being destructured when used in arguments) and (b) the arrow function implicit return of objects (difficult to discern between implicit return and a function body).

This is how I would write:

export default withSelect( ( select, ownProps ) => {
	const { getMedia } = select( 'core' );
	const { id } = ownProps;

	return {
		image: id ? getMedia( id ) : null,
	};
} )( GalleryImage );

Or:

export default withSelect( ( select, ownProps ) => {
	const { getMedia } = select( 'core' );
	const { id } = ownProps;
	if ( ! id ) {
		return;
	}

	return {
		image: getMedia( id ),
	};
} )( GalleryImage );

image: id ? select( 'core' ).getMedia( id ) : {},
} ) )( GalleryImage );
8 changes: 4 additions & 4 deletions blocks/library/image/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import {
SelectControl,
TextControl,
Toolbar,
withAPIData,
withContext,
} from '@wordpress/components';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -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' ], {} );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

render() {
Expand Down Expand Up @@ -297,14 +297,14 @@ export default compose( [
withContext( 'editor' )( ( settings ) => {
return { settings };
} ),
withAPIData( ( props ) => {
withSelect( ( select, props ) => {
const { id } = props.attributes;
if ( ! id ) {
return {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: With #5421, we don't need to return an object.

}

return {
image: `/wp/v2/media/${ id }`,
image: select( 'core' ).getMedia( id ),
};
} ),
] )( ImageBlock );
19 changes: 19 additions & 0 deletions core-data/actions.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { castArray } from 'lodash';

/**
* Returns an action object used in signalling that the request for a given
* data type has been made.
Expand Down Expand Up @@ -31,3 +36,17 @@ export function receiveTerms( taxonomy, terms ) {
terms,
};
}

/**
* Returns an action object used in signalling that media have been received.
*
* @param {Array|Object} media Media received.
*
* @return {Object} Action object.
*/
export function receiveMedia( media ) {
return {
type: 'RECEIVE_MEDIA',
media: castArray( media ),
};
}
20 changes: 19 additions & 1 deletion core-data/reducer.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/**
* External dependencies
*/
import { combineReducers } from 'redux';
import { keyBy } from 'lodash';

/**
* WordPress dependencies
*/
import { combineReducers } from '@wordpress/data';

/**
* Reducer managing terms state. Keyed by taxonomy slug, the value is either
Expand Down Expand Up @@ -37,6 +42,19 @@ export function terms( state = {}, action ) {
return state;
}

export function media( state = {}, action ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocBlock 😄

switch ( action.type ) {
case 'RECEIVE_MEDIA':
return {
...state,
...keyBy( action.media, 'id' ),
};
}

return state;
}

export default combineReducers( {
terms,
media,
} );
13 changes: 12 additions & 1 deletion core-data/resolvers.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import apiRequest from '@wordpress/api-request';
/**
* Internal dependencies
*/
import { setRequested, receiveTerms } from './actions';
import { setRequested, receiveTerms, receiveMedia } from './actions';

/**
* Requests categories from the REST API, yielding action objects on request
Expand All @@ -17,3 +17,14 @@ export async function* getCategories() {
const categories = await apiRequest( { path: '/wp/v2/categories' } );
yield receiveTerms( 'categories', categories );
}

/**
* Requests a media element from the REST API.
*
* @param {Object} state State tree
* @param {number} id Media id
*/
export async function* getMedia( state, id ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const media = await apiRequest( { path: `/wp/v2/media/${ id }` } );
yield receiveMedia( media );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
12 changes: 12 additions & 0 deletions core-data/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,15 @@ export function isRequestingTerms( state, taxonomy ) {
export function isRequestingCategories( state ) {
return isRequestingTerms( state, 'categories' );
}

/**
* Returns the media object by id.
*
* @param {Object} state Data state.
* @param {number} id Media id.
*
* @return {Object?} Media object.
*/
export function getMedia( state, id ) {
return state.media[ id ];
}
23 changes: 22 additions & 1 deletion core-data/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import deepFreeze from 'deep-freeze';
/**
* Internal dependencies
*/
import { terms } from '../reducer';
import { terms, media } from '../reducer';

describe( 'terms()', () => {
it( 'returns an empty object by default', () => {
Expand Down Expand Up @@ -67,3 +67,24 @@ describe( 'terms()', () => {
expect( state ).toEqual( {} );
} );
} );

describe( 'media', () => {
it( 'returns an empty object by default', () => {
const state = media( undefined, {} );

expect( state ).toEqual( {} );
} );

it( 'returns with received media by id', () => {
const originalState = deepFreeze( {} );
const state = media( originalState, {
type: 'RECEIVE_MEDIA',
media: [ { id: 1, title: 'beach' }, { id: 2, title: 'sun' } ],
} );

expect( state ).toEqual( {
1: { id: 1, title: 'beach' },
2: { id: 2, title: 'sun' },
} );
} );
} );
22 changes: 20 additions & 2 deletions core-data/test/resolvers.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import apiRequest from '@wordpress/api-request';
/**
* Internal dependencies
*/
import { getCategories } from '../resolvers';
import { setRequested, receiveTerms } from '../actions';
import { getCategories, getMedia } from '../resolvers';
import { setRequested, receiveTerms, receiveMedia } from '../actions';

jest.mock( '@wordpress/api-request' );

Expand All @@ -30,3 +30,21 @@ describe( 'getCategories', () => {
expect( received ).toEqual( receiveTerms( 'categories', CATEGORIES ) );
} );
} );

describe( 'getMedia', () => {
const MEDIA = { id: 1 };

beforeAll( () => {
apiRequest.mockImplementation( ( options ) => {
if ( options.path === '/wp/v2/media/1' ) {
return Promise.resolve( MEDIA );
}
} );
} );

it( 'yields with requested media', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const fulfillment = getMedia( {}, 1 );
const received = ( await fulfillment.next() ).value;
expect( received ).toEqual( receiveMedia( MEDIA ) );
} );
} );
18 changes: 17 additions & 1 deletion core-data/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import deepFreeze from 'deep-freeze';
/**
* Internal dependencies
*/
import { getTerms, isRequestingTerms } from '../selectors';
import { getTerms, isRequestingTerms, getMedia } from '../selectors';

describe( 'getTerms()', () => {
it( 'returns value of terms by taxonomy', () => {
Expand Down Expand Up @@ -56,3 +56,19 @@ describe( 'isRequestingTerms()', () => {
expect( result ).toBe( true );
} );
} );

describe( 'getMedia', () => {
it( 'returns a media element by id', () => {
let state = deepFreeze( {
media: {},
} );
expect( getMedia( state, 1 ) ).toBe( undefined );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Seems like a separate test case to me (it should return undefined for unknown media)


state = deepFreeze( {
media: {
1: { id: 1 },
},
} );
expect( getMedia( state, 1 ) ).toEqual( { id: 1 } );
} );
} );
19 changes: 12 additions & 7 deletions editor/components/post-featured-image/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { __ } from '@wordpress/i18n';
import { Button, Spinner, ResponsiveWrapper, withAPIData } from '@wordpress/components';
import { MediaUpload } from '@wordpress/blocks';
import { compose } from '@wordpress/element';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -38,15 +39,15 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med
modalClass="editor-post-featured-image__media-modal"
render={ ( { open } ) => (
<Button className="button-link editor-post-featured-image__preview" onClick={ open } >
{ media && !! media.data &&
{ media &&
<ResponsiveWrapper
naturalWidth={ media.data.media_details.width }
naturalHeight={ media.data.media_details.height }
naturalWidth={ media.media_details.width }
naturalHeight={ media.media_details.height }
>
<img src={ media.data.source_url } alt={ __( 'Featured image' ) } />
<img src={ media.source_url } alt={ __( 'Featured image' ) } />
</ResponsiveWrapper>
}
{ media && media.isLoading && <Spinner /> }
{ ! media && <Spinner /> }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</Button>
) }
/>
Expand Down Expand Up @@ -96,14 +97,18 @@ const applyConnect = connect(
}
);

const applyWithAPIData = withAPIData( ( { featuredImageId, postTypeName } ) => {
const applyWithAPIData = withAPIData( ( { postTypeName } ) => {
return {
media: featuredImageId ? `/wp/v2/media/${ featuredImageId }` : undefined,
postType: postTypeName ? `/wp/v2/types/${ postTypeName }?context=edit` : undefined,
};
} );

const applyWithSelect = withSelect( ( select, { featuredImageId } ) => ( {
media: featuredImageId ? select( 'core' ).getMedia( featuredImageId ) : undefined,
} ) );

export default compose(
applyConnect,
applyWithAPIData,
applyWithSelect,
)( PostFeaturedImage );
1 change: 1 addition & 0 deletions lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ function gutenberg_register_scripts_and_styles() {
'wp-utils',
'wp-viewport',
'wp-plugins',
'wp-core-data',
'word-count',
'editor',
),
Expand Down